Horton

Thread

Commits

  1. Fix busted test for ldap_initialize.

  2. Un-break AIX build, take 2.

  3. Un-break AIX build.

  4. Update configure's probe for libldap to work with OpenLDAP 2.5.

  1. BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    The Post Office <noreply@postgresql.org> — 2021-07-06T11:42:34Z

    The following bug has been logged on the website:

Bug reference:      17083
Logged by:          Adrian Ho
Email address:      ml+postgresql@03s.net
PostgreSQL version: 13.3
Operating system:   Ubuntu 20.04.2 LTS
Description:        

OpenLDAP 2.5 merged the `ldap_r` and `ldap` libraries, so all per-handle
routines are now thread-safe by default. However, the removal of `ldap_r`
results in the following `configure` error:
```
checking for ldap_bind in -lldap... yes
checking for ldap_simple_bind in -lldap_r... no
configure: error: library 'ldap_r' is required for LDAP
```
This patch therefore fixes and clarifies the LDAP library detection
logic, by first selecting on thread-safety requirements, then looking
for the appropriate libraries. The underlying assumption is that for a
pre-2.5 setup, both `ldap` and `ldap_r` libraries are installed, so it
should only fall back to (thread-safe) `ldap` in a 2.5.x installation.

diff --git a/configure.in b/configure.in
index 14a6be6..02ad260 100644
--- a/configure.in
+++ b/configure.in
@@ -1241,17 +1241,19 @@ fi
 if test "$with_ldap" = yes ; then
   _LIBS="$LIBS"
   if test "$PORTNAME" != "win32"; then
-    AC_CHECK_LIB(ldap, ldap_bind, [],
-		 [AC_MSG_ERROR([library 'ldap' is required for LDAP])],
-		 [$EXTRA_LDAP_LIBS])
-    LDAP_LIBS_BE="-lldap $EXTRA_LDAP_LIBS"
     if test "$enable_thread_safety" = yes; then
       # on some platforms ldap_r fails to link without PTHREAD_LIBS
