[PATCH] Detect escape of ErrorContextCallback stack pointers (and from PG_TRY() )

From: Craig Ringer <craig(at)2ndquadrant(dot)com>
To: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Cc: Álvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Subject: [PATCH] Detect escape of ErrorContextCallback stack pointers (and from PG_TRY() )
Date: 2020-09-03 11:21:59
Message-ID: CAMsr+YE8tveiaGPuNzw+5fuo0yeZf7LePVLpx9MxUrFHMBG0gQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi folks

The attached patch series adds support for detecting coding errors where a
stack-allocated ErrorContextCallback is not popped from the
error_context_stack before the variable leaves scope.

With the attached patches this code will emit a backtrace and abort() on
cassert builds at the "return" statement where it leaks the stack pointer:

ErrorContextCallback cb pg_errcontext_check();
...
cb.previous = error_context_stack;
error_context_stack = &cb;
...
if (something)
return; /* whoops! */
...
error_context_stack = error_context_stack->previous;

as I discuss on this old blog article here:
https://www.2ndquadrant.com/en/blog/dev-corner-error-context-stack-corruption/

Note the pgl_errcontext_check() in the above, which enables the check.

The infrastructure added by the patch could well be useful for other
similar validation steps. In fact, I've added a similar validator that
detects escapes from PG_TRY() blocks, so you get a neat assertion failure
at the point of escape instead of a crash at a __longjmp() with a
nonsensical stack. Example using a deliberately introduced mistake in
postgres_fdw as a test:

Without the PG_TRY() patch:

#0 __longjmp () at ../sysdeps/x86_64/__longjmp.S:111
#1 0xc35a5d9e1a902cfc in ?? ()
Backtrace stopped: Cannot access memory at address 0xc0ea5d9e1a902cfc

With the PG_TRY() patch (abbreviated):

#3 0x0000000000abbd4b in pg_try_check_return_guard
#4 0x00007f8e2e20e41a in fetch_more_data
#5 0x00007f8e2e20a80a in postgresIterateForeignScan
...

Note that valgrind's memcheck, gcc's -fstack-protector, etc are all quite
bad at detecting this class of error, so being able to do so automatically
should be nice. I've had an interesting time chasing down error context
stack mistakes a few times in the past

We unfortunately cannot use it for RAII-style coding because the underlying
__attribute__((callback(..))) is not available on MSVC. As usual. But it
remains useful for checking and assertions.

The patch is supplied as a small series:

(1) Add some documentation on error context callback usage
(2) Add pg_errcontext_check() to detect ErrorContextCallback escapes
(3) Enable pg_errcontext_check() for all in-tree users
(4) Document that generally PG_CATCH() should PG_RE_THROW()
(5) Assert() if returning from inside a PG_TRY() / PG_CATCH() block
(6) [RFC] Add a more succinct API for error context stack callbacks
(7) [RFC] Use __has_attribute for compiler attribute detection where
possible

I also added a patch (6) on top as a RFC of sorts, which adds a more
succinct API for error context setup and teardown. It's not required for
these changes, but it might make sense to adopt it at the same time if
we're changing usage across the tree anyway.

A further patch (7) switches over to using __has_attribute instead of
explicit compiler version checks. Again, optional, more of a RFC.

--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise

Attachment Content-Type Size
0001-Add-some-documentation-on-error-context-callback-usa.patch text/x-patch 2.6 KB
0002-Add-pg_errcontext_check-to-detect-ErrorContextCallba.patch text/x-patch 5.5 KB
0004-Document-that-generally-PG_CATCH-should-PG_RE_THROW.patch text/x-patch 1.6 KB
0005-Assert-if-returning-from-inside-a-PG_TRY-PG_CATCH-bl.patch text/x-patch 2.2 KB
0003-Enable-pg_errcontext_check-for-all-in-tree-users.patch text/x-patch 19.4 KB
0006-Add-a-more-succinct-API-for-error-context-stack-call.patch text/x-patch 2.6 KB
0007-Use-__has_attribute-for-compiler-attribute-detection.patch text/x-patch 8.3 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Borisov 2020-09-03 11:25:52 Re: PoC: custom signal handler for extensions
Previous Message Daniel Gustafsson 2020-09-03 10:47:22 Re: Switch to multi-inserts for pg_depend