Re: BUG #15367: Crash in pg_fe_scram_free when using foreign tables - Mailing list pgsql-bugs

I wrote:
> On the other front of developing a test case to detect this problem,
> I did not have much luck with mechanizing this specific test: it
> requires some functionality we have in the TAP tests, like setting
> up an installation with SCRAM password authentication enabled, as
> well as other functionality we have in the isolation tester, like
> doing things in two different sessions concurrently.  But we don't
> really need to test this *exact* scenario; we just need something
> that will behave differently if libpq links to backend versions of
> any of the problematic functions.  I'm a bit tempted to add
> something like this to saslprep.c:

> bool
> saslprep_is_frontend()
> {
> #ifdef FRONTEND
>     return true;
> #else
>     return false;
> #endif
> }

On reflection, saslprep.c is not the right place for this: if some
other shlib needs a similar test, it might be forced to import SASL
support it has no need for.  So I put the test function into its
own new .c file, which increases the footprint of the patch a bit
but seems much less ad-hoc.

I verified that this patch causes a failure in the postgres_fdw
regression test on affected platforms, even my FreeBSD box where
the original bug doesn't cause an easily-detectable failure.

I propose this for HEAD only; the main point is just to detect
which platforms have the issue, and HEAD seems like enough for that.

            regards, tom lane

diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index cdd71a9..578af2e 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -24,6 +24,7 @@
 #include "catalog/index.h"
 #include "catalog/pg_collation.h"
 #include "catalog/pg_type.h"
+#include "common/link-canary.h"
 #include "libpq/pqsignal.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
@@ -503,6 +504,13 @@ BootstrapModeMain(void)
     Assert(IsBootstrapProcessingMode());

     /*
+     * To ensure that src/common/link-canary.c is linked into the backend, we
+     * must call it from somewhere.  Here is as good as anywhere.
+     */
+    if (pg_link_canary_is_frontend())
+        elog(ERROR, "backend is incorrectly linked to frontend functions");
+
+    /*
      * Do backend-like initialization for bootstrap mode
      */
     InitProcess();
diff --git a/src/common/Makefile b/src/common/Makefile
index 1fc2c66..6871747 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -41,7 +41,8 @@ override CPPFLAGS := -DFRONTEND $(CPPFLAGS)
 LIBS += $(PTHREAD_LIBS)

 OBJS_COMMON = base64.o config_info.o controldata_utils.o exec.o file_perm.o \
-    ip.o keywords.o md5.o pg_lzcompress.o pgfnames.o psprintf.o relpath.o \
+    ip.o keywords.o link-canary.o md5.o pg_lzcompress.o \
+    pgfnames.o psprintf.o relpath.o \
     rmtree.o saslprep.o scram-common.o string.o unicode_norm.o \
     username.o wait_error.o

diff --git a/src/common/link-canary.c b/src/common/link-canary.c
new file mode 100644
index 0000000..5b4e562
--- /dev/null
+++ b/src/common/link-canary.c
@@ -0,0 +1,36 @@
+/*-------------------------------------------------------------------------
+ * link-canary.c
+ *      Detect whether src/common functions came from frontend or backend.
+ *
+ * Copyright (c) 2018, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *      src/common/link-canary.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "c.h"
+
+#include "common/link-canary.h"
+
+/*
+ * This function just reports whether this file was compiled for frontend
+ * or backend environment.  We need this because in some systems, mainly
+ * ELF-based platforms, it is possible for a shlib (such as libpq) loaded
+ * into the backend to call a backend function named XYZ in preference to
+ * the shlib's own function XYZ.  That's bad if the two functions don't
+ * act identically.  This exact situation comes up for many functions in
+ * src/common and src/port, where the same function names exist in both
+ * libpq and the backend but they don't act quite identically.  To verify
+ * that appropriate measures have been taken to prevent incorrect symbol
+ * resolution, libpq should test that this function returns true.
+ */
+bool
+pg_link_canary_is_frontend(void)
+{
+#ifdef FRONTEND
+    return true;
+#else
+    return false;
+#endif
+}
diff --git a/src/include/common/link-canary.h b/src/include/common/link-canary.h
new file mode 100644
index 0000000..917faae
--- /dev/null
+++ b/src/include/common/link-canary.h
@@ -0,0 +1,17 @@
+/*-------------------------------------------------------------------------
+ *
+ * link-canary.h
+ *      Detect whether src/common functions came from frontend or backend.
+ *
+ * Copyright (c) 2018, PostgreSQL Global Development Group
+ *
+ * src/include/common/link-canary.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef LINK_CANARY_H
+#define LINK_CANARY_H
+
+extern bool pg_link_canary_is_frontend(void);
+
+#endif                            /* LINK_CANARY_H */
diff --git a/src/interfaces/libpq/.gitignore b/src/interfaces/libpq/.gitignore
index 5c232ae..ce1576e 100644
--- a/src/interfaces/libpq/.gitignore
+++ b/src/interfaces/libpq/.gitignore
@@ -25,6 +25,7 @@
 /ip.c
 /md5.c
 /base64.c
