From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
cc: Jacob Champion <pchampion@vmware.com>,
        "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
        Daniel Gustafsson <daniel@yesql.se>
Subject: Preventing abort() and exit() calls in libpq
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3128874.1624742957.0@sss.pgh.pa.us>
Date: Sat, 26 Jun 2021 17:29:29 -0400
Message-ID: <3128896.1624742969@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3128896.1624742969%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3128874.1624742957.1@sss.pgh.pa.us>

[ starting a new thread so as not to confuse the cfbot ]

I wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> Good point.  That's worse than just pfree() which is just a plain call
>> to free() in the frontend.  We could have more policies here, but my
>> take is that we'd better move fe_memutils.o to OBJS_FRONTEND in
>> src/common/Makefile so as shared libraries don't use those routines in
>> the long term.

> Ugh.  Not only is that bad, but your proposed fix doesn't fix it.
> At least in psql, and probably in most/all of our other clients,
> removing fe_memutils.o from libpq's link just causes it to start
> relying on the copy in the psql executable :-(.  So I agree that
> some sort of mechanical enforcement would be a really good thing,
> but I'm not sure what it would look like.

After some thought I propose that what we really want is to prevent
any calls of abort() or exit() from inside libpq.  Attached is a
draft patch to do that.  This can't be committed as-is, because
we still have some abort() calls in there in HEAD, but if we could
get that cleaned up it'd work.  Alternatively we could just disallow
exit(), which'd be enough to catch the problematic src/common files.

This relies on "nm" being able to work on shlibs, which it's not
required to by POSIX.  However, it seems to behave as desired even
on my oldest dinosaurs.  In any case, if "nm" doesn't work then
we'll just not detect such problems on that platform, which should
be OK as long as the test does work on common platforms.
Other than that point I think it's relying only on POSIX-spec
features.

I'll stick this into the CF list to see if the cfbot agrees that
it finds the abort() problems...

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="disallow-abort-and-exit-in-libpq-1.patch";
	charset="us-ascii"
Content-ID: <3128874.1624742957.2@sss.pgh.pa.us>
Content-Description: disallow-abort-and-exit-in-libpq-1.patch

diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index 0c4e55b6ad..3d992fdc78 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -96,12 +96,18 @@ SHLIB_EXPORTS = exports.txt
 
 PKG_CONFIG_REQUIRES_PRIVATE = libssl libcrypto
 
-all: all-lib
+all: all-lib check-libpq-refs
 
 # Shared library stuff
 include $(top_srcdir)/src/Makefile.shlib
 backend_src = $(top_srcdir)/src/backend
 
+# Check for functions that libpq must not call.
+# (If nm doesn't exist or doesn't work on shlibs, this test will silently
+# do nothing, which is fine.)
+.PHONY: check-libpq-refs
+check-libpq-refs: $(shlib)
+	@! nm -A -g -u $< 2>/dev/null | grep -e abort -e exit
 
 # Make dependencies on pg_config_paths.h visible in all builds.
 fe-connect.o: fe-connect.c $(top_builddir)/src/port/pg_config_paths.h

------- =_aaaaaaaaaa0--