openldap-bugs December 2010

openldap-bugs@openldap.org

32 participants
128 discussions

Re: (ITS#6625) draft-zeilenga-ldap-c-api-concurrency support
by doug.leavitt＠oracle.com 06 Dec '10

06 Dec '10

On 12/ 6/10 04:09 AM, h.b.furuseth(a)usit.uio.no wrote: > I've made two fixes (I hope) to the concurrency commit, please review: > > - In do_abandon(), protected ldap_free_connection() with > ld->ld_conn_mutex. The function would unlock it when it did not own > the mutex. > > However I do not know why the mutex was moved out of > ldap_free_connection(), so maybe something more needs to be done. > The abandon operation is now protected by it's own abandon mutex, not by the conn mutex. So do_abandon is not protecting the conn unless it is actually in use. ldap_free_connection reference counts the connection and only frees it if there are no other users of the connection. This is why ldap_free_connection requires protection. If there are multiple simultaneous threads using the connection some of which may have in flight operations, those threads are not interrupted by the abandon of another thread. Consider connection A with 3 threads performing searches. Thread 3 may abandon it's search but threads 1 and 2 may continue with their operations. Removing the protection around ldap_free_connection will disrupt the reference counting for the connection and cause potential connection leakage and/or disrupt other threads usage of the connection. Doug.

1 0

Re: (ITS#6625) draft-zeilenga-ldap-c-api-concurrency support
by h.b.furuseth＠usit.uio.no 06 Dec '10

06 Dec '10

I've made two fixes (I hope) to the concurrency commit, please review: - I inverted the test in NT threads MUTEX_FIRSTCREATE. I assume LDO_MUTEX_NULL = (HANDLE)0 is not a valid handle, so FIRSTCREATE must initialize m if it *is* 0, not if is *not*. - In do_abandon(), protected ldap_free_connection() with ld->ld_conn_mutex. The function would unlock it when it did not own the mutex. However I do not know why the mutex was moved out of ldap_free_connection(), so maybe something more needs to be done. Also I've cleaned up macros and namespace: Add proper values for NO_THREADS. Support LDAP_THREAD_DEBUG. Define LDAP_INT_* and LDAP_DEBUG_* in ldap_int_thread.h, LDAP_PVT_* in ldap_pvt_thread.h. libraries/libldap/ldap-int.h: Remove unnecessary #ifdef LDAP_R_COMPILE for LDAP_NEXT_MSGID. define LDAP_LDO_MUTEX_NULLARG as ,LDAP_PVT_MUTEX_NULL or empty in the same place which decides whether the initializer is needed. Wrap some macros and arguments in parens or do{}while(0), so they can be used more like C statements. Also replace empty macros with ((void) 0) expansions, to shut up 'empty statement' compiler warnings. Finally, I've another wish for the test060-mt-hot: This test takes way to long under Valgrind. It ought to obey $TESTLOOPS (default 50) like the concurrency tests do: Set parameters to run ca $TESTLOOPS/50 of the current time. I don't know which parameters it's best to reduce in that case, however? -- Hallvard

1 0

Re: (ITS#6737) slapmodify strangeness
by masarati＠aero.polimi.it 06 Dec '10

06 Dec '10

> >> slapdmodify(8) is the (nearly untested) result of non negligible >> cut'n'paste. > > In any case, there seems to be significant side code commonality between > slapadd and slapmodify, basically everything related to syncing the > context entry. Maybe we should move that code in helpers in slapcommon.c. See <ftp://ftp.openldap.org/incoming/pierangelo.masarati-2010-12-06-update_ctxcs…>. p.

1 0

Re: (ITS#6737) slapmodify strangeness
by masarati＠aero.polimi.it 06 Dec '10

06 Dec '10

> slapdmodify(8) is the (nearly untested) result of non negligible > cut'n'paste. In any case, there seems to be significant side code commonality between slapadd and slapmodify, basically everything related to syncing the context entry. Maybe we should move that code in helpers in slapcommon.c. p.

1 0

Re: (ITS#6620) ppolicy: pwdChangedTime/userPassword delete issue.
by h.b.furuseth＠usit.uio.no 06 Dec '10

06 Dec '10

hyc(a)symas.com writes: > See the definition of pwdChangedTime, section 5.3.2: Duh, I misread what you said. In any case, it's a separate issue. That's about slapd expiring the passwor, this patch is about the user deleting it. -- Hallvard

1 0

(ITS#6739) broken do_syncrep2()
by h.b.furuseth＠usit.uio.no 06 Dec '10

06 Dec '10

Full_Name: Hallvard B Furuseth Version: HEAD OS: URL: Submission from: (NULL) (129.240.6.233) Submitted by: hallvard slapd/syncrepl.c:do_syncrep2() initializes many variables once which should have been initialized inside the loop. I'll add the initializations I can figure out, and move most variables into the loop for readability. TODO: Variables 'm' and 'refreshDeletes' may still be wrong. In particular 'm'. It "feels" like it should be local to the loop, but there's no logic that I can see to prevent previous value from being used. Perhaps they depend on the peer not breaking the sync protocol? I haven't read that myself, I'm just peering at the code. I've added three somewhat arbitrary bits of code for error handling: - A Debug() statement on line 847 which can likely be improved. - Set err and rc to LDAP_OTHER on ldap_parse_result/ldap_get_option failure. The correct fix would be proper error checks (ITS#6738), but I don't know what to do on error.

1 0

Re: (ITS#6737) slapmodify strangeness
by masarati＠aero.polimi.it 06 Dec '10

06 Dec '10

> Full_Name: Hallvard B Furuseth > Version: HEAD > OS: > URL: > Submission from: (NULL) (129.240.6.233) > Submitted by: hallvard > > > 'sid' in slapmodify.c is read uninitialized before it is written, at line > 693: > > if ( rc == EXIT_SUCCESS && update_ctxcsn && !dryrun && sid != > SLAP_SYNC_SID_MAX > + 1 ) { > > A loop _below_ that statement can set sid to the expected value: > > for ( sid = 0; sid <= SLAP_SYNC_SID_MAX; sid++ ) > > Also slapmodify sets up but does not use variables opbuf and op. slapdmodify(8) is the (nearly untested) result of non negligible cut'n'paste. p.

1 0

(ITS#6738) missing checks for ber errors
by h.b.furuseth＠usit.uio.no 06 Dec '10

06 Dec '10

Full_Name: Hallvard B Furuseth Version: HEAD OS: URL: Submission from: (NULL) (129.240.6.233) Submitted by: hallvard There are a fair number of ber calls which do not check for errors: Just ber_scanf and use the expected output variable, which may or may not have a useful NULL value on error. Syncrepl is particularly bad. This in slapd/ and libldap/: egrep '^<tab>*ber_(scanf|get_)' *.c */*.c returns mostly syncrepl matches. I can't even fix these easily because there's not enough error handling code to copy. grepping for ber_(printf|put_) find other modules as well.

1 0

(ITS#6737) slapmodify strangeness
by h.b.furuseth＠usit.uio.no 05 Dec '10

05 Dec '10

Full_Name: Hallvard B Furuseth Version: HEAD OS: URL: Submission from: (NULL) (129.240.6.233) Submitted by: hallvard 'sid' in slapmodify.c is read uninitialized before it is written, at line 693: if ( rc == EXIT_SUCCESS && update_ctxcsn && !dryrun && sid != SLAP_SYNC_SID_MAX + 1 ) { A loop _below_ that statement can set sid to the expected value: for ( sid = 0; sid <= SLAP_SYNC_SID_MAX; sid++ ) Also slapmodify sets up but does not use variables opbuf and op.

1 0

(ITS#6736) Listener info destroyed too early on shutdown
by h.b.furuseth＠usit.uio.no 05 Dec '10

05 Dec '10

Full_Name: Hallvard B Furuseth Version: HEAD OS: URL: Submission from: (NULL) (129.240.6.233) Submitted by: hallvard On shutdown, slapd_daemon_task() calls close_listeners() which destroys slap_listeners[] while other threads may be accessing it. This is not mutex-protected. Valgrind reports freed data accesses in back-monitor and slapd_listener() when they try to read the URLs in the listeners. Giving HEAD a minimal fix: Stop listening but delay destruction. If anyone feels like it, the new 'listening' flag probably can be used more intelligently, e.g. by rearranging the use of it and some shutdown flags.

1 0

← Newer
1
...
7
8
9
10
11
12
13
Older →

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

openldap-bugs December 2010