Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fyqhk-0006hB-SQ for pgsql-bugs@arkaria.postgresql.org; Sun, 09 Sep 2018 03:48:09 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fyqhg-00089s-QQ for pgsql-bugs@arkaria.postgresql.org; Sun, 09 Sep 2018 03:48:04 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fyqhg-00089l-Jz for pgsql-bugs@lists.postgresql.org; Sun, 09 Sep 2018 03:48:04 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fyqhc-0002cW-0o for pgsql-bugs@lists.postgresql.org; Sun, 09 Sep 2018 03:48:03 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id w893lsZ8017263; Sat, 8 Sep 2018 23:47:55 -0400 From: Tom Lane To: Andrew Gierth cc: Jeremy Evans , Michael Paquier , pgsql-bugs@lists.postgresql.org Subject: Re: BUG #15367: Crash in pg_fe_scram_free when using foreign tables 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> Comments: In-reply-to Tom Lane message dated "Sat, 08 Sep 2018 21:39:42 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <17173.1536464755.0@sss.pgh.pa.us> Date: Sat, 08 Sep 2018 23:47:54 -0400 Message-ID: <17262.1536464874@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <17173.1536464755.1@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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="detect-bogus-ELF-linkage.patch"; charset="us-ascii" Content-ID: <17173.1536464755.2@sss.pgh.pa.us> Content-Description: detect-bogus-ELF-linkage.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/boo= tstrap.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, w= e + * 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 :=3D -DFRONTEND $(CPPFLAGS) LIBS +=3D $(PTHREAD_LIBS) = OBJS_COMMON =3D base64.o config_info.o controldata_utils.o exec.o file_pe= rm.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-ca= nary.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/.gitig= nore 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 +=3D encnames.o wchar.o # src/common -OBJS +=3D base64.o ip.o md5.o scram-common.o saslprep.o unicode_norm.o +OBJS +=3D base64.o ip.o link-canary.o md5.o scram-common.o saslprep.o uni= code_norm.o = ifeq ($(with_openssl),yes) OBJS +=3D fe-secure-openssl.o fe-secure-common.o sha2_openssl.o @@ -106,7 +106,7 @@ backend_src =3D $(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 pgstrca= secmp.c pqsignal.c snprintf.c strerror.c strlcpy.c strnlen.c thread.c win3= 2error.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 unico= de_norm.c: % : $(top_srcdir)/src/common/% +ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c sa= slprep.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_opens= sl.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-c= onnect.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, PQconninf= oOption *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 =3D conn->inCursor =3D conn->inEnd =3D 0; conn->outCount =3D 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 =3D 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 rmtre= e.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); = ------- =_aaaaaaaaaa0--