Unsafe coding in ReorderBufferCommit()

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: Unsafe coding in ReorderBufferCommit()
Date: 2015-01-23 21:47:30
Message-ID: 20514.1422049650@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Stephen Frost 2015-01-24 03:24:31 Re: WITH CHECK and Column-Level Privileges
Previous Message Peter Geoghegan 2015-01-23 21:26:29 Re: hung backends stuck in spinlock heavy endless loop