Обсуждение: Missing error_context_stack = NULL in AutoVacWorkerMain()

On Fri, Oct 18, 2019 at 05:55:32PM -0700, Ashwin Agrawal wrote:
> I am not sure if this causes any potential problems or not, but for
> consistency of code seems we are missing below. All other places in code
> where sigsetjmp() exists for top level handling has error_context_stack set
> to NULL.

Resetting error_context_stack prevents calling any callbacks which may
be set.  These would not be much useful in this context anyway, and
visibly that's actually not an issue with the autovacuum code so far
(I don't recall seeing a custom callback setup in this area, but I may
have missed something).  So fixing it would be a good thing actually,
on HEAD.

Any thoughts from others?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Oct 18, 2019 at 05:55:32PM -0700, Ashwin Agrawal wrote:
>> I am not sure if this causes any potential problems or not, but for
>> consistency of code seems we are missing below. All other places in code
>> where sigsetjmp() exists for top level handling has error_context_stack set
>> to NULL.

> Resetting error_context_stack prevents calling any callbacks which may
> be set.  These would not be much useful in this context anyway, and
> visibly that's actually not an issue with the autovacuum code so far
> (I don't recall seeing a custom callback setup in this area, but I may
> have missed something).  So fixing it would be a good thing actually,
> on HEAD.

> Any thoughts from others?

This seems like a real and possibly serious bug to me.  Backend sigsetjmp
callers *must* clear error_context_stack (or restore it to a previous
value), because if it isn't NULL it's surely pointing at garbage, ie a
local variable that's no longer part of the valid stack.

The issue might be argued to be insignificant because the autovacuum
worker is just going to do proc_exit anyway.  But if it encountered
another error during proc_exit, elog.c might try to invoke error
callbacks using garbage callback data.

In short, I think we'd better back-patch too.

            regards, tom lane

I wrote:
> The issue might be argued to be insignificant because the autovacuum
> worker is just going to do proc_exit anyway.  But if it encountered
> another error during proc_exit, elog.c might try to invoke error
> callbacks using garbage callback data.

Oh --- looking closer, proc_exit itself will clear error_context_stack
before doing much.  So a problem would only occur if we suffered an error
during EmitErrorReport, which seems somewhat unlikely.  Still, it's bad
that this code isn't like all the others.  There's certainly no downside
to clearing the pointer.

            regards, tom lane

On Mon, Oct 21, 2019 at 12:47:40AM -0400, Tom Lane wrote:
> This seems like a real and possibly serious bug to me.  Backend sigsetjmp
> callers *must* clear error_context_stack (or restore it to a previous
> value), because if it isn't NULL it's surely pointing at garbage, ie a
> local variable that's no longer part of the valid stack.

Sure.  From my recollection of memories we never set it in autovacuum
code paths (including index entry deletions), so I don't think that we
have an actual live bug here.

> The issue might be argued to be insignificant because the autovacuum
> worker is just going to do proc_exit anyway.  But if it encountered
> another error during proc_exit, elog.c might try to invoke error
> callbacks using garbage callback data.
>
> In short, I think we'd better back-patch too.

Okay, no objections to back-patch.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Oct 21, 2019 at 12:47:40AM -0400, Tom Lane wrote:
>> This seems like a real and possibly serious bug to me.  Backend sigsetjmp
>> callers *must* clear error_context_stack (or restore it to a previous
>> value), because if it isn't NULL it's surely pointing at garbage, ie a
>> local variable that's no longer part of the valid stack.

> Sure.  From my recollection of memories we never set it in autovacuum
> code paths (including index entry deletions), so I don't think that we
> have an actual live bug here.

Uh ... what about, say, auto-analyze on an expression index?  That
could call user-defined PL functions and thus reach just about all
of the backend.

            regards, tom lane

On Mon, Oct 21, 2019 at 12:53:27AM -0400, Tom Lane wrote:
> Oh --- looking closer, proc_exit itself will clear error_context_stack
> before doing much.  So a problem would only occur if we suffered an error
> during EmitErrorReport, which seems somewhat unlikely.  Still, it's bad
> that this code isn't like all the others.  There's certainly no downside
> to clearing the pointer.

Good point about index predicates/expressions.  There is the elog()
hook as well in the area, and it's hard to predict how people use
that.  So applied and back-patched down 9.4.
--
Michael