+/link-canary.c
 /scram-common.c
 /sha2.c
 /sha2_openssl.c
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index abe0a50..ec0122a 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -49,7 +49,7 @@ endif
 # src/backend/utils/mb
 OBJS += encnames.o wchar.o
 # src/common
-OBJS += base64.o ip.o md5.o scram-common.o saslprep.o unicode_norm.o
+OBJS += base64.o ip.o link-canary.o md5.o scram-common.o saslprep.o unicode_norm.o

 ifeq ($(with_openssl),yes)
 OBJS += fe-secure-openssl.o fe-secure-common.o sha2_openssl.o
@@ -106,7 +106,7 @@ backend_src = $(top_srcdir)/src/backend
 chklocale.c crypt.c erand48.c getaddrinfo.c getpeereid.c inet_aton.c inet_net_ntop.c noblock.c open.c system.c
pgsleep.cpg_strong_random.c pgstrcasecmp.c pqsignal.c snprintf.c strerror.c strlcpy.c strnlen.c thread.c win32error.c
win32setlocale.c:% : $(top_srcdir)/src/port/% 
     rm -f $@ && $(LN_S) $< .

-ip.c md5.c base64.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c: % : $(top_srcdir)/src/common/%
+ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c: % :
$(top_srcdir)/src/common/%
     rm -f $@ && $(LN_S) $< .

 encnames.c wchar.c: % : $(backend_src)/utils/mb/%
@@ -156,7 +156,7 @@ clean distclean: clean-lib
     rm -f pg_config_paths.h
 # Remove files we (may have) symlinked in from src/port and other places
     rm -f chklocale.c crypt.c erand48.c getaddrinfo.c getpeereid.c inet_aton.c inet_net_ntop.c noblock.c open.c
system.cpgsleep.c pg_strong_random.c pgstrcasecmp.c pqsignal.c snprintf.c strerror.c strlcpy.c strnlen.c thread.c
win32error.cwin32setlocale.c 
-    rm -f ip.c md5.c base64.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c
+    rm -f ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c
     rm -f encnames.c wchar.c

 maintainer-clean: distclean maintainer-clean-lib
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index db5aacd..42cdb97 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -71,6 +71,7 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
 #endif

 #include "common/ip.h"
+#include "common/link-canary.h"
 #include "common/scram-common.h"
 #include "mb/pg_wchar.h"
 #include "port/pg_bswap.h"
@@ -1748,6 +1749,19 @@ connectDBStart(PGconn *conn)
     if (!conn->options_valid)
         goto connect_errReturn;

+    /*
+     * Check for bad linking to backend-internal versions of src/common
+     * functions (see comments in link-canary.c for the reason we need this).
+     * Nobody but developers should see this message, so we don't bother
+     * translating it.
+     */
+    if (!pg_link_canary_is_frontend())
+    {
+        printfPQExpBuffer(&conn->errorMessage,
+                          "libpq is incorrectly linked to backend functions\n");
+        goto connect_errReturn;
+    }
+
     /* Ensure our buffers are empty */
     conn->inStart = conn->inCursor = conn->inEnd = 0;
     conn->outCount = 0;
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 14d2a3c..9ce03ce 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -116,7 +116,8 @@ sub mkvcbuild

     our @pgcommonallfiles = qw(
       base64.c config_info.c controldata_utils.c exec.c file_perm.c ip.c
-      keywords.c md5.c pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c
+      keywords.c link-canary.c md5.c
+      pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c
       saslprep.c scram-common.c string.c unicode_norm.c username.c
       wait_error.c);