Re: [HACKERS] Unsafe coding in ReorderBufferCommit()
Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unsafe coding in ReorderBufferCommit()
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
