From 42451e1c5d84ba00ace2490fd245d354185c5cd3 Mon Sep 17 00:00:00 2001 From: amitlan Date: Wed, 24 Feb 2021 16:58:48 +0900 Subject: [PATCH v2] Fix use-after-tree bug with AfterTriggersTableData.storeslot AfterTriggerSaveEvent() wrongly allocates the slot in execution-span memory context, whereas the correct thing is to allocate it in a transaction-span context, because that's where the enclosing AfterTriggersTableData instance belongs. Author: Bertrand Drouvot Author: Amit Langote --- src/backend/commands/trigger.c | 59 ++++++++++++++++-------- src/test/regress/expected/triggers.out | 58 +++++++++++++++++++++++ src/test/regress/sql/triggers.sql | 64 ++++++++++++++++++++++++++ 3 files changed, 162 insertions(+), 19 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 8908847c6c..7275d5b4cd 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -3536,6 +3536,8 @@ static void AfterTriggerExecute(EState *estate, TupleTableSlot *trig_tuple_slot2); static AfterTriggersTableData *GetAfterTriggersTableData(Oid relid, CmdType cmdType); +static TupleTableSlot *GetAfterTriggerTableStoreSlot(AfterTriggersTableData *table, + TupleDesc tupdesc); static void AfterTriggerFreeQuery(AfterTriggersQueryData *qs); static SetConstraintState SetConstraintStateCreate(int numalloc); static SetConstraintState SetConstraintStateCopy(SetConstraintState state); @@ -4336,6 +4338,38 @@ GetAfterTriggersTableData(Oid relid, CmdType cmdType) return table; } +/* + * Returns the TupleTableSlot suitable for holding the tuples to be put + * into transition table tuplestores. + */ +static TupleTableSlot * +GetAfterTriggerTableStoreSlot(AfterTriggersTableData *table, + TupleDesc tupdesc) +{ + TupleTableSlot *storeslot = table->storeslot; + + /* Create it if not already done. */ + if (!storeslot) + { + /* + * Make the slot valid until end of subtransaction. We really only + * need it until AfterTriggerEndQuery(). + */ + MemoryContext oldcxt = MemoryContextSwitchTo(CurTransactionContext); + + storeslot = MakeSingleTupleTableSlot(tupdesc, &TTSOpsVirtual); + + /* + * Remember it for future requests. Will be released in + * AfterTriggerFreeQuery(). + */ + table->storeslot = storeslot; + MemoryContextSwitchTo(oldcxt); + } + + return storeslot; +} + /* * MakeTransitionCaptureState @@ -4625,6 +4659,8 @@ AfterTriggerFreeQuery(AfterTriggersQueryData *qs) table->new_tuplestore = NULL; if (ts) tuplestore_end(ts); + if (table->storeslot) + ExecDropSingleTupleTableSlot(table->storeslot); } /* @@ -5474,17 +5510,10 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo, if (map != NULL) { + AfterTriggersTableData *table = transition_capture->tcs_private; TupleTableSlot *storeslot; - storeslot = transition_capture->tcs_private->storeslot; - if (!storeslot) - { - storeslot = ExecAllocTableSlot(&estate->es_tupleTable, - map->outdesc, - &TTSOpsVirtual); - transition_capture->tcs_private->storeslot = storeslot; - } - + storeslot = GetAfterTriggerTableStoreSlot(table, map->outdesc); execute_attr_map_slot(map->attrMap, oldslot, storeslot); tuplestore_puttupleslot(old_tuplestore, storeslot); } @@ -5504,18 +5533,10 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo, original_insert_tuple); else if (map != NULL) { + AfterTriggersTableData *table = transition_capture->tcs_private; TupleTableSlot *storeslot; - storeslot = transition_capture->tcs_private->storeslot; - - if (!storeslot) - { - storeslot = ExecAllocTableSlot(&estate->es_tupleTable, - map->outdesc, - &TTSOpsVirtual); - transition_capture->tcs_private->storeslot = storeslot; - } - + storeslot = GetAfterTriggerTableStoreSlot(table, map->outdesc); execute_attr_map_slot(map->attrMap, newslot, storeslot); tuplestore_puttupleslot(new_tuplestore, storeslot); } diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index b263002293..6832a30cfa 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -3290,3 +3290,61 @@ create trigger aft_row after insert or update on trigger_parted create table trigger_parted_p1 partition of trigger_parted for values in (1) partition by list (a); create table trigger_parted_p1_1 partition of trigger_parted_p1 for values in (1); +-- test cases for a bug around AfterTriggersTableData.storeslot +create table convslot_test_parent (col1 text primary key); +create table convslot_test_child (col1 text primary key, + foreign key (col1) references convslot_test_parent(col1) on delete cascade on update cascade +); +alter table convslot_test_child add column col2 text not null default 'tutu'; +insert into convslot_test_parent(col1) values ('1'); +insert into convslot_test_child(col1) values ('1'); +insert into convslot_test_parent(col1) values ('3'); +insert into convslot_test_child(col1) values ('3'); +create or replace function trigger_function1() +returns trigger +language plpgsql +AS $$ +begin +raise notice 'trigger = %, old_table = %', + TG_NAME, + (select string_agg(old_table::text, ', ' order by col1) from old_table); +return null; +end; $$; +create or replace function trigger_function2() +returns trigger +language plpgsql +AS $$ +begin +raise notice 'trigger = %, new table = %', + TG_NAME, + (select string_agg(new_table::text, ', ' order by col1) from new_table); +return null; +end; $$; +create trigger but_trigger after update on convslot_test_child +referencing new table as new_table +for each statement execute function trigger_function2(); +update convslot_test_parent set col1 = col1 || '1'; +NOTICE: trigger = but_trigger, new table = (11,tutu), (31,tutu) +create or replace function trigger_function3() +returns trigger +language plpgsql +AS $$ +begin +raise notice 'trigger = %, old_table = %, new table = %', + TG_NAME, + (select string_agg(old_table::text, ', ' order by col1) from old_table), + (select string_agg(new_table::text, ', ' order by col1) from new_table); +return null; +end; $$; +create trigger but_trigger2 after update on convslot_test_child +referencing old table as old_table new table as new_table +for each statement execute function trigger_function3(); +update convslot_test_parent set col1 = col1 || '1'; +NOTICE: trigger = but_trigger, new table = (111,tutu), (311,tutu) +NOTICE: trigger = but_trigger2, old_table = (11,tutu), (31,tutu), new table = (111,tutu), (311,tutu) +create trigger bdt_trigger after delete on convslot_test_child +referencing old table as old_table +for each statement execute function trigger_function1(); +delete from convslot_test_parent; +NOTICE: trigger = bdt_trigger, old_table = (111,tutu), (311,tutu) +drop table convslot_test_child, convslot_test_parent; diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 01f66af699..dac59a5003 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -2470,3 +2470,67 @@ create trigger aft_row after insert or update on trigger_parted create table trigger_parted_p1 partition of trigger_parted for values in (1) partition by list (a); create table trigger_parted_p1_1 partition of trigger_parted_p1 for values in (1); + +-- test cases for a bug around AfterTriggersTableData.storeslot +create table convslot_test_parent (col1 text primary key); +create table convslot_test_child (col1 text primary key, + foreign key (col1) references convslot_test_parent(col1) on delete cascade on update cascade +); + +alter table convslot_test_child add column col2 text not null default 'tutu'; +insert into convslot_test_parent(col1) values ('1'); +insert into convslot_test_child(col1) values ('1'); +insert into convslot_test_parent(col1) values ('3'); +insert into convslot_test_child(col1) values ('3'); + +create or replace function trigger_function1() +returns trigger +language plpgsql +AS $$ +begin +raise notice 'trigger = %, old_table = %', + TG_NAME, + (select string_agg(old_table::text, ', ' order by col1) from old_table); +return null; +end; $$; + +create or replace function trigger_function2() +returns trigger +language plpgsql +AS $$ +begin +raise notice 'trigger = %, new table = %', + TG_NAME, + (select string_agg(new_table::text, ', ' order by col1) from new_table); +return null; +end; $$; + +create trigger but_trigger after update on convslot_test_child +referencing new table as new_table +for each statement execute function trigger_function2(); + +update convslot_test_parent set col1 = col1 || '1'; + +create or replace function trigger_function3() +returns trigger +language plpgsql +AS $$ +begin +raise notice 'trigger = %, old_table = %, new table = %', + TG_NAME, + (select string_agg(old_table::text, ', ' order by col1) from old_table), + (select string_agg(new_table::text, ', ' order by col1) from new_table); +return null; +end; $$; + +create trigger but_trigger2 after update on convslot_test_child +referencing old table as old_table new table as new_table +for each statement execute function trigger_function3(); +update convslot_test_parent set col1 = col1 || '1'; + +create trigger bdt_trigger after delete on convslot_test_child +referencing old table as old_table +for each statement execute function trigger_function1(); +delete from convslot_test_parent; + +drop table convslot_test_child, convslot_test_parent; -- 2.24.1