slapd with wrong url causes "Error detected by libpthread: Invalid mutex"
by Jean-Luc Wasmer
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>
15 years, 9 months
(ITS#4992) cldap ber_write assertion fix
by mba2000＠ioplex.com
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
15 years, 9 months
(ITS#4991) pcache and rwm don't play well together
by rhafer＠suse.de
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.
15 years, 9 months
Re: (ITS#4943) tpool.c pause vs. finish
by h.b.furuseth＠usit.uio.no
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
15 years, 9 months
(ITS#4990) memory debugging issues
by h.b.furuseth＠usit.uio.no
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).
15 years, 9 months
Re: (ITS#4943) tpool.c pause vs. finish
by hyc＠symas.com
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/
15 years, 9 months
(ITS#4989) Quirk in Dynlist overlay configuration
by marg＠rz.tu-clausthal.de
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.
15 years, 9 months
Quirk in Dynlist overlay configuration
by Christian Marg
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>
15 years, 9 months