-      AC_CHECK_LIB(ldap_r, ldap_simple_bind, [],
-		   [AC_MSG_ERROR([library 'ldap_r' is required for LDAP])],
+      # OpenLDAP 2.5 merged ldap_r with ldap
+      AC_SEARCH_LIBS(ldap_simple_bind, [ldap_r ldap], [],
+		   [AC_MSG_ERROR([not found in any LDAP library])],
 		   [$PTHREAD_CFLAGS $PTHREAD_LIBS $EXTRA_LDAP_LIBS])
-      LDAP_LIBS_FE="-lldap_r $EXTRA_LDAP_LIBS"
+      LDAP_LIBS_BE="-lldap $EXTRA_LDAP_LIBS"
+      LDAP_LIBS_FE="${ac_lib:+-l}$ac_lib $EXTRA_LDAP_LIBS"
     else
+      AC_CHECK_LIB(ldap, ldap_bind, [],
+		   [AC_MSG_ERROR([library 'ldap' is required for LDAP])],
+		   [$EXTRA_LDAP_LIBS])
+      LDAP_LIBS_BE="-lldap $EXTRA_LDAP_LIBS"
       LDAP_LIBS_FE="-lldap $EXTRA_LDAP_LIBS"
     fi
     AC_CHECK_FUNCS([ldap_initialize])

  2. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-06T13:46:41Z

    PG Bug reporting form <noreply@postgresql.org> writes:
> OpenLDAP 2.5 merged the `ldap_r` and `ldap` libraries, so all per-handle
> routines are now thread-safe by default.

Hm, does 2.5 exist in the wild yet?  I checked Fedora rawhide, which
is my usual go-to platform for bleeding-edge stuff, but they are still
at 2.4.58.

As for the patch itself, I'm wondering about

+      LDAP_LIBS_FE="${ac_lib:+-l}$ac_lib $EXTRA_LDAP_LIBS"

That seems undesirably intimate with the implementation details
of AC_SEARCH_LIBS.  Surely there's a better way?

			regards, tom lane

  3. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Adrian Ho <ml+postgresql@03s.net> — 2021-07-06T14:22:00Z

    On 6/7/21 9:46 pm, Tom Lane wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
>> OpenLDAP 2.5 merged the `ldap_r` and `ldap` libraries, so all per-handle
>> routines are now thread-safe by default.
> 
> Hm, does 2.5 exist in the wild yet?  I checked Fedora rawhide, which
> is my usual go-to platform for bleeding-edge stuff, but they are still
> at 2.4.58.

Gentoo and some other distros have pushed 2.5.5 out: 
https://repology.org/project/openldap/badges. This patch actually grew 
out of fixing the Homebrew Linux PostgreSQL build: 
https://github.com/Homebrew/homebrew-core/pull/80643

> As for the patch itself, I'm wondering about
> 
> +      LDAP_LIBS_FE="${ac_lib:+-l}$ac_lib $EXTRA_LDAP_LIBS"
> 
> That seems undesirably intimate with the implementation details
> of AC_SEARCH_LIBS.  Surely there's a better way?

Hmmm, good point, my Autotools-fu is not very strong. I'll see if 
there's a blessed way of doing the above, otherwise I'll probably have 
to test each library separately in an AC_CHECK_LIB cascade instead.

-- 
Best Regards,
Adrian

  4. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-06T14:47:45Z

    Adrian Ho <ml+postgresql@03s.net> writes:
> On 6/7/21 9:46 pm, Tom Lane wrote:
>> As for the patch itself, I'm wondering about
>> +      LDAP_LIBS_FE="${ac_lib:+-l}$ac_lib $EXTRA_LDAP_LIBS"
>> That seems undesirably intimate with the implementation details
>> of AC_SEARCH_LIBS.  Surely there's a better way?

> Hmmm, good point, my Autotools-fu is not very strong. I'll see if 
> there's a blessed way of doing the above, otherwise I'll probably have 
> to test each library separately in an AC_CHECK_LIB cascade instead.

Looking at the Autoconf docs, what AC_SEARCH_LIBS is specified to do is
"Prepend `-lLIBRARY' to `LIBS' for the first library found to contain
FUNCTION".  So I'd suggest

* Save contents of LIBS and set it to empty
* Run AC_SEARCH_LIBS
* LDAP_LIBS_FE="$LIBS $EXTRA_LDAP_LIBS"
* Restore LIBS

I think we have some instances of that pattern already.

			regards, tom lane

  5. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Adrian Ho <ml+postgresql@03s.net> — 2021-07-07T07:54:59Z

    On 6/7/21 10:47 pm, Tom Lane wrote:
> Looking at the Autoconf docs, what AC_SEARCH_LIBS is specified to do is
> "Prepend `-lLIBRARY' to `LIBS' for the first library found to contain
> FUNCTION".  So I'd suggest
> 
> * Save contents of LIBS and set it to empty
> * Run AC_SEARCH_LIBS
> * LDAP_LIBS_FE="$LIBS $EXTRA_LDAP_LIBS"
> * Restore LIBS
> 
> I think we have some instances of that pattern already.

Thanks, Tom, that turned out to only require one additional line, since 
$LIBS is already being saved and restored around that block:

diff --git a/configure.in b/configure.in
index 14a6be6..842d61b 100644
--- a/configure.in
+++ b/configure.in
@@ -1241,17 +1241,20 @@ fi
  if test "$with_ldap" = yes ; then
    _LIBS="$LIBS"
    if test "$PORTNAME" != "win32"; then
-    AC_CHECK_LIB(ldap, ldap_bind, [],
-		 [AC_MSG_ERROR([library 'ldap' is required for LDAP])],
-		 [$EXTRA_LDAP_LIBS])
-    LDAP_LIBS_BE="-lldap $EXTRA_LDAP_LIBS"
      if test "$enable_thread_safety" = yes; then
        # on some platforms ldap_r fails to link without PTHREAD_LIBS
-      AC_CHECK_LIB(ldap_r, ldap_simple_bind, [],
-		   [AC_MSG_ERROR([library 'ldap_r' is required for LDAP])],
+      # OpenLDAP 2.5 merged ldap_r with ldap
+      LIBS=""
+      AC_SEARCH_LIBS(ldap_simple_bind, [ldap_r ldap], [],
+		   [AC_MSG_ERROR([not found in any LDAP library])],
  		   [$PTHREAD_CFLAGS $PTHREAD_LIBS $EXTRA_LDAP_LIBS])
-      LDAP_LIBS_FE="-lldap_r $EXTRA_LDAP_LIBS"
+      LDAP_LIBS_BE="-lldap $EXTRA_LDAP_LIBS"
+      LDAP_LIBS_FE="$LIBS $EXTRA_LDAP_LIBS"
      else
+      AC_CHECK_LIB(ldap, ldap_bind, [],
+		   [AC_MSG_ERROR([library 'ldap' is required for LDAP])],
+		   [$EXTRA_LDAP_LIBS])
+      LDAP_LIBS_BE="-lldap $EXTRA_LDAP_LIBS"
        LDAP_LIBS_FE="-lldap $EXTRA_LDAP_LIBS"
      fi
      AC_CHECK_FUNCS([ldap_initialize])


-- 
Best Regards,
Adrian

  6. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-07T16:57:31Z

    Adrian Ho <ml+postgresql@03s.net> writes:
> Thanks, Tom, that turned out to only require one additional line, since 
> $LIBS is already being saved and restored around that block:

I poked at this a bit further and realized that it's got a showstopper
problem: in OpenLDAP 2.4, ldap_simple_bind() exists in both libldap.so
and libldap_r.so.  Thus, if we were dealing with an installation that
does not have a thread-safe libldap, the patched configure would
incorrectly seize on libldap.so as being an acceptable substitute,
allowing weird hard-to-diagnose failures at runtime.

IOW, while it might look from our existing coding like there's something
about ldap_simple_bind() that is tied to reentrancy, there isn't.  How
else can we determine whether an OpenLDAP library is reentrant, if we
can no longer depend on the _r suffix?

Maybe we can decide that in 2021 such situations no longer exist in
the wild, but I'm unsure.

			regards, tom lane

  7. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Daniel Gustafsson <daniel@yesql.se> — 2021-07-08T07:53:11Z

    > On 7 Jul 2021, at 18:57, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Adrian Ho <ml+postgresql@03s.net> writes:
>> Thanks, Tom, that turned out to only require one additional line, since 
>> $LIBS is already being saved and restored around that block:
> 
> I poked at this a bit further and realized that it's got a showstopper
> problem: in OpenLDAP 2.4, ldap_simple_bind() exists in both libldap.so
> and libldap_r.so.  Thus, if we were dealing with an installation that
> does not have a thread-safe libldap, the patched configure would
> incorrectly seize on libldap.so as being an acceptable substitute,
> allowing weird hard-to-diagnose failures at runtime.
> 
> IOW, while it might look from our existing coding like there's something
> about ldap_simple_bind() that is tied to reentrancy, there isn't.  How
> else can we determine whether an OpenLDAP library is reentrant, if we
> can no longer depend on the _r suffix?

Couldn't we add a version check to find if we have < 2.5 or >= 2.5, and adjust
libs accordingly?  Alternatively we could perhaps check for
LDAP_API_FEATURE_X_OPENLDAP_REENTRANT which IIUC when set defines -lldap to be
reentrant.

--
Daniel Gustafsson		https://vmware.com/

  8. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Michael Paquier <michael@paquier.xyz> — 2021-07-08T08:04:54Z

    On Thu, Jul 08, 2021 at 09:53:11AM +0200, Daniel Gustafsson wrote:
> Couldn't we add a version check to find if we have < 2.5 or >= 2.5, and adjust
> libs accordingly?  Alternatively we could perhaps check for
> LDAP_API_FEATURE_X_OPENLDAP_REENTRANT which IIUC when set defines -lldap to be
> reentrant.

Would you fetch the version number directly from the library?  I'd
like to think that it could be more simple to rely on an object
defined in the library instead as that's what we do for OpenSSL.  Now
OpenLDAP has not a history with forks as complicated as OpenSSL,
making a version number something more reliable?
--
Michael

  9. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-08T19:55:27Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Jul 08, 2021 at 09:53:11AM +0200, Daniel Gustafsson wrote:
>> Couldn't we add a version check to find if we have < 2.5 or >= 2.5, and adjust
>> libs accordingly?  Alternatively we could perhaps check for
>> LDAP_API_FEATURE_X_OPENLDAP_REENTRANT which IIUC when set defines -lldap to be
>> reentrant.

> Would you fetch the version number directly from the library?

Yeah, that all sounds kind of fragile.

On investigation it seems that libldap_r has been around basically
forever.  Even my second-oldest buildfarm animal prairiedog has got
it (indeed libldap.dylib and libldap_r.dylib are symlinks to
the same file on that platform ... hmm).  My oldest animal gaur
is irrelevant to this discussion, because we don't support
--enable-thread-safety on it in the first place.

That being the case, I think we might be okay just going with the
solution in Adrian's last patch, ie use libldap_r if it's there
and otherwise assume that libldap is thread-safe.  It looks
fairly unlikely that anyone will ever have an issue with that;
while if we complicate the configure probe, we might be introducing
new problems just from that.

			regards, tom lane

  10. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Daniel Gustafsson <daniel@yesql.se> — 2021-07-08T20:02:30Z

    > On 8 Jul 2021, at 21:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> That being the case, I think we might be okay just going with the
> solution in Adrian's last patch, ie use libldap_r if it's there
> and otherwise assume that libldap is thread-safe.  It looks
> fairly unlikely that anyone will ever have an issue with that;
> while if we complicate the configure probe, we might be introducing
> new problems just from that.

Absolutely, in light of that I agree that's the best option.

--
Daniel Gustafsson		https://vmware.com/

  11. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Michael Paquier <michael@paquier.xyz> — 2021-07-09T02:25:03Z

    On Thu, Jul 08, 2021 at 10:02:30PM +0200, Daniel Gustafsson wrote:
> On 8 Jul 2021, at 21:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That being the case, I think we might be okay just going with the
>> solution in Adrian's last patch, ie use libldap_r if it's there
>> and otherwise assume that libldap is thread-safe.  It looks
>> fairly unlikely that anyone will ever have an issue with that;
>> while if we complicate the configure probe, we might be introducing
>> new problems just from that.
> 
> Absolutely, in light of that I agree that's the best option.

+1.
--
Michael

  12. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-09T16:40:16Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Jul 08, 2021 at 10:02:30PM +0200, Daniel Gustafsson wrote:
>> On 8 Jul 2021, at 21:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> That being the case, I think we might be okay just going with the
>>> solution in Adrian's last patch, ie use libldap_r if it's there
>>> and otherwise assume that libldap is thread-safe.

>> Absolutely, in light of that I agree that's the best option.

> +1.

Done that way.

			regards, tom lane

  13. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Michael Paquier <michael@paquier.xyz> — 2021-07-10T11:56:17Z

    On Fri, Jul 09, 2021 at 12:40:16PM -0400, Tom Lane wrote:
> Done that way.

This broke LDAPS, and the regression tests of src/test/ldap/.  Elver
is one animal complaining, but I am able to reproduce this failure on
my own Debian laptop:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=elver&dt=2021-07-09%2023%3A37%3A13
 not ok 26 - LDAPS

#   Failed test 'LDAPS'
#   at t/001_auth.pl line 174.
#          got: '2'
#     expected: '0'
--
Michael

  14. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-10T14:50:29Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Jul 09, 2021 at 12:40:16PM -0400, Tom Lane wrote:
>> Done that way.

> This broke LDAPS, and the regression tests of src/test/ldap/.  Elver
> is one animal complaining, but I am able to reproduce this failure on
> my own Debian laptop:

Oh, that's interesting.  elver's failure seems to be because it's now
deciding it doesn't HAVE_LDAP_INITIALIZE.  However, on my RHEL8 machine
the current configure coding is still finding ldap_initialize; so I'd
assumed this was something BSD-specific.  If you're seeing it on Debian
then I'm really confused about what the problem is.

			regards, tom lane

  15. Re: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-10T17:25:33Z

    I wrote:
> Oh, that's interesting.  elver's failure seems to be because it's now
> deciding it doesn't HAVE_LDAP_INITIALIZE.  However, on my RHEL8 machine
> the current configure coding is still finding ldap_initialize;

... or not.  I must have been seeing what I expected to see yesterday,
because when I check it again now, it isn't finding ldap_initialize.

Close inspection of the Autoconf manual reveals the problem:
AC_CHECK_LIB updates LIBS only as part of its *default* success
action, which the current coding isn't using for libldap_r.
So we arrive at the ldap_initialize test with neither library
in LIBS.

But we really ought to probe libldap not libldap_r for
ldap_initialize, because we use that only on the backend side.
So the right fix is to do that probe before we mess around
with libldap_r, which I've now done.

			regards, tom lane