Обсуждение: Unsafe coding in ReorderBufferCommit()

There are at least two bugs in reorderbuffer.c's ReorderBufferCommit():

1. Although "iterstate" is modified within the PG_TRY segment and
referenced within the PG_CATCH segment, it is not marked "volatile".
This means that its value upon reaching the PG_CATCH segment is
indeterminate.  In practice, what can happen is that it gets set back
to its value at the time of reaching PG_TRY, which will always be NULL,
so that the effect would be to miss out calling
ReorderBufferIterTXNFinish in the CATCH code.

2. On the other hand, if we get past the ReorderBufferIterTXNFinish
call within the PG_TRY block and then suffer a failure,
ReorderBufferIterTXNFinish will be called again in the PG_CATCH block.
This is due to failure to reset iterstate to NULL after the finish call.
(So this error could be masked if the compiler did cause iterstate
to revert to NULL during longjmp.)

I'm not sure whether #1 is harmless, but #2 most certainly isn't, as
it would result in access to already-freed memory.

The first of these was pointed out to me by Mark Wilding of Salesforce.
It's really pretty distressing that modern versions of gcc don't warn
about this (not even with -Wclobbered).  The very ancient gcc on "gaur"
does warn, but my experience is that it emits a lot of false positives
too, so I'm not that eager anymore to plaster "volatile" on any variable
it whinges about.  Still, it sure looks like we need a "volatile" here.
        regards, tom lane

Hi Tom,

On 2015-01-23 16:47:30 -0500, Tom Lane wrote:
> There are at least two bugs in reorderbuffer.c's ReorderBufferCommit():

Thanks for fixing these!

Unfortunately there's more - we'll currently do bad things if
transaction commit fails. At the very least the (sub-)transaction begin
commands need to be moved out of the exception block as they can
fail... :(. E.g. because this is the 2^32-1 subxact or similar...

I actually also want to strip the CATCH block of most of it's contents -
there's really no need anymore for most of what it does.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2015-01-23 16:47:30 -0500, Tom Lane wrote:
>> There are at least two bugs in reorderbuffer.c's ReorderBufferCommit():

> Thanks for fixing these!

> Unfortunately there's more - we'll currently do bad things if
> transaction commit fails. At the very least the (sub-)transaction begin
> commands need to be moved out of the exception block as they can
> fail... :(. E.g. because this is the 2^32-1 subxact or similar...

> I actually also want to strip the CATCH block of most of it's contents -
> there's really no need anymore for most of what it does.

No objection here.  I was just doing a mechanical transform of the
function, not based on any deep understanding of what it does.

The less you need to do in a CATCH block, the better.
        regards, tom lane