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

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

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

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

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

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

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/

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

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

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/

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

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

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&amp;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

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

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