From d6fa672aa3450b44192bef4e1874d9836ca73c0a Mon Sep 17 00:00:00 2001 From: Alexander Lakhin Date: Mon, 8 Jan 2024 08:15:59 +0300 Subject: [PATCH] Fix potential use-after-free for BEFORE ROW UPDATE triggers When oldslot in ExecBRUpdateTriggers() is the only owner of the underlying buffer, fetching a tuple from oldslot leads to freeing the buffer, which is also might be used by newslot with no explicit pin. Thus, fetching a tuple from newslot might lead to invalid read. To fix, materialize newslot before fetching a tuple from oldslot. With the unconditional ExecMaterilizeSlot() call added, the conditional call added by 75e03eabe becomes redundant, hence remove it. Also remove unreachable ExecCopySlot() call in ExecBRUpdateTriggers(). Discussion: https://postgr.es/m/17798-0907404928dcf0dd@postgresql.org --- src/backend/commands/trigger.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 6873c3b4d9..aeae69b987 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -2978,21 +2978,21 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, * received in newslot. Neither we nor our callers have any further * interest in the passed-in tuple, so it's okay to overwrite newslot * with the newer data. - * - * (Typically, newslot was also generated by ExecGetUpdateNewTuple, so - * that epqslot_clean will be that same slot and the copy step below - * is not needed.) */ if (epqslot_candidate != NULL) - { - TupleTableSlot *epqslot_clean; - - epqslot_clean = ExecGetUpdateNewTuple(relinfo, epqslot_candidate, - oldslot); + newslot = ExecGetUpdateNewTuple(relinfo, epqslot_candidate, + oldslot); - if (newslot != epqslot_clean) - ExecCopySlot(newslot, epqslot_clean); - } + /* + * We need to materialize newslot because 1) it might share oldslot's, + * buffer, which might be released on fetching trigtuple from oldslot + * if oldslot is the only owner of the buffer, + * and 2) if some trigger returns/stores the old trigtuple, + * and the heap tuple passed to the trigger was located locally, + * newslot might reference that tuple storage, which is freed at the + * end of this function. + */ + ExecMaterializeSlot(newslot); trigtuple = ExecFetchSlotHeapTuple(oldslot, true, &should_free_trig); } @@ -3049,15 +3049,6 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, { ExecForceStoreHeapTuple(newtuple, newslot, false); - /* - * If the tuple returned by the trigger / being stored, is the old - * row version, and the heap tuple passed to the trigger was - * allocated locally, materialize the slot. Otherwise we might - * free it while still referenced by the slot. - */ - if (should_free_trig && newtuple == trigtuple) - ExecMaterializeSlot(newslot); - if (should_free_new) heap_freetuple(oldtuple); -- 2.34.1