Re: Segfault with before triggers and after triggers with a WHEN clause.

Yoran Heling <info(at)yorhel(dot)nl> writes:
> After upgrading to PostgreSQL 9.0.4 (don't remember exactly where I
> came from, but I believe it was an earlier 9.0.x), postgresql began to
> segault on certain queries. I have managed to isolate the problem and
> can reproduce the crash on a newly created and empty database with the
> following queries:

Thanks, nice example! I traced through this and found that:

1. ExecBRUpdateTriggers returns the tuple-modified-by-the-before-trigger
in the estate->es_trig_tuple_slot slot.

2. ExecUpdate does ExecMaterializeSlot() on that slot. Now the slot
has a privately allocated copy of the tuple. (This is necessary since
we'll scribble on the tuple's header fields during heap_update.)

3. During ExecARUpdateTriggers, TriggerEnabled needs to put the new
tuple into a slot for execution of the WHEN condition. It thinks it
can use the estate->es_trig_tuple_slot slot for this, but it's passing
the same tuple *already* stored in that slot to ExecStoreTuple.
ExecStoreTuple sees it's clearing a slot with shouldFree = true, so
it pfree's the tuple, and then stores a dangling pointer back into
the slot. Ooops.

TriggerEnabled's apparently-similar use of estate->es_trig_oldtup_slot
is perfectly safe because that slot is actually dedicated for the use
of this function. The safest fix for this bug would be to make another
dedicated slot for the new tuple too. That will require adding a field
to EState, which is a bit risky in released branches, but I think we
can get away with it if we add the field at the end of the struct.
We did the same in a post-release 8.1 patch (in fact, that was adding
es_trig_tuple_slot itself) and did not get complaints.

The only alternative I can see that doesn't add another field to EState
is to hack the TriggerEnabled code so that it checks if the tuple is
already stored in the slot and skips ExecStoreTuple if so. That seems
like a modularity violation, though: it'd require more knowledge about
the detailed behavior of slots than I think this function ought to have.
And it's still fairly fragile, in that es_trig_tuple_slot is mainly
meant for the use of the layer of functions that are calling
TriggerEnabled --- it's not hard to foresee other bugs if we rearrange
the timing of the existing ExecStoreTuple calls in ExecBRUpdateTriggers
and friends.

regards, tom lane