Re: OpenLDAP's libldap and libldap_r

[Jeff Trawick <tr...@gmail.com>]

On Fri, Jan 23, 2009 at 11:14 AM, Joe Orton <jo...@redhat.com> wrote:

> On Fri, Jan 23, 2009 at 07:10:36AM -0500, Jeff Trawick wrote:
> > On Fri, Jan 23, 2009 at 7:00 AM, Eric Covener <co...@gmail.com> wrote:
> > > On Fri, Jan 23, 2009 at 3:46 AM, Graham Leggett <mi...@sharp.fm>
> wrote:
> > > > This is the first time I have heard of ldap_r. Is there a reason why
> we
> > > > can't just try bind to ldap_r first, and if that fails fall back to
> ldap?
> > >
> > > I think that's the way to go.
> >
> > Yeah, sounds right.
> >
> > FWIW, /usr/sbin/httpd.worker on RHEL 5 U2 loads just plain libldap (and
> > libldap_r is a distinct, and larger, library).  A naive observer (okay,
> me)
> > would have expected that to be an explosive combination that would have
> been
> > fixed long ago.
>
> From reading the OpenLDAP source code the libldap_r build appears to
> compile in a bunch of mutex locking calls around many/most of the ldap_*
> interfaces.
>
> It's not obvious to me whether:
>
> a) this is because those calls are in fact manipulating process-global
> state in some thread-unsafe way (not obvious how, if so), or
>
> b) this is enabling an additional API guarantee, that concurrent use of
> a single LDAP * object from multiple threads is safe.
>

(I had a reason to look at this in a little more detail; here are my notes.)

AFAICT, both a) and b) are true

Besides the more obvious issue b (above), the reentrant build

i. uses thread-safe forms of C library functions where available
ii. uses mutexes around calls to thread-unsafe library functions that can't
be avoided

and, perhaps suprisingly, the non-reentrant build purposefully forgets about
thread-safe forms of C library functions.

Example from util-int.c, which encapsulates a number of library functions
with thread safety issues:

#ifndef LDAP_R_COMPILE
# undef HAVE_REENTRANT_FUNCTIONS
# undef HAVE_CTIME_R
# undef HAVE_GETHOSTBYNAME_R
# undef HAVE_GETHOSTBYADDR_R

So, apart from the unforgotten detail of which flavor of libldap is linked
to by the code you want to combine:

Applications with multiple threads sharing LDAP connections need to use
libldap_r, for proper sharing of the connection.

Slightly less obvious:

Applications with multiple threads calling OpenLDAP need to use libldap_r,
even if they don't share LDAP connections; otherwise, resolver and certain
other calls made for independent LDAP connections will not be thread-safe
and may clobber global data owned by the system library.

Even less obvious are the implications for applications which use OpenLDAP
only on a single thread but have other threads running arbitrary code:

Generally, all code in a multi-threaded application should use threadsafe
forms of all library functions (you're broken as soon as you get two callers
to gethostbyname(), for example).  libldap_r is the build that maximizes the
use of threadsafe forms of library functions, so it needs to be used in this
case.  The mutexes used by libldap_r for callers of library functions with
no threadsafe form (or no form on this platform) won't protect against
non-OpenLDAP callers of the function (since they don't share that mutex), so
it is by no means perfect, but hopefully the number of non-threadsafe
library functions that must be used on modern systems is extremely small.



>
> I would assume any user of this apr-util API, hahaha sorry, let me start
> again....  I would assume that httpd does not rely on the additional API
> guarantee in (b).
>
> Since libldap and libldap_r seem to implement the same set of symbols
> (and without symbol versioning) this kind of issue is a minefield for
> downstream distributors, if apr-util/httpd link against libldap_r and
> some perl LDAP foo links against libldap, everything goes boom.
>

Downstream distributors shouldn't have any problem figuring this stuff out,
and they have at least some control of what libldap vs. libldap_r mean
anyway.

httpd's LDAP support has to use libldap_r instead of libldap within a
threaded MPM*, and other code which can be combined with it needs to
accommodate :(

*obviously it may not be practical to restrict the use of libldap_r to this
minimal extent

--/--

To sanity check that libldap really disables the reentrant forms of library
functions, I found the call to plain gethostbyname() with no protection of
the hostent structure in util-int.c and confirmed that it is indeed used
when building libldap.

#else
        *buf = NULL;
        *result = gethostbyname( name );
#error FOO        <- angry compiler

        if (*result!=NULL) {
                return 0;
        }

        *herrno_ptr = h_errno;

        return -1;
#endif

This is Solaris, where gethostbyname() simply cannot be used from multiple
threads.