possible self-deadlock window after bad ProcessStartupPacket

On Thu, Feb 2, 2017 at 3:18 PM, Jimmy Yih <jyih@pivotal.io> wrote:

In that pull request, we fix the issue by checking for proc_exit_inprogress.
Is there a reason why startup_die should not check for proc_exit_inprogress?

startup_die() is just calling proc_exit(), so it seems like it might
be better to fix it by putting the check into proc_exit itself.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-02-02 12:18:22 -0800, Jimmy Yih wrote:

In the above pull request, Heikki also mentions that a similar scenario can
happen during palloc() as well... which is similar to what we saw in
Greenplum a couple years back for a deadlock in a malloc() call where we
responded by changing exit() to _exit() in quickdie as a fix. That could
possibly be applicable to latest Postgres as well.

Isn't the quickdie() issue that we palloc/malloc in the first place,
rather than the exit call? There's some risk for exit() too, but we
reset our own atexit handlers before exiting, so the risk seems fairly
small.

In my opinion the fix here would be to do it right and never emit error
messages from signal handlers via elog.c - we've progressed a lot
towards the goal and do a lot less in signal handlers these days - but
quickdie() is one glaring exception to that. I think a reasonable fix
here would be to use write() of a statically allocated message, rather
then elog proper, and not to send the message to the client. Libpq, and
I presume other clients, synthethize a message upon unexpected socket
closure anyway, and it's completely unclear whether we can send a
message anyway.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-06-22 10:41:41 -0700, Andres Freund wrote:

On 2017-02-02 12:18:22 -0800, Jimmy Yih wrote:

In the above pull request, Heikki also mentions that a similar scenario can
happen during palloc() as well... which is similar to what we saw in
Greenplum a couple years back for a deadlock in a malloc() call where we
responded by changing exit() to _exit() in quickdie as a fix. That could
possibly be applicable to latest Postgres as well.

Isn't the quickdie() issue that we palloc/malloc in the first place,
rather than the exit call? There's some risk for exit() too, but we
reset our own atexit handlers before exiting, so the risk seems fairly
small.

In my opinion the fix here would be to do it right and never emit error
messages from signal handlers via elog.c - we've progressed a lot
towards the goal and do a lot less in signal handlers these days - but
quickdie() is one glaring exception to that. I think a reasonable fix
here would be to use write() of a statically allocated message, rather
then elog proper, and not to send the message to the client. Libpq, and
I presume other clients, synthethize a message upon unexpected socket
closure anyway, and it's completely unclear whether we can send a
message anyway.

Or, probably more robust: Simply _exit(2) without further ado, and rely
on postmaster to output an appropriate error message. Arguably it's not
actually useful to see hundreds of "WARNING: terminating connection because of
crash of another server process" messages in the log anyway.

- Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@anarazel.de> writes:

Or, probably more robust: Simply _exit(2) without further ado, and rely
on postmaster to output an appropriate error message. Arguably it's not
actually useful to see hundreds of "WARNING: terminating connection because of
crash of another server process" messages in the log anyway.

At that point you might as well skip the entire mechanism and go straight
to SIGKILL. IMO the only reason quickdie() exists at all is to try to
send a helpful message to the client. And it is helpful --- your attempt
to claim that it isn't sounds very much like wishful thinking.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jun 22, 2017 at 10:50 AM, Andres Freund <andres@anarazel.de> wrote:

Or, probably more robust: Simply _exit(2) without further ado, and rely
on postmaster to output an appropriate error message. Arguably it's not
actually useful to see hundreds of "WARNING: terminating connection because of
crash of another server process" messages in the log anyway.

To support using _exit(2) in *quickdie() handlers, I would like to
share another stack trace indicating self-deadlock. In this case, WAL
writer process got SIGQUIT while it was already handling a SIGQUIT,
leading to self-deadlock.

