On Thu, Jun 19, 2014 at 01:01:34PM -0400, Tom Lane wrote:
> Noah Misch <n...@leadboat.com> writes:
> > On Thu, Jun 12, 2014 at 05:02:19PM -0400, Noah Misch wrote:
> >> You can cause the at-exit crash by building PostgreSQL against OpenLDAP
> >> 2.4.31, connecting with LDAP authentication, and issuing "LOAD 'dblink'".
>
> >> 4. Detect older OpenLDAP versions at runtime, just before we would
> >> otherwise
> >> initialize OpenLDAP, and raise an error. Possibly make the same check at
> >> compile time, for packager convenience.
>
> > Having pondered this some more, I lean toward the following conservative
> > fix.
> > Add to all supported branches a test case that triggers the crash and a
> > configure-time warning if the OpenLDAP version falls in the vulnerable
> > range.
> > So long as those who build from source monitor either "configure" output or
> > test suite failures, they'll have the opportunity to head off the problem.
>
> +1 for a configure warning, but I share your concern that it's likely to
> go unnoticed (sometimes I wish autoconf were not so chatty...).
> I'm not sure about the practicality of adding a test case --- how will we
> test that if no LDAP server is at hand?
Merely attempting an LDAP connection (to a closed port, for example)
initializes the library far enough to trigger the problem. Here's a patch
implementing the warning and test case. The test case is based on the one I
posted upthread, modified to work with installcheck, work with non-LDAP
builds, and close a race condition.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
diff --git a/configure b/configure
index 481096c..e2850e4 100755
--- a/configure
+++ b/configure
@@ -9464,6 +9464,17 @@ fi
fi
+# PGAC_LDAP_SAFE
+# --------------
+# PostgreSQL sometimes loads libldap_r and plain libldap into the same
+# process. Check for OpenLDAP versions known not to tolerate doing so; assume
+# non-OpenLDAP implementations are safe. The dblink test suite exercises the
+# hazardous interaction directly.
+
+
+
+
+
if test "$with_ldap" = yes ; then
if test "$PORTNAME" != "win32"; then
for ac_header in ldap.h
@@ -9480,6 +9491,47 @@ fi
done
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for compatible LDAP
implementation" >&5
+$as_echo_n "checking for compatible LDAP implementation... " >&6; }
+if ${pgac_cv_ldap_safe+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <ldap.h>
+#if !defined(LDAP_VENDOR_VERSION) || \
+ (defined(LDAP_API_FEATURE_X_OPENLDAP) && \
+ LDAP_VENDOR_VERSION >= 20424 && LDAP_VENDOR_VERSION <= 20431)
+choke me
+#endif
+int
+main ()
+{
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+ pgac_cv_ldap_safe=yes
+else
+ pgac_cv_ldap_safe=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_ldap_safe" >&5
+$as_echo "$pgac_cv_ldap_safe" >&6; }
+
+if test "$pgac_cv_ldap_safe" != yes; then
+ { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING:
+*** With OpenLDAP versions 2.4.24 through 2.4.31, inclusive, each backend
+*** process that loads libpq (via WAL receiver, dblink, or postgres_fdw) and
+*** also uses LDAP will crash on exit." >&5
+$as_echo "$as_me: WARNING:
+*** With OpenLDAP versions 2.4.24 through 2.4.31, inclusive, each backend
+*** process that loads libpq (via WAL receiver, dblink, or postgres_fdw) and
+*** also uses LDAP will crash on exit." >&2;}
+fi
else
for ac_header in winldap.h
do :
diff --git a/configure.in b/configure.in
index c938a5d..9f324f0 100644
--- a/configure.in
+++ b/configure.in
@@ -1096,10 +1096,39 @@ if test "$with_libxslt" = yes ; then
AC_CHECK_HEADER(libxslt/xslt.h, [], [AC_MSG_ERROR([header file
<libxslt/xslt.h> is required for XSLT support])])
fi
+# PGAC_LDAP_SAFE
+# --------------
+# PostgreSQL sometimes loads libldap_r and plain libldap into the same
+# process. Check for OpenLDAP versions known not to tolerate doing so; assume
+# non-OpenLDAP implementations are safe. The dblink test suite exercises the
+# hazardous interaction directly.
+
+AC_DEFUN([PGAC_LDAP_SAFE],
+[AC_CACHE_CHECK([for compatible LDAP implementation], [pgac_cv_ldap_safe],
+[AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
+[#include <ldap.h>
+#if !defined(LDAP_VENDOR_VERSION) || \
+ (defined(LDAP_API_FEATURE_X_OPENLDAP) && \
+ LDAP_VENDOR_VERSION >= 20424 && LDAP_VENDOR_VERSION <= 20431)
+choke me
+#endif], [])],
+[pgac_cv_ldap_safe=yes],
+[pgac_cv_ldap_safe=no])])
+
+if test "$pgac_cv_ldap_safe" != yes; then
+ AC_MSG_WARN([
+*** With OpenLDAP versions 2.4.24 through 2.4.31, inclusive, each backend
+*** process that loads libpq (via WAL receiver, dblink, or postgres_fdw) and
+*** also uses LDAP will crash on exit.])
+fi])
+
+
+
if test "$with_ldap" = yes ; then
if test "$PORTNAME" != "win32"; then
AC_CHECK_HEADERS(ldap.h, [],
[AC_MSG_ERROR([header file <ldap.h> is required for
LDAP])])
+ PGAC_LDAP_SAFE
else
AC_CHECK_HEADERS(winldap.h, [],
[AC_MSG_ERROR([header file <winldap.h> is required for
LDAP])],
diff --git a/contrib/dblink/Makefile b/contrib/dblink/Makefile
index 32314a0..09032f8 100644
--- a/contrib/dblink/Makefile
+++ b/contrib/dblink/Makefile
@@ -9,7 +9,9 @@ SHLIB_PREREQS = submake-libpq
EXTENSION = dblink
DATA = dblink--1.1.sql dblink--1.0--1.1.sql dblink--unpackaged--1.0.sql
-REGRESS = dblink
+REGRESS = paths dblink
+REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
+EXTRA_CLEAN = sql/paths.sql expected/paths.out
# the db name is hard-coded in the tests
override USE_MODULE_DB =
diff --git a/contrib/dblink/expected/.gitignore
b/contrib/dblink/expected/.gitignore
new file mode 100644
index 0000000..d9c7942
--- /dev/null
+++ b/contrib/dblink/expected/.gitignore
@@ -0,0 +1 @@
+/paths.out
diff --git a/contrib/dblink/expected/dblink.out
b/contrib/dblink/expected/dblink.out
index 36fdf73..eee31a2 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -103,6 +103,33 @@ SELECT *
FROM dblink('SELECT * FROM foo') AS t(a int, b text, c text[])
WHERE t.a > 7;
ERROR: connection not available
+-- The first-level connection's backend will crash on exit given OpenLDAP
+-- [2.4.24, 2.4.31]. We won't see evidence of any crash until the victim
+-- process terminates and the postmaster responds. If process termination
+-- entails writing a core dump, that can take awhile. Wait for the process to
+-- vanish. At that point, the postmaster has called waitpid() on the crashed
+-- process, and it will accept no new connections until it has reinitialized
+-- the cluster. (We can't exploit pg_stat_activity, because the crash happens
+-- after the backend updates shared memory to reflect its impending exit.)
+DO $pl$
+DECLARE
+ detail text;
+BEGIN
+ PERFORM wait_pid(crash_pid)
+ FROM dblink('dbname=contrib_regression', $$
+ SELECT pg_backend_pid() FROM dblink(
+ 'service=test_ldap dbname=contrib_regression',
+ -- This string concatenation is a hack to shoehorn a
+ -- set_pgservicefile call into the SQL statement.
+ 'SELECT 1' || set_pgservicefile('pg_service.conf')
+ ) t(c int)
+ $$) AS t(crash_pid int);
+EXCEPTION WHEN OTHERS THEN
+ GET STACKED DIAGNOSTICS detail = PG_EXCEPTION_DETAIL;
+ -- Expected error in a non-LDAP build.
+ IF NOT detail LIKE 'syntax error in service file%' THEN RAISE; END IF;
+END
+$pl$;
-- create a persistent connection
SELECT dblink_connect('dbname=contrib_regression');
dblink_connect
diff --git a/contrib/dblink/input/paths.source
b/contrib/dblink/input/paths.source
new file mode 100644
index 0000000..aab3a3b
--- /dev/null
+++ b/contrib/dblink/input/paths.source
@@ -0,0 +1,14 @@
+-- Initialization that requires path substitution.
+
+CREATE FUNCTION putenv(text)
+ RETURNS void
+ AS '@libdir@/regress@DLSUFFIX@', 'regress_putenv'
+ LANGUAGE C STRICT;
+
+CREATE FUNCTION wait_pid(int)
+ RETURNS void
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C STRICT;
+
+CREATE FUNCTION set_pgservicefile(text) RETURNS void LANGUAGE SQL
+ AS $$SELECT putenv('PGSERVICEFILE=@abs_srcdir@/' || $1)$$;
diff --git a/contrib/dblink/output/paths.source
b/contrib/dblink/output/paths.source
new file mode 100644
index 0000000..e1097f0
--- /dev/null
+++ b/contrib/dblink/output/paths.source
@@ -0,0 +1,11 @@
+-- Initialization that requires path substitution.
+CREATE FUNCTION putenv(text)
+ RETURNS void
+ AS '@libdir@/regress@DLSUFFIX@', 'regress_putenv'
+ LANGUAGE C STRICT;
+CREATE FUNCTION wait_pid(int)
+ RETURNS void
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C STRICT;
+CREATE FUNCTION set_pgservicefile(text) RETURNS void LANGUAGE SQL
+ AS $$SELECT putenv('PGSERVICEFILE=@abs_srcdir@/' || $1)$$;
diff --git a/contrib/dblink/pg_service.conf b/contrib/dblink/pg_service.conf
new file mode 100644
index 0000000..92201f0
--- /dev/null
+++ b/contrib/dblink/pg_service.conf
@@ -0,0 +1,7 @@
+# pg_service.conf for minimally exercising libpq use of LDAP.
+
+# Having failed to reach an LDAP server, libpq essentially ignores the
+# "service=test_ldap" in its connection string. Contact the "discard"
+# service; the test works whether or not it answers.
+[test_ldap]
+ldap://127.0.0.1:9/base?attribute?one?filter
diff --git a/contrib/dblink/sql/.gitignore b/contrib/dblink/sql/.gitignore
new file mode 100644
index 0000000..d175078
--- /dev/null
+++ b/contrib/dblink/sql/.gitignore
@@ -0,0 +1 @@
+/paths.sql
diff --git a/contrib/dblink/sql/dblink.sql b/contrib/dblink/sql/dblink.sql
index 30396ed..500c684 100644
--- a/contrib/dblink/sql/dblink.sql
+++ b/contrib/dblink/sql/dblink.sql
@@ -65,6 +65,34 @@ SELECT *
FROM dblink('SELECT * FROM foo') AS t(a int, b text, c text[])
WHERE t.a > 7;
+-- The first-level connection's backend will crash on exit given OpenLDAP
+-- [2.4.24, 2.4.31]. We won't see evidence of any crash until the victim
+-- process terminates and the postmaster responds. If process termination
+-- entails writing a core dump, that can take awhile. Wait for the process to
+-- vanish. At that point, the postmaster has called waitpid() on the crashed
+-- process, and it will accept no new connections until it has reinitialized
+-- the cluster. (We can't exploit pg_stat_activity, because the crash happens
+-- after the backend updates shared memory to reflect its impending exit.)
+DO $pl$
+DECLARE
+ detail text;
+BEGIN
+ PERFORM wait_pid(crash_pid)
+ FROM dblink('dbname=contrib_regression', $$
+ SELECT pg_backend_pid() FROM dblink(
+ 'service=test_ldap dbname=contrib_regression',
+ -- This string concatenation is a hack to shoehorn a
+ -- set_pgservicefile call into the SQL statement.
+ 'SELECT 1' || set_pgservicefile('pg_service.conf')
+ ) t(c int)
+ $$) AS t(crash_pid int);
+EXCEPTION WHEN OTHERS THEN
+ GET STACKED DIAGNOSTICS detail = PG_EXCEPTION_DETAIL;
+ -- Expected error in a non-LDAP build.
+ IF NOT detail LIKE 'syntax error in service file%' THEN RAISE; END IF;
+END
+$pl$;
+
-- create a persistent connection
SELECT dblink_connect('dbname=contrib_regression');
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index c146109..09e027c 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -6,6 +6,7 @@
#include <float.h>
#include <math.h>
+#include <signal.h>
#include "access/htup_details.h"
#include "access/transam.h"
@@ -16,6 +17,7 @@
#include "commands/trigger.h"
#include "executor/executor.h"
#include "executor/spi.h"
+#include "miscadmin.h"
#include "utils/builtins.h"
#include "utils/geo_decls.h"
#include "utils/rel.h"
@@ -822,3 +824,44 @@ make_tuple_indirect(PG_FUNCTION_ARGS)
*/
PG_RETURN_POINTER(newtup->t_data);
}
+
+PG_FUNCTION_INFO_V1(regress_putenv);
+
+Datum
+regress_putenv(PG_FUNCTION_ARGS)
+{
+ MemoryContext oldcontext;
+ char *envbuf;
+
+ if (!superuser())
+ elog(ERROR, "must be superuser to change environment
variables");
+
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+ envbuf = text_to_cstring((text *) PG_GETARG_POINTER(0));
+ MemoryContextSwitchTo(oldcontext);
+
+ if (putenv(envbuf) != 0)
+ elog(ERROR, "could not set environment variable: %m");
+
+ PG_RETURN_VOID();
+}
+
+/* Sleep until no process has a given PID. */
+PG_FUNCTION_INFO_V1(wait_pid);
+
+Datum
+wait_pid(PG_FUNCTION_ARGS)
+{
+ int pid = PG_GETARG_INT32(0);
+
+ if (!superuser())
+ elog(ERROR, "must be superuser to check PID liveness");
+
+ while (kill(pid, 0) == 0)
+ pg_usleep(50000);
+
+ if (errno != ESRCH)
+ elog(ERROR, "could not check PID %d liveness: %m", pid);
+
+ PG_RETURN_VOID();
+}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
