Thread: BUG #17083: [PATCH] PostgreSQL fails to build with OpenLDAP 2.5.x
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])
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/
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/
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
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