#0 __lll_lock_wait_private () at
../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:95
#1 0x00007f0bf04db2bd in _int_free (av=0x7f0bf081fb20 <main_arena>,
p=0x1557e60, have_lock=0) at malloc.c:3962
#2 0x00007f0bf04df53c in __GI___libc_free (mem=mem@entry=0x1557e70)
at malloc.c:2968
#3 0x00007f0bf0495025 in __run_exit_handlers (status=2,
listp=0x7f0bf081f5f8 <__exit_funcs>,
run_list_atexit=run_list_atexit@entry=true)
at exit.c:91
#4 0x00007f0bf0495045 in __GI_exit (status=<optimized out>) at exit.c:104
#5 0x0000000000843994 in wal_quickdie ()
#6 <signal handler called>
#7 0x00007f0bf04db014 in _int_free (av=0x7f0bf081fb20 <main_arena>,
p=<optimized out>, have_lock=0) at malloc.c:4014
#8 0x00007f0bf04df53c in __GI___libc_free (mem=<optimized out>) at
malloc.c:2968
#9 0x00007f0bebf8b2ba in ?? () from /usr/lib/x86_64-linux-gnu/libtasn1.so.6
#10 0x00007f0bebf8c4ba in asn1_delete_structure2 () from
/usr/lib/x86_64-linux-gnu/libtasn1.so.6
#11 0x00007f0beec24738 in ?? () from /usr/lib/x86_64-linux-gnu/libgnutls.so.30
#12 0x00007f0bf3bb6de7 in _dl_fini () at dl-fini.c:235
#13 0x00007f0bf0494ff8 in __run_exit_handlers (status=2,
listp=0x7f0bf081f5f8 <__exit_funcs>,
run_list_atexit=run_list_atexit@entry=true)
at exit.c:82
#14 0x00007f0bf0495045 in __GI_exit (status=<optimized out>) at exit.c:104
#15 0x0000000000843994 in wal_quickdie ()
#16 <signal handler called>
#17 0x00007f0bf05585b3 in __select_nocancel () at
../sysdeps/unix/syscall-template.S:84
#18 0x0000000000b7c5da in pg_usleep ()
#19 0x0000000000843c4a in WalWriterMain ()
#20 0x000000000059ac47 in AuxiliaryProcessMain ()

On 19/07/18 03:26, Asim R P wrote:

On Thu, Jun 22, 2017 at 10:50 AM, Andres Freund <andres@anarazel.de> wrote:

Or, probably more robust: Simply _exit(2) without further ado, and rely
on postmaster to output an appropriate error message. Arguably it's not
actually useful to see hundreds of "WARNING: terminating connection because of
crash of another server process" messages in the log anyway.

To support using _exit(2) in *quickdie() handlers, I would like to
share another stack trace indicating self-deadlock. In this case, WAL
writer process got SIGQUIT while it was already handling a SIGQUIT,
leading to self-deadlock.

Ugh. Yeah, in wal_quickdie, and other aux process *_quickdie() handlers,
I agree we should just _exit(2). All we want to do is to exit the
process immediately.

bgworker_quickdie() makes some effort to block SIGQUIT during the exit()
processing, but that doesn't solve the whole problem. The process
could've been in the middle of a malloc/free when the signal arrived,
for example. exit() is simply not safe to call from a signal handler.

The regular backend's quickdie() function is more tricky. It should also
call _exit(2) rather than exit(2). But it also tries to ereport a
WARNING, and that is quite useful. ereport() is generally not safe in a
signal handler. There is some protection for specific failures: see
socket_putmessage(), for example, which has a mechanism to prevent a
message from being sent to the client in the middle of another message.
But at the end of the day, ereport() is complicated enough that it will
surely do a lot of unsafe things.

I don't have any great ideas on how to make the ereport(WARNING) safer,
but I agree we should at least change all the exit(2) calls in quickdie
handlers to _exit(2).

