From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrew Gierth <andrew@tao11.riddles.org.uk>
Cc: Jeremy Evans <code@jeremyevans.net>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15367: Crash in pg_fe_scram_free when using foreign tables
Date: Sat, 08 Sep 2018 23:47:54 -0400
Message-ID: <17262.1536464874@sss.pgh.pa.us> (raw)
In-Reply-To: <3989.1536457182@sss.pgh.pa.us>
References: <153626613985.23143.4743626885618266803@wrigleys.postgresql.org>
<20180906215836.GP2726@paquier.xyz>
<20180906222843.GF17425@jeremyevans.local>
<20180907165103.GH17425@jeremyevans.local>
<20180907175518.GI17425@jeremyevans.local>
<20180907181522.GB32058@paquier.xyz>
<20016.1536351535@sss.pgh.pa.us>
<20180907204403.GC32058@paquier.xyz>
<16558.1536407783@sss.pgh.pa.us>
<18398.1536410055@sss.pgh.pa.us>
<87o9d8rv08.fsf@news-spur.riddles.org.uk>
<3979.1536433320@sss.pgh.pa.us>
<6869.1536436921@sss.pgh.pa.us>
<874lezskcu.fsf@news-spur.riddles.org.uk>
<31648.1536451185@sss.pgh.pa.us>
<3989.1536457182@sss.pgh.pa.us>
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
Attachments:detect-bogus-ELF-linkage.patch (text/x-diff, 7.2K) downloaddiff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.cindex 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/Makefileindex 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.cnew file mode 100644index 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.hnew file mode 100644index 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/.gitignoreindex 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/Makefileindex 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.c pg_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.c pgsleep.c pg_strong_random.c pgstrcasecmp.c pqsignal.c snprintf.c strerror.c strlcpy.c strnlen.c thread.c win32error.c win32setlocale.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.cindex 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.pmindex 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);
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, andrew@tao11.riddles.org.uk, code@jeremyevans.net, michael@paquier.xyz, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15367: Crash in pg_fe_scram_free when using foreign tables
In-Reply-To: <17262.1536464874@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox