- openldap-bugs - openldap.org

Re: (ITS#4989) Quirk in Dynlist overlay configuration
by ando＠sys-net.it 02 Jun '07

02 Jun '07

marg(a)rz.tu-clausthal.de wrote: > I found a behaviour issue with the dynlist overlay configuration: > > I tried the following configuration: > > overlay dynlist > dynlist-attrset posixGroup memberURL > dynlist-attrset groupOfURLs memberURL owner The reason of that check is that the same attribute "memberURL" could otherwise be used by both group classes to expand. > > but slaptest complains about it - and the slapd doesn't start with it: > > @(#) $OpenLDAP: slapd 2.3.34[...] > /usr/local/etc/openldap/slapd.conf: line 91: "dynlist-attrset <oc> > <URL-ad> [<member-ad>]": URL attributeDescription "memberURL" already > mapped. > slapd stopped. > > When I use multiple "overlay dynlist"-entries like: > > overlay dynlist > dynlist-attrset posixGroup memberURL > overlay dynlist > dynlist-attrset groupOfURLs memberURL owner > > it works as expected, but there is a warning: > > overlay_config(): warning, overlay "dynlist" already in list > > PS: Although the new Version 2.3.35 isn't available via FreeBSD Ports > yet, I don't think that it would change anything, because the source > file of the dynlist-overlay doesn't seem to have changed in any part > that matters to this issue. I think the documentation describes the intended behavior, but configuration parsing is not (no longer?) in agreement with the documentation. I think the real issue is that the behavior is no longer consistent for compares and searches: - for searches, only the first group that matches the entry's objectClass will be expanded - for compares, all groups that match will be expanded until a match is found I'm reworking the configuration test to be consistent with the documentation, and the code to be self-consistent: now all groups matching the entry are expanded. So now a configuration like dynlist-attrset groupOfURLs memberURL dynlist-attrset groupOfURLs memberURL member will simultaneously merge all attributes listed in a "memberURL", and add the expanded entries' DN as "member". However, I believe something like dynlist-attrset posixGroup memberURL dynlist-attrset groupOfURLs memberURL should still be forbiden, otherwise the same "memberURL" would expand twice. This, strictly speaking, is not an issue, as duplicates would simply be discarded, but it would cause unnecessary overhead. Right now, I have decided to turn this check into a config-time warning. Please test and report. p. Ing. Pierangelo Masarati OpenLDAP Core Team SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it --------------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Email: pierangelo.masarati(a)sys-net.it ---------------------------------------

1 0

Re: (ITS#4994) Clarify -y option in man pages
by ando＠sys-net.it 02 Jun '07

02 Jun '07

rra(a)stanford.edu wrote: > A Debian user was confused by the -y option and its inclusion of whitespace in > the password. The current man page text does explain this, but it doesn't make > a point of it, and I think a bit more explanation would be useful. Here's a > patch for the ldapcompare.1 man page; if you think this is a good idea, similar > changes could be made to all the other command-line utilities that take the -y > option. I have committed a different clarification that provides a hint to a portable manner to generate appropriate password files. Please check and improve; afterwards I'll apply it to other command line tools man pages. p. Ing. Pierangelo Masarati OpenLDAP Core Team SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it --------------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Email: pierangelo.masarati(a)sys-net.it ---------------------------------------

1 0

(ITS#4997) lmpasswd support using gcrypt
by rra＠stanford.edu 01 Jun '07

01 Jun '07

Full_Name: Russ Allbery Version: 2.4 (HEAD) OS: Debian URL: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=245341 Submission from: (NULL) (171.66.157.14) Now that 2.4 has native GnuTLS support, the patch that's been sitting around in Debian bug #245341 becomes potentially interesting (see the associated URL). This is a request to support LAN Manager password hashes when OpenLDAP is built with GnuTLS instead of OpenSSL, thus requiring using libgcrypt to do the DES work instead of OpenSSL's DES library. The patch in that bug almost certainly isn't okay in its current form, but I wanted to get the ITS filed for this feature request so that there's a record in the database and just in case someone else feels inspired to bring the patch up to date and clean it up. (Unlikely, I know.) Otherwise, I will probably clean this patch up for further submission at some point near the 2.4 release (on which side of it, I'm not sure).

1 0

Re: (ITS#4996) Use SRV records to locate local server for command-line clients
by kurt＠OpenLDAP.org 01 Jun '07

01 Jun '07

On Jun 2, 2007, at 5:31 AM, rra(a)stanford.edu wrote: > Full_Name: Russ Allbery > Version: 2.3.35 > OS: Debian > URL: > Submission from: (NULL) (171.66.157.14) > > > A user of the Debian OpenLDAP package requested support in the > command-line > utilities for using SRV entries to locate the local LDAP server. My > understanding of the suggestion is that if one didn't specify -h or > -H, a SRV > record lookup would be tried before falling back to localhost. > (You may not > want to change the default behavior, though, and add another switch.) One could use DNS SRV on the domain provided by -H, or by ldap.conf (5), and use it present, with (likely best) or without an option to enable the behavior. One could also use DNS SRV on the domain associated with the baseObject/target DN with an option to enable this behavior. That is, ldapsearch -b "dc=example,dc=org" would cause a DNS SRV lookup on example.org. This is what the DNSSRV backend does. Not sure adding to the command line tools would be especially useful. That is, I don't think DNS SRV fits well in the common use pattern of command line tools. But someone implements this behind an option, it shouldn't do any harm. Lastly I note that the domain to use DNS SRV should come from the user (or application entity), not the local host. Using the local resolver configuration is a really bad idea. -- Kurt > > For the full suggestion, see: > > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=221173 > > It looks like much of the necessary code is already there in > libldap, and > looking at the libldap code, you could also intuit the correct > server based on > any search base provided. > >

1 0

(ITS#4996) Use SRV records to locate local server for command-line clients
by rra＠stanford.edu 01 Jun '07

01 Jun '07

Full_Name: Russ Allbery Version: 2.3.35 OS: Debian URL: Submission from: (NULL) (171.66.157.14) A user of the Debian OpenLDAP package requested support in the command-line utilities for using SRV entries to locate the local LDAP server. My understanding of the suggestion is that if one didn't specify -h or -H, a SRV record lookup would be tried before falling back to localhost. (You may not want to change the default behavior, though, and add another switch.) For the full suggestion, see: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=221173 It looks like much of the necessary code is already there in libldap, and looking at the libldap code, you could also intuit the correct server based on any search base provided.

1 0

slapd with wrong url causes "Error detected by libpthread: Invalid mutex"
by Jean-Luc Wasmer 01 Jun '07

01 Jun '07

Hi, When slapd is started with an invalid url, it looks like it's trying to destroy an uninitialized mutex: Program received signal SIGABRT, Aborted. 0xbb87dfff in kill () from /usr/lib/libc.so.12 (gdb) backtrace #0 0xbb87dfff in kill () from /usr/lib/libc.so.12 #1 0xbb94adb3 in pthread__errorfunc () from /usr/lib/libpthread.so.0 #2 0xbb9491fe in pthread_mutex_destroy () from /usr/lib/libpthread.so.0 #3 0xbbbbd1eb in ldap_pvt_thread_mutex_destroy () from /usr/pkg/lib/libldap_r-2.3.so.0 #4 0x0805a756 in ?? () #5 0x0819e4fc in __ps_strings () #6 0x00000002 in ?? () #7 0xbfbfe8e8 in ?? () #8 0x0805a713 in ?? () #9 0x00000286 in ?? () #10 0x00000001 in ?? () #11 0xbfbfe958 in ?? () #12 0x0804e2c1 in ?? () #13 0xbfa00000 in ?? () #14 0xbb92cdd0 in tzname () from /usr/lib/libc.so.12 #15 0x00000002 in ?? () #16 0x00000000 in ?? () This was triggered by calling slapd with an invalid URLlist argument: -h 'ldap://127.0.0.1 ldaps://<external ip>' with no interfaces on the system configured with <external ip>

4 6

(ITS#4995) SampleLDAP.pm w/back_perl appears to be broken
by rra＠stanford.edu 01 Jun '07

01 Jun '07

Full_Name: Russ Allbery Version: 2.3.35 OS: Debian URL: Submission from: (NULL) (171.66.157.14) While investigating another issue with back_perl, I tried to use the SampleLDAP.pm module that comes with the OpenLDAP source. I added the following to my slapd.conf: moduleload back_perl # ... database perl suffix "o=AnyOrg,c=US" perlModulePath /home/eagle/SampleLDAP.pm perlModule SampleLDAP The result of slapd -d 1 is: ... WARNING: No dynamic config support for database perl. config_build_entry: "olcDatabase={2}perl" backend_startup_one: starting "dc=Stanford,dc=EDU" bdb_db_open: unclean shutdown detected; attempting recovery. bdb_db_open: dbenv_open(/var/lib/ldap) backend_startup_one: starting "o=AnyOrg,c=US" Can't call method "init" on an undefined value. I suspect the code in perl_back_db_open where it does: dSP; ENTER; SAVETMPS; PUSHMARK(sp); XPUSHs( perl_back->pb_obj_ref ); PUTBACK; #ifdef PERL_IS_5_6 count = call_method("init", G_SCALAR); #else count = perl_call_method("init", G_SCALAR); #endif The error message is consistent with perl_back->pb_obj_ref being an undefined value. Why it's undefined, though, I'm not sure. See also http://www.openldap.org/lists/openldap-software/200705/msg00126.html

1 0

(ITS#4994) Clarify -y option in man pages
by rra＠stanford.edu 01 Jun '07

01 Jun '07

Full_Name: Russ Allbery Version: 2.3.35 OS: Debian URL: Submission from: (NULL) (171.64.19.147) A Debian user was confused by the -y option and its inclusion of whitespace in the password. The current man page text does explain this, but it doesn't make a point of it, and I think a bit more explanation would be useful. Here's a patch for the ldapcompare.1 man page; if you think this is a good idea, similar changes could be made to all the other command-line utilities that take the -y option. --- ldapcompare.1 (revision 805) +++ ldapcompare.1 (working copy) @@ -106,8 +106,12 @@ Use \fIpasswd\fP as the password for simple authentication. .TP .BI \-y \ passwdfile -Use complete contents of \fIpasswdfile\fP as the password for -simple authentication. +Use complete contents of \fIpasswdfile\fP, including any leading or +trailing whitespace and any final newlines, as the password for +simple authentication. If the password does not contain a newline, +be careful when creating this file. Many editors and commands like +\fBecho\fP will add a trailing newline, which will then be taken as part +of the password. .TP .BI \-H \ ldapuri Specify URI(s) referring to the ldap server(s); only the protocol/host/port

1 0

Re: (ITS#4943) tpool.c pause vs. finish
by h.b.furuseth＠usit.uio.no 01 Jun '07

01 Jun '07

hyc(a)symas.com writes: >hyc(a)symas.com wrote: >> When we finally get the time to write up a new C API we can worry about >> where to draw the line between the library and slapd code but for now >> it's all (philosophically) private to slapd. > > (And of course, historically it made sense to put it in the library > because it was also used by slurpd. But that reason is no longer > pertinent...) It was? slurpd uses ldap_pvt_thread_create() directly in both RE23 and openldap-2.0.0. Grepping it for pool finds nothing. However it does use libldap_r/threads.c, which starts and stops a pool. Yet another matter, since I'm messing so much with tpool anyway: It's been confusing to have two types of contexts, thread and user contexts. I suggest to rename thread contexts to "tasks". They don't look very contexty to me anyway. As far as I can tell they are not exposed outside tpool.c, so it won't affect anything else. User contexts can be renamed back to plain contexts like in RE23. (The code is confused, too - ldap_int_main_thrctx and my DELETED_THREAD_CTX are user contexts, not thread contexts.) -- Regards, Hallvard

1 0

Re: (ITS#4943) tpool.c pause vs. finish
by hyc＠symas.com 01 Jun '07

01 Jun '07

hyc(a)symas.com wrote: > When we finally get the time to write up a new C API we can worry about > where to draw the line between the library and slapd code but for now > it's all (philosophically) private to slapd. (And of course, historically it made sense to put it in the library because it was also used by slurpd. But that reason is no longer pertinent...) -- -- Howard Chu Chief Architect, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

Re: (ITS#4943) tpool.c pause vs. finish
by hyc＠symas.com 01 Jun '07

01 Jun '07

h.b.furuseth(a)usit.uio.no wrote: > Howard Chu writes: >>> tpool.c breaks if there are multiple pools. The simplest >>> solution seems to be to remove support for multiple pools. >>> The problem is thread_keys[]. It is shared between pools, but: >> slapd only uses a single pool, and that's really the only use case we >> care about. In fact it makes no sense to support multiple pools, >> because we have no mechanism to assert scheduling priorities of one >> pool over another. > > I'm afraid I did not think of scheduling priorities at all, only bugs. > But it certainly makes things simpler if we remove multiple pools. > > Simple documentation of how tpool should be used may remove some of > my concerns - or how it _is_ used, since it's only used in slapd. > (Several probably only are problems if it is used as a library feature. > tpool depends on being used the way slapd is using it. I've wondered > what it's doing in libldap_r/ instead of in slapd/.) One thing to remember, which has been discussed before - all of libldap_r exists solely for the benefit of slapd. The only "official" LDAP API is what was defined in the expired C API draft, and that draft never addressed reentrancy issues. Any use of libldap_r outside of slapd is and always will be unsupported; any bugs that might exist solely due to unintended usage are not our concern. When we finally get the time to write up a new C API we can worry about where to draw the line between the library and slapd code but for now it's all (philosophically) private to slapd. -- -- Howard Chu Chief Architect, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

(ITS#4993) Roadmap very out of date
by bevan＠fulcrummicro.com 31 May '07

31 May '07

Full_Name: Bevan Bennett Version: NA OS: NA URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (66.59.247.209) The roadmap at http://www.openldap.org/software/roadmap.html has not been updated since January of 2006, and lists version 2.4 as being expected in "Q1 of 2006". Since we're now nearly done with Q2 of 2007, this is clearly not the case. Someone should pop in and update the roadmap to reflect the last 16 months of progress or lack thereof.

1 0

(ITS#4992) cldap ber_write assertion fix
by mba2000＠ioplex.com 31 May '07

31 May '07

Full_Name: Michael B Allen Version: 2.3.33 OS: Linux 2.6 URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (69.142.196.170) ldap_search_ext with cldap:// will assert because it calls ldap_build_search_req which calls ber_write w/ a NULL ld->ld_options.ldo_peer. The following patch defers encoding the address with ber_write until after the ldo_peer is initialized in request.c. Note that cldap functionality is still not optimal. The transport hangs if there is no reply. --- search.c-2007-05-30 2007-05-30 21:20:06.000000000 -0400 +++ search.c 2007-05-30 21:50:45.000000000 -0400 @@ -259,8 +259,9 @@ LDAP_NEXT_MSGID( ld, *idp ); #ifdef LDAP_CONNECTIONLESS if ( LDAP_IS_UDP(ld) ) { - err = ber_write( ber, ld->ld_options.ldo_peer, - sizeof(struct sockaddr), 0); + struct sockaddr sa; + memset(&sa, 0, sizeof sa); + err = ber_write( ber, &sa, sizeof(struct sockaddr), 0); } if ( LDAP_IS_UDP(ld) && ld->ld_options.ldo_version == LDAP_VERSION2) { char *dn = ld->ld_options.ldo_cldapdn; --- request.c-2007-05-30 2007-05-30 21:39:05.000000000 -0400 +++ request.c 2007-05-30 21:54:33.000000000 -0400 @@ -222,6 +222,19 @@ use_connection( ld, lc ); +#ifdef LDAP_CONNECTIONLESS + if ( LDAP_IS_UDP(ld) ) { + BerElement tmp = *ber; + ber_rewind( &tmp ); /* encode addr at start */ + rc = ber_write( &tmp, ld->ld_options.ldo_peer, + sizeof(struct sockaddr), 0); + if ( rc == -1 ) { + ld->ld_errno = LDAP_ENCODING_ERROR; + return( -1 ); + } + } +#endif + /* If we still have an incomplete write, try to finish it before * dealing with the new request. If we don't finish here, return * LDAP_BUSY and let the caller retry later. We only allow a single

1 0

Re: (ITS#4991) pcache and rwm don't play well together
by rhafer＠suse.de 31 May '07

31 May '07

Fix is in rwm.c (r1.85) and pcache.c (r1.105) -- Ralf

1 0

(ITS#4991) pcache and rwm don't play well together
by rhafer＠suse.de 31 May '07

31 May '07

Full_Name: Ralf Haferkamp Version: RE23, HEAD OS: Linux (Kernel 2.6) URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (89.166.181.19) The rwm and pcache overlays don't work very well together on a single database. The main reason for this seems to be that both overlays touch the list of requested Attributes in a search request and are not prepared for the fact that other overlays might touch that list as well. As a result the Entries returned by a Search against such a database can be incomplete and the server crashes because the various AttributeList arrays are free'd multiple times (or in the wrong place). I think I've a fix for HEAD is mostly ready, still testing it. I'll commit it shortly.

1 0

Re: (ITS#4943) tpool.c pause vs. finish
by h.b.furuseth＠usit.uio.no 31 May '07

31 May '07

hyc(a)symas.com wrote: >h.b.furuseth(a)usit.uio.no wrote: >> (...) >> But it would cleaner to ch_malloc an integer instead of the (void*) >> cast. Then it won't depend on integer->pointer->integer conversions >> preserving the value either. > > Ugh. That's just silly. We should just typedef a union that's big enough > to hold a pointer or an integer... Eh? Then we'd need to change every setkey/getkey call to use set/get that union instead of a simple pointer. > (...) >> Was that an instruction/permission to axe it or not? > > Yes, go ahead. Too late, I #ifdeffed instead:-) >> (...) >> I do not understand the calls to ldap_pvt_thread_pool_context_reset() >> in slapd (init.c and bconfig.c). That frees the thread's sl_malloc >> memory with slap_sl_mem_destroy(), which seems rather hazardous until >> the thread is really, really done. > > Those calls are from the main thread, and only occur during startup. And in slap_destroy(). > The point is to free up that memory since the main thread will never > need it again. Ah. I guess I need to figure out just what is allowed to use slap_sl_malloc() and what isn't. >> For that matter, I guess that means context_reset() had better release >> keys in the reverse order of how they were set. That usually happened, >> but not always, and my clear_key_idx() made the "not always" more >> common. So to close the gap from a deleted key I'll make it move all >> later keys forward, instead of just moving the latest key to the gap. > > I don't think the order is really important... ?? Well, I was thinking if some key could manage to get in front of the sl_malloc key and then be deleted, and then some key which sl_malloced memory ended up before it. But as noted above I don't have a clue:-) Also see tpool.c 1.30.2.17: "ITS#4805 free thread keys in reverse order". However I don't see from the ITS notes how it is related. >> Bdb uses the database environment address as a key to pool_setkey(). >> So there can be at most ~30 BDB databases with an active key at the >> same time in the same thread. What does that mean - 30 overlays >> all using bdb? Access control groups spanning 30 databases? > > Seems so. We could raise this limit if it's a problem, but I don't see > why anyone would use 30 separate databases instead of just one. > Configuring the caches would be a real bear... Well, it's the most natural way to set up an LDAP hotel - 1 database per hosted organization, where each org can configure its own indexes etc. But cache config is definitely an argument against it. It's no less strange to have e.g. 100 databases than 30, so I'm not suggesting to raise the limit in OpenLDAP. Though I suppose we could make it a default, overridden by CPPFLAGS=-DLDAP_THREAD_MAXKEYS=<num>. >> Nitpick: Slapd uses both function pointers and data pointers as keys to >> pool_[sg]etkey(). That makes it possible for two "different" keys to >> compare equal, on a host where functions and data live in different >> segments that otherwise share the same address space. So I guess I >> should clean up and replace (void*)<function> keys with the address of >> static char <function>_key; > > Hosts such as ... ? Not even OS/390 is so strange. Don't know, but it wouldn't surprise me if SINTRAN on ND-500 worked that way. What I do remember is that a program or library consisted of 2-3 files: program segment, data segment and optionally link segment. But it's likely less uncommon for function pointers to be wider than data pointers, which OpenLDAP doesn't support anyway. > The use of function pointers was just a convenience, since the address > is guaranteed to be a unique value within the process and easily > recognizable in a debugger. We could easily use addresses of static > variables instead but it's silly to introduce new variables just for > this purpose. Someday I want to revisit this and use OS-supported thread-specific keys, then it'll change anyway. -- Regards, Hallvard

1 0

(ITS#4990) memory debugging issues
by h.b.furuseth＠usit.uio.no 31 May '07

31 May '07

Full_Name: Hallvard B Furuseth Version: HEAD, RE23 OS: URL: Submission from: (NULL) (129.240.202.105) Submitted by: hallvard CPPFLAGS=-DLDAP_MEMORY_DEBUG won't build: ber_int_meminuse is not defined. sl_malloc.c needs s/ber_malloc_x/ber_memalloc_x/ under SLAP_NO_SL_MALLOC. There are both #if and #ifdef LDAP_MEMORY_DEBUG statements, but value 0 breaks compilation. LDAP_MEMORY_DEBUG enables a number of assert()s that break OpenLDAP code, like ldap_control_free(NULL). Until they are cleaned up (if they are still considered errors) I'll disable them by default unless LDAP_MEMORY_DEBUG & 2. Could do the opposite if someone wants. I'll insert LDAP_MEMORY_DEBUG code in slap_sl_malloc() as well (insert memory signature before each returned chunk, check upon free/realloc).

1 0

Re: (ITS#4943) tpool.c pause vs. finish
by hyc＠symas.com 31 May '07

31 May '07

h.b.furuseth(a)usit.uio.no wrote: > I wrote in the tpool.c 1.66 -> 1.67 patch: > >> API changes, need review - I'm not sure what's indented here: >> - setkey(data=NULL, kfree!=NULL) searched as if intended to reset >> the key, but updated by setting the key. Now always updates. > > Right. That matches the one place it can happen: In bdb_locker_id() > with setkey(,,data=(void *)(long)lockid,). I don't know if lockid > can be 0, but it doesn't need to for data to be NULL. lockid will never be zero. > But it would cleaner to ch_malloc an integer instead of the (void*) > cast. Then it won't depend on integer->pointer->integer conversions > preserving the value either. Ugh. That's just silly. We should just typedef a union that's big enough to hold a pointer or an integer... > BTW, is there a reason for the cast to long? lockid is u_int32_t, > and the getkey 'long' result is restored to a u_int32_t. I don't recall. > Howard Chu wrote: > >>> Also if ctx is invalid (another thread handled it) and a new >>> pending request has gotten the now-unused ctx, then the >>> 'if (pool->ltp_open_count == 0)' code will free that new >>> pending request instead of the old one. >> Most likely this is due to unlocking and relocking the mutex around >> the semaphore code. Axe that, remove the gratuitous unlock/relock, >> and then that problem goes away. > > Was that an instruction/permission to axe it or not? Yes, go ahead. > If not I'll just move the unlock/lock into the #ifdef for now. > >> Thus far we've only had to worry about a single pool. If anyone can >> come up with a reason to need multiple pools, I guess we need to >> hear it. > > I'll insert "static int initialized; assert(!initialized++);" > in pool_init() and see if anyone complains. Sounds fine. >>> - setkey does not free the old data with ltk_free. Possibly that's >>> intentional, but if so that requires weird code elsewhere. >> Yes, that's intentional. ltk_free is only intended for cleanup if a key >> is still set by the time a thread exits. All of the keys we set are live >> for the entire life of a thread, they don't get re-assigned. > > Even the (void*)slap_sasl_bind key? > lutil_passwd() looks like it can be recursive: Register a password > scheme which finds an entry in LDAP and authenticates via that > entry's userPassword - which again could call lutil_passwd(). > If that's legal, this code in be_isroot_pw()/slap_passwd_check() > can break: > pool_setkey(, slap_sasl_bind, <ctx>, <freefunc> ); > ... /* slap_passwd_check() does access_allowed() here */ > lutil_passwd(); > ... > pool_setkey(, slap_sasl_bind, NULL, NULL); > The final setkey would need to restore the old key. I guess so. > I'm not sure of contrib/slapd-modules/smbk5pwd/smbk5pwd.c + some > overlay either. rwm + back-relay which uses another smbk5pwd or > something. (And I have no idea of the BDB cache stuff, but since > you wrote it I assume you know:-) > > If it's a problem, we can replace code like that with > void *data = <ctx>; > ldap_pvt_thread_pool_keyfree_t *kfree = <freefunc>; > pool_swapkey(, slap_sasl_bind, &data, &kfree ); /* new function */ > lutil_passwd(); > pool_setkey(, slap_sasl_bind, data, kfree ); > or if setkey reverts to ignoring kfree when it checks whether we are > setting or deleting a key, the kfree function can be passed directly. > > > I wrote: > >> But this reminds me: How much should an ltk_free() function be allowed >> to do? >> - pool_context_reset() calls ltk_free(). Should pool_wrapper() go >> active (wait for !pause, increment ltp_active_count, unlock ltp_mutex) >> before calling pool_context_reset()? > > I think so. It makes slapi safer, and I don't see that it can hurt: > context_reset() can call connection_fake_destroy(), which via > slapi_int_free_object_extensions() calls slapi module destructors, > which can do who-knows-what. > > Another stray thought: It would similarly be safer for context_reset() > and purgekey() to delete the key before calling ltk_free, so that > function can't access the key's data after freeing it. OTOH that would > break any slapi module whose ltk_free calls a function which uses the > key before freeing it. I'll leave it alone unless someone says otherwise. > > > Other matters: > > pool->ltp_active_list is maintained but not used for anything. > Do I delete it? Or does tpool maintain the list to ease debugging? No idea. > Use of tpool: > > Another bug: > Slapd does not always check if pool_submit() and pool_setkey() succeed, > even though failure is not a bug. (They can run into some limits.) > > > I do not understand the calls to ldap_pvt_thread_pool_context_reset() > in slapd (init.c and bconfig.c). That frees the thread's sl_malloc > memory with slap_sl_mem_destroy(), which seems rather hazardous until > the thread is really, really done. Those calls are from the main thread, and only occur during startup. The point is to free up that memory since the main thread will never need it again. > For that matter, I guess that means context_reset() had better release > keys in the reverse order of how they were set. That usually happened, > but not always, and my clear_key_idx() made the "not always" more > common. So to close the gap from a deleted key I'll make it move all > later keys forward, instead of just moving the latest key to the gap. I don't think the order is really important... ?? > Bdb uses the database environment address as a key to pool_setkey(). > So there can be at most ~30 BDB databases with an active key at the > same time in the same thread. What does that mean - 30 overlays > all using bdb? Access control groups spanning 30 databases? Seems so. We could raise this limit if it's a problem, but I don't see why anyone would use 30 separate databases instead of just one. Configuring the caches would be a real bear... > Nitpick: Slapd uses both function pointers and data pointers as keys to > pool_[sg]etkey(). That makes it possible for two "different" keys to > compare equal, on a host where functions and data live in different > segments that otherwise share the same address space. So I guess I > should clean up and replace (void*)<function> keys with the address of > static char <function>_key; Hosts such as ... ? Not even OS/390 is so strange. The use of function pointers was just a convenience, since the address is guaranteed to be a unique value within the process and easily recognizable in a debugger. We could easily use addresses of static variables instead but it's silly to introduce new variables just for this purpose. -- -- Howard Chu Chief Architect, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

(ITS#4989) Quirk in Dynlist overlay configuration
by marg＠rz.tu-clausthal.de 31 May '07

31 May '07

Full_Name: Christian Marg Version: 2.3.34 OS: FreeBSD 6.2-RELEASE-p3 URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (91.3.23.106) I found a behaviour issue with the dynlist overlay configuration: I tried the following configuration: overlay dynlist dynlist-attrset posixGroup memberURL dynlist-attrset groupOfURLs memberURL owner but slaptest complains about it - and the slapd doesn't start with it: @(#) $OpenLDAP: slapd 2.3.34[...] /usr/local/etc/openldap/slapd.conf: line 91: "dynlist-attrset <oc> <URL-ad> [<member-ad>]": URL attributeDescription "memberURL" already mapped. slapd stopped. When I use multiple "overlay dynlist"-entries like: overlay dynlist dynlist-attrset posixGroup memberURL overlay dynlist dynlist-attrset groupOfURLs memberURL owner it works as expected, but there is a warning: overlay_config(): warning, overlay "dynlist" already in list PS: Although the new Version 2.3.35 isn't available via FreeBSD Ports yet, I don't think that it would change anything, because the source file of the dynlist-overlay doesn't seem to have changed in any part that matters to this issue.

1 0

Quirk in Dynlist overlay configuration
by Christian Marg 30 May '07

30 May '07

Hello, in the FAQ-o-Matic http://www.openldap.org/faq/index.cgi?file=1209 it says Unlike previous versions, you do not have to have multiple overlay dynlist entries in your configuration; multiple occurrences of the dynlist-attrpair/ dynlist-attrset statement must be used instead, with a caveat: only the first match on the dyn-oc is used. I tried this with the following configuration: overlay dynlist dynlist-attrset posixGroup memberURL dynlist-attrset groupOfURLs memberURL owner but slaptest complains about it - and the slapd doesn't start with it: @(#) $OpenLDAP: slapd 2.3.34[...] /usr/local/etc/openldap/slapd.conf: line 91: "dynlist-attrset <oc> <URL-ad> [<member-ad>]": URL attributeDescription "memberURL" already mapped. slapd stopped. When I use multiple "overlay dynlist"-entries like: overlay dynlist dynlist-attrset posixGroup memberURL overlay dynlist dynlist-attrset groupOfURLs memberURL owner it works as expected, even though there is a warning: overlay_config(): warning, overlay "dynlist" already in list BTW: Although the new Version 2.3.35 isn't available via FreeBSD Ports yet, I don't think that it would change anything, because the source file of the dynlist-overlay doesn't seem to have changed in any part that matters to this issue. bye Christian -- Christian Marg mail: mailto:marg@rz.tu-clausthal.de Rechenzentrum TU Clausthal web : http://www.rz.tu-clausthal.de D-38678 Clausthal-Zellerfeld fon : 05323/72-2043 Germany ICQ : <on request>

2 1

Re: (ITS#4980) misaligned memory access (openssl?)
by richton＠nbcs.rutgers.edu 30 May '07

30 May '07

I'm going to claim this as possible corruption because the RADIUS module was definitely overstepping bounds when this happened...feel free to close this out, I'll keep an eye out for recurrences. On Wed, 23 May 2007, Howard Chu wrote: > This stack trace is pretty exclusively OpenSSL and libc code. Unless there's > been a heap corruption somewhere, it appears to be an OpenSSL issue.

1 0

Re: (ITS#4951) [contrib] RADIUS password module fixes
by richton＠nbcs.rutgers.edu 30 May '07

30 May '07

I think this does it; I'm now happy with RADIUS in production. --- ldap-src/contrib/slapd-modules/passwd/radius.c 2007-05-30 19:38:29.210866000 -0400 +++ radius.c 2007-05-29 11:02:09.728936000 -0400 @@ -17,6 +17,7 @@ #include <lber.h> #include <lber_pvt.h> /* BER_BVC definition */ #include "lutil.h" +#include <portable.h> #include <ldap_pvt_thread.h> #include <ac/string.h> #include <ac/unistd.h> On Tue, 29 May 2007, Aaron Richton wrote: > Boy, that was some real quality time with dbx...ugh. > > > I'm fairly confident that the issue is that portable.h isn't included and > should have been as part of my patch. This worked in a development > environment; I'll be putting it through some more tests and, most likely, in > production soon (the =NULL workaround is just a time bomb).

1 0

Re: (ITS#4972) thr_debug problems
by h.b.furuseth＠usit.uio.no 30 May '07

30 May '07

I wrote: > RE23/thr_debug.c has some other problems that are fixed in HEAD, but > take a bit more work to fix and test. I'll align it with HEAD if > anyone is interested. Apparenlty not. I'll import the fixes local to thr_debug.c to RE23, but not the ones that need changes in include/. -- Regards, Hallvard

1 0

(ITS#4988) --disable-overlays breaks overlays/Makefile
by h.b.furuseth＠usit.uio.no 29 May '07

29 May '07

Full_Name: Hallvard B Furuseth Version: RE23 OS: HPUX URL: Submission from: (NULL) (129.240.202.105) Submitted by: hallvard 'make' on HP-UX does not like '\' - newline - blank line, produced by OBJS = overlays.o \ statover.o \ @SLAPD_STATIC_OVERLAYS@ in overlays/Makefile.in with --disable-overlays. Fixed in HEAD overlays/Makefile.in rev 1.46, importing to RE23 now.

1 0

Re: (ITS#4943) tpool.c pause vs. finish
by h.b.furuseth＠usit.uio.no 29 May '07

29 May '07

Some more comments, review, planned changes, and questions. I guess I should summarize the bugs I've fixed in HEAD first, for completeness. (I was a bit too busy at the time.) Restating the CVS log comments, nothing new but maybe a bit clearer. Possibly some of the issues are tpool-only issues and can't happen the way slapd uses tpool. ---------------------------------------- rev 1.63: ldap_pvt_thread_pool_submit(), after failed thread creation: - ltp_pending_count++ should be --. - should signal if there are no more threads, in case pool_destroy() is waiting for that. (Not sure if that can happen.) ldap_int_thread_pool_wrapper(): - The 'if(ltp_state...)' was spurious: It would always be true, and if it were not we'd get an eternal loop. rev 1.64: pool_pause(): - would cancel states FINISHING and STOPPING, pool_resume() would set it back to RUNNING. - could hang when waiting for another pool_pause(), since it did not signal ltp_pcond if it decremented ltp_active_count to 1. pool_wrapper(): - could update thread_keys[] during a pause while initializing and possibly while finishing, - just after initializing, could handle a new ctx during a pause. pool_destroy(): - would go on with destruction if pool_resume() was called, since it used an if(){wait} instead of while() and they use the same cond. rev 1.65: - pool_context() read thread_keys[] without mutex protection. pool_purgekey() too, but it expected a pause to protect the keys. pool_wrapper() protected with ltp_mutex while writing, and did not quite ensure a pause. rev 1.66: - thread_keys[] is a (poor) open-addessed hash table, but it lacked a "deleted item" mark. So keys (contexts) could become hidden and re-emerge later if threads exited and then new ones were started. - TID_HASH() did hash calculation in signed arithmetic, thus depending on friendly integer overflow. rev 1.67: - setkey/getkey expected ldap_int_thread_userctx_t.ltu_key[] to be NULL-terminated when not full. purgekey() could set a NULL in the middle, hiding later keys until a new key overwrote the NULL. API changes: - setkey(data=NULL, kfree!=NULL) searched as if intended to reset the key, but updated by setting the key. Now always updates. - setkey(key=<not found>, data=NULL) could return either success or failure. Now succeeds iff (data == NULL && kfree == NULL). rev 1.68: - ltp_max_count <= 0 meant allow an unlimited number of threads, but thread #LDAP_MAXTHR+1 and later would loop forever looking for a free thread_keys[] slot. (Not fixed for multi-pool applications.) ---------------------------------------- I wrote in the tpool.c 1.66 -> 1.67 patch: > API changes, need review - I'm not sure what's indented here: > - setkey(data=NULL, kfree!=NULL) searched as if intended to reset > the key, but updated by setting the key. Now always updates. Right. That matches the one place it can happen: In bdb_locker_id() with setkey(,,data=(void *)(long)lockid,). I don't know if lockid can be 0, but it doesn't need to for data to be NULL. But it would cleaner to ch_malloc an integer instead of the (void*) cast. Then it won't depend on integer->pointer->integer conversions preserving the value either. BTW, is there a reason for the cast to long? lockid is u_int32_t, and the getkey 'long' result is restored to a u_int32_t. Howard Chu wrote: >> Also if ctx is invalid (another thread handled it) and a new >> pending request has gotten the now-unused ctx, then the >> 'if (pool->ltp_open_count == 0)' code will free that new >> pending request instead of the old one. > > Most likely this is due to unlocking and relocking the mutex around > the semaphore code. Axe that, remove the gratuitous unlock/relock, > and then that problem goes away. Was that an instruction/permission to axe it or not? If not I'll just move the unlock/lock into the #ifdef for now. > Thus far we've only had to worry about a single pool. If anyone can > come up with a reason to need multiple pools, I guess we need to > hear it. I'll insert "static int initialized; assert(!initialized++);" in pool_init() and see if anyone complains. >> - setkey does not free the old data with ltk_free. Possibly that's >> intentional, but if so that requires weird code elsewhere. > > Yes, that's intentional. ltk_free is only intended for cleanup if a key > is still set by the time a thread exits. All of the keys we set are live > for the entire life of a thread, they don't get re-assigned. Even the (void*)slap_sasl_bind key? lutil_passwd() looks like it can be recursive: Register a password scheme which finds an entry in LDAP and authenticates via that entry's userPassword - which again could call lutil_passwd(). If that's legal, this code in be_isroot_pw()/slap_passwd_check() can break: pool_setkey(, slap_sasl_bind, <ctx>, <freefunc> ); ... /* slap_passwd_check() does access_allowed() here */ lutil_passwd(); ... pool_setkey(, slap_sasl_bind, NULL, NULL); The final setkey would need to restore the old key. I'm not sure of contrib/slapd-modules/smbk5pwd/smbk5pwd.c + some overlay either. rwm + back-relay which uses another smbk5pwd or something. (And I have no idea of the BDB cache stuff, but since you wrote it I assume you know:-) If it's a problem, we can replace code like that with void *data = <ctx>; ldap_pvt_thread_pool_keyfree_t *kfree = <freefunc>; pool_swapkey(, slap_sasl_bind, &data, &kfree ); /* new function */ lutil_passwd(); pool_setkey(, slap_sasl_bind, data, kfree ); or if setkey reverts to ignoring kfree when it checks whether we are setting or deleting a key, the kfree function can be passed directly. I wrote: > But this reminds me: How much should an ltk_free() function be allowed > to do? > - pool_context_reset() calls ltk_free(). Should pool_wrapper() go > active (wait for !pause, increment ltp_active_count, unlock ltp_mutex) > before calling pool_context_reset()? I think so. It makes slapi safer, and I don't see that it can hurt: context_reset() can call connection_fake_destroy(), which via slapi_int_free_object_extensions() calls slapi module destructors, which can do who-knows-what. Another stray thought: It would similarly be safer for context_reset() and purgekey() to delete the key before calling ltk_free, so that function can't access the key's data after freeing it. OTOH that would break any slapi module whose ltk_free calls a function which uses the key before freeing it. I'll leave it alone unless someone says otherwise. Other matters: pool->ltp_active_list is maintained but not used for anything. Do I delete it? Or does tpool maintain the list to ease debugging? Use of tpool: Another bug: Slapd does not always check if pool_submit() and pool_setkey() succeed, even though failure is not a bug. (They can run into some limits.) I do not understand the calls to ldap_pvt_thread_pool_context_reset() in slapd (init.c and bconfig.c). That frees the thread's sl_malloc memory with slap_sl_mem_destroy(), which seems rather hazardous until the thread is really, really done. For that matter, I guess that means context_reset() had better release keys in the reverse order of how they were set. That usually happened, but not always, and my clear_key_idx() made the "not always" more common. So to close the gap from a deleted key I'll make it move all later keys forward, instead of just moving the latest key to the gap. Bdb uses the database environment address as a key to pool_setkey(). So there can be at most ~30 BDB databases with an active key at the same time in the same thread. What does that mean - 30 overlays all using bdb? Access control groups spanning 30 databases? Nitpick: Slapd uses both function pointers and data pointers as keys to pool_[sg]etkey(). That makes it possible for two "different" keys to compare equal, on a host where functions and data live in different segments that otherwise share the same address space. So I guess I should clean up and replace (void*)<function> keys with the address of static char <function>_key; -- Regards, Hallvard

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

openldap-bugs