BTW, if postmaster is still alive, it will send a SIGKILL to any child
process that doesn't terminate in 5 seconds. So a deadlock in SIGQUIT
handler isn't that bad. Some other nasty things could happen, however;
all bets are off if you call unsafe functions from a signal handler.

- Heikki

Hi,

On 2018-07-19 11:57:25 +0300, Heikki Linnakangas wrote:

On 19/07/18 03:26, Asim R P wrote:

On Thu, Jun 22, 2017 at 10:50 AM, Andres Freund <andres@anarazel.de> wrote:

Or, probably more robust: Simply _exit(2) without further ado, and rely
on postmaster to output an appropriate error message. Arguably it's not
actually useful to see hundreds of "WARNING: terminating connection because of
crash of another server process" messages in the log anyway.

To support using _exit(2) in *quickdie() handlers, I would like to
share another stack trace indicating self-deadlock. In this case, WAL
writer process got SIGQUIT while it was already handling a SIGQUIT,
leading to self-deadlock.

Ugh. Yeah, in wal_quickdie, and other aux process *_quickdie() handlers, I
agree we should just _exit(2). All we want to do is to exit the process
immediately.

Indeed.

bgworker_quickdie() makes some effort to block SIGQUIT during the exit()
processing, but that doesn't solve the whole problem. The process could've
been in the middle of a malloc/free when the signal arrived, for example.
exit() is simply not safe to call from a signal handler.

Yea. I really don't understand why we have't been able to agree on this
for *years* now.

The regular backend's quickdie() function is more tricky. It should also
call _exit(2) rather than exit(2). But it also tries to ereport a WARNING,
and that is quite useful.

Is that actually true? Clients like libpq create the same error message
(which has its own issues, because it'll sometimes mis-interpret
things). The message doesn't actually have useful content, no?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-07-19 11:57:25 +0300, Heikki Linnakangas wrote:

The regular backend's quickdie() function is more tricky. It should also
call _exit(2) rather than exit(2). But it also tries to ereport a WARNING,
and that is quite useful.

There's already an on_exit_reset in there; why do we need more than that?

Is that actually true? Clients like libpq create the same error message
(which has its own issues, because it'll sometimes mis-interpret
things). The message doesn't actually have useful content, no?

Yes, it does: it lets users tell the difference between exit due to a
SIGQUIT and a crash of their own backend.

Admittedly, if we crash trying to send the message, then we're not
better off. But since that happens only very rarely, I do not think
it's a reasonable tradeoff to never send it at all.

regards, tom lane

On Thu, Jul 19, 2018 at 12:20:53PM -0700, Andres Freund wrote:

On 2018-07-19 11:57:25 +0300, Heikki Linnakangas wrote:

Ugh. Yeah, in wal_quickdie, and other aux process *_quickdie() handlers, I
agree we should just _exit(2). All we want to do is to exit the process
immediately.

Indeed.

bgworker_quickdie() makes some effort to block SIGQUIT during the exit()
processing, but that doesn't solve the whole problem. The process could've
been in the middle of a malloc/free when the signal arrived, for example.
exit() is simply not safe to call from a signal handler.

Yea. I really don't understand why we have't been able to agree on this
for *years* now.

I mean, only calling async-signal-safe functions from signal handlers is
a critical POSIX requirement, and exit(3) is NOT async-signal-safe.

The regular backend's quickdie() function is more tricky. It should also
call _exit(2) rather than exit(2). But it also tries to ereport a WARNING,
and that is quite useful.

Is that actually true? Clients like libpq create the same error message
(which has its own issues, because it'll sometimes mis-interpret
things). The message doesn't actually have useful content, no?

Is ereport() async-signal-safe? No. It could be, but it uses stdio to
write to stderr/console, and stdio is not async-signal-safe.

Nico
--

On Thu, Jul 19, 2018 at 03:49:35PM -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-07-19 11:57:25 +0300, Heikki Linnakangas wrote:

The regular backend's quickdie() function is more tricky. It should also
call _exit(2) rather than exit(2). But it also tries to ereport a WARNING,
and that is quite useful.

There's already an on_exit_reset in there; why do we need more than that?

You're not allowed to call exit(3) in signal handlers. exit(3) is not
async-signal-safe (for example, it calls atexit handlers, flushes stdio,
and who knows what else).

Is that actually true? Clients like libpq create the same error message
(which has its own issues, because it'll sometimes mis-interpret
things). The message doesn't actually have useful content, no?

Yes, it does: it lets users tell the difference between exit due to a
SIGQUIT and a crash of their own backend.

ereport() calls (via errstart() and write_stderr()) vfprintf() and
fflush(stderr). That's absolutely not async-signal-safe. The WIN32
code in write_stderr() does use vsnprintf() instead, and that could get
written to stderr with write(2), which is async-signal-safe.

Admittedly, if we crash trying to send the message, then we're not
better off. But since that happens only very rarely, I do not think
it's a reasonable tradeoff to never send it at all.

If sending it means using OpenSSL or what have you, that's probably even
more async-signal-safe code.

Now, ereport() could all be made safe enough for use in signal handlers,
but it isn't at this time.

What I'd do is have a volatile sig_atomic_t in_signal_handler_context
variable to indicate that we're dying, and then when that is non-zero,
ereport() and friends could use all-async-signal-safe codepaths.

Nico
--

Nico Williams <nico@cryptonector.com> writes:

What I'd do is have a volatile sig_atomic_t in_signal_handler_context
variable to indicate that we're dying, and then when that is non-zero,
ereport() and friends could use all-async-signal-safe codepaths.

I eagerly await your patch with an async-safe implementation of ereport...

regards, tom lane

On Thu, Jun 22, 2017 at 03:10:31PM -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Or, probably more robust: Simply _exit(2) without further ado, and rely
on postmaster to output an appropriate error message. Arguably it's not
actually useful to see hundreds of "WARNING: terminating connection because of
crash of another server process" messages in the log anyway.

At that point you might as well skip the entire mechanism and go straight
to SIGKILL. IMO the only reason quickdie() exists at all is to try to
send a helpful message to the client. And it is helpful --- your attempt
to claim that it isn't sounds very much like wishful thinking.

I dunno if it is or isn't helpful. But I do know that this must be done
in an async-signal-safe way.

Besides making ereport() async-signal-safe, which is tricky, you could
write(2) the arguments to a pipe that another thread in the same process
is reading from and which will then call ereport() and exit(3). This
would be less work if you're willing to use a thread for that (the
thread would only block in read(2) on that pipe, and would only provide
this one service).

Nico
--

On Thu, Jul 19, 2018 at 04:04:01PM -0400, Tom Lane wrote:

Nico Williams <nico@cryptonector.com> writes:

What I'd do is have a volatile sig_atomic_t in_signal_handler_context
variable to indicate that we're dying, and then when that is non-zero,
ereport() and friends could use all-async-signal-safe codepaths.

I eagerly await your patch with an async-safe implementation of ereport...

Once my ALWAYS DEFERRED patch is done, sure :)

Also, see my other post just now where I propose a thread-based
mechanism for making quickdie() async-signal-safe. Basically, there
would be a pipe and a thread that only blocks in read(2) on that pipe,
and quickdie() would write to the pipe the relevant details, then that
thread would call ereport() and then exit(3). This would be much much
simpler to implement than making ereport() async-signal-safe.

Nico
--

Hi,

On 2018-07-19 15:49:35 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-07-19 11:57:25 +0300, Heikki Linnakangas wrote:

The regular backend's quickdie() function is more tricky. It should also
call _exit(2) rather than exit(2). But it also tries to ereport a WARNING,
and that is quite useful.

There's already an on_exit_reset in there; why do we need more than that?

Because there's plenty things, many of which are not signal safe,
running atexit handlers, not necessarily just ours. exit() simply isn't
signal safe. In Asim's case that's a gnutls (where did that come from)
atexit(). There's plenty other libraries with things like that, not to
speak of PLs.

Is that actually true? Clients like libpq create the same error message
(which has its own issues, because it'll sometimes mis-interpret
things). The message doesn't actually have useful content, no?

Yes, it does: it lets users tell the difference between exit due to a
SIGQUIT and a crash of their own backend.

Not really reliably, though. Given that we can deadlock, the message
might not arrive, the absence doesn't allow to infer that it was a
crash.

Admittedly, if we crash trying to send the message, then we're not
better off. But since that happens only very rarely, I do not think
it's a reasonable tradeoff to never send it at all.

Failovers that randomly take longer aren't all that harmless.

Greetings,

Andres Freund

On 2018-07-19 15:04:15 -0500, Nico Williams wrote:

Besides making ereport() async-signal-safe, which is tricky, you could
write(2) the arguments to a pipe that another thread in the same process
is reading from and which will then call ereport() and exit(3). This
would be less work if you're willing to use a thread for that (the
thread would only block in read(2) on that pipe, and would only provide
this one service).

It'd also increase memory usage noticably (we'd have twice the process
count in the kernel, would have a lot of additional stacks etc), would
tie us to supporting threading in the backend, ... This is a DOA
approach imo.

Greetings,

Andres Freund

On 2018-07-19 14:54:52 -0500, Nico Williams wrote:

On Thu, Jul 19, 2018 at 12:20:53PM -0700, Andres Freund wrote:

On 2018-07-19 11:57:25 +0300, Heikki Linnakangas wrote:

Ugh. Yeah, in wal_quickdie, and other aux process *_quickdie() handlers, I
agree we should just _exit(2). All we want to do is to exit the process
immediately.

Indeed.

bgworker_quickdie() makes some effort to block SIGQUIT during the exit()
processing, but that doesn't solve the whole problem. The process could've
been in the middle of a malloc/free when the signal arrived, for example.
exit() is simply not safe to call from a signal handler.

Yea. I really don't understand why we have't been able to agree on this
for *years* now.

I mean, only calling async-signal-safe functions from signal handlers is
a critical POSIX requirement, and exit(3) is NOT async-signal-safe.

There's a few standard requirements that aren't particularly relevant in
practice (including some functions not being listed as signal
safe). Just arguing from that isn't really helpful. But there's plenty
hard evidence, including a few messages upthread!, of it being
practically problematic. Just looking at the reasoning at why exit (and
malloc) aren't signal safe however, makes it clear that this particular
restriction should be adhered to, however.

The regular backend's quickdie() function is more tricky. It should also
call _exit(2) rather than exit(2). But it also tries to ereport a WARNING,
and that is quite useful.

Is that actually true? Clients like libpq create the same error message
(which has its own issues, because it'll sometimes mis-interpret
things). The message doesn't actually have useful content, no?

Is ereport() async-signal-safe? No. It could be, but it uses stdio to
write to stderr/console, and stdio is not async-signal-safe.

Im not sure what you're trying to tell me here? What you quoted doesn't
advocate doing anything with ereport()?

Greetings,

Andres Freund

Nico Williams <nico@cryptonector.com> writes:

I dunno if it is or isn't helpful. But I do know that this must be done
in an async-signal-safe way.

I haven't actually heard a convincing reason why that's true. As per
the previous discussion, if we happen to service the SIGQUIT at an
unfortunate moment, we might get a deadlock or crash in the backend
process, and thereby fail to send the message. But we're no worse
off in such cases than if we'd not tried to send it at all. The only
likely penalty is that, in the deadlock case, a few seconds will elapse
before the postmaster runs out of patience and sends SIGKILL.

Yeah, it'd be nicer if we weren't taking such risks, but the amount
of effort required to get there seems very far out of proportion to
the benefit.

Besides making ereport() async-signal-safe, which is tricky, you could
write(2) the arguments to a pipe that another thread in the same process
is reading from and which will then call ereport() and exit(3).

We have not yet accepted any patch that introduces threading into the
core backend, and this seems like a particularly unlikely place to
start. Color me dubious, as well, that thread 2 calling exit() (never
mind ereport()) while the main thread continues to do $whatever offers
any actual gain in safety.

regards, tom lane

On Thu, Jul 19, 2018 at 01:10:14PM -0700, Andres Freund wrote:

On 2018-07-19 15:04:15 -0500, Nico Williams wrote:

Besides making ereport() async-signal-safe, which is tricky, you could
write(2) the arguments to a pipe that another thread in the same process
is reading from and which will then call ereport() and exit(3). This
would be less work if you're willing to use a thread for that (the
thread would only block in read(2) on that pipe, and would only provide
this one service).

It'd also increase memory usage noticably (we'd have twice the process
count in the kernel, would have a lot of additional stacks etc), would
tie us to supporting threading in the backend, ... This is a DOA
approach imo.

You can create that thread with a really small stack given that its only
purpose is to do this error reporting and exit.

Running a thread that does only this does not impact the rest of the
code in the backend at all -- it's not "threading" the backend. When it
gets invoked, the caller would be blocking / sleeping, waiting for the
coming exit, while this helper thread would block until invoked. It's
really not a big deal.

I use this technique in some of my programs (unfortunately none in my
github repos). Usually I use it for detection of parent process death
(so that if the parent dies silently, the children die too). In that
case the child-side of fork() closes the write end of a pipe and starts
a thread that blocks in read(2) on the read end of the pipe, and exit()s
when the read returns anything other than EINTR.

Nico
--

On Thu, Jul 19, 2018 at 04:16:31PM -0400, Tom Lane wrote:

Nico Williams <nico@cryptonector.com> writes:

I dunno if it is or isn't helpful. But I do know that this must be done
in an async-signal-safe way.

I haven't actually heard a convincing reason why that's true. As per

It's undefined behavior. The system is free to do all sorts of terrible
things. In practice deadlock or crash are the things you'll see, as you
say.

the previous discussion, if we happen to service the SIGQUIT at an
unfortunate moment, we might get a deadlock or crash in the backend
process, and thereby fail to send the message. But we're no worse
off in such cases than if we'd not tried to send it at all. The only
likely penalty is that, in the deadlock case, a few seconds will elapse
before the postmaster runs out of patience and sends SIGKILL.

Yeah, it'd be nicer if we weren't taking such risks, but the amount
of effort required to get there seems very far out of proportion to
the benefit.

Besides making ereport() async-signal-safe, which is tricky, you could
write(2) the arguments to a pipe that another thread in the same process
is reading from and which will then call ereport() and exit(3).

We have not yet accepted any patch that introduces threading into the
core backend, and this seems like a particularly unlikely place to
start. Color me dubious, as well, that thread 2 calling exit() (never
mind ereport()) while the main thread continues to do $whatever offers
any actual gain in safety.

No, the other thread does NOT continue to do whatever -- it
blocks/sleeps forever waiting for the coming exit(3).

I.e., quickdie() would look like this:

/* Marshall info in to an automatic */
struct quickdie_info info, *infop;

info.this = that;
...
infop = &info;

/* Tell the death thread to report and exit */
/* XXX actually something like net_write(), to handle EINTR */
write(quickdie_pipe[1], infop, sizeof(infop));

/* Wait forever */
for (;;) usleep(FOREVER);

and the thread would basically do:

struct quickdie_info *info;

/* Wait forever for a quickdie() to happen on the main thread */
/* XXX Actually something like net_read(), to handle EINTR */
read(quickdie_pipe[0], &infop, sizeof(infop));

ereport(infop->...);

exit(1);

This use of threads does not require any changes to the rest of the
codebase.

Nico
--