Prevent dangling-pointer access when update trigger returns old tuple.

A before-update row trigger may choose to return the "new" or "old" tuple
unmodified.  ExecBRUpdateTriggers failed to consider the second
possibility, and would proceed to free the "old" tuple even if it was the
one returned, leading to subsequent access to already-deallocated memory.
In debug builds this reliably leads to an "invalid memory alloc request
size" failure; in production builds it might accidentally work, but data
corruption is also possible.

This is a very old bug.  There are probably a couple of reasons it hasn't
been noticed up to now.  It would be more usual to return NULL if one
wanted to suppress the update action; returning "old" is significantly less
efficient since the update will occur anyway.  Also, none of the standard
PLs would ever cause this because they all returned freshly-manufactured
tuples even if they were just copying "old".  But commit 4b93f5799 changed
that for plpgsql, making it possible to see the bug with a plpgsql trigger.
Still, this is certainly legal behavior for a trigger function, so it's
ExecBRUpdateTriggers's fault not plpgsql's.

It seems worth creating a test case that exercises returning "old" directly
with a C-language trigger; testing this through plpgsql seems unreliable
because its behavior might change again.

Report and fix by Rushabh Lathia; regression test case by me.
Back-patch to all supported branches.

Discussion: https://postgr.es/m/CAGPqQf1P4pjiNPrMof=P_16E-DFjt457j+nH2ex3=nBTew7tXw@mail.gmail.com

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index fffc0095a79509ed6463e68e6707738529fee352..fbd176b5d03de0284c421d4e08ea9c5583bf614c 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2815,7 +2815,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
                        return NULL;            /* "do nothing" */
                }
        }
-       if (trigtuple != fdw_trigtuple)
+       if (trigtuple != fdw_trigtuple && trigtuple != newtuple)
                heap_freetuple(trigtuple);
 
        if (newtuple != slottuple)

diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 1b99dec27dc75477409876062e8f996ed690dd1c..99be9ac6e9eb3ae6d98e2b9abf5badf60b00b2e4 100644 (file)
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -119,6 +119,32 @@ SELECT trigger_name, event_manipulation, event_object_schema, event_object_table
 DROP TABLE pkeys;
 DROP TABLE fkeys;
 DROP TABLE fkeys2;
+-- Check behavior when trigger returns unmodified trigtuple
+create table trigtest (f1 int, f2 text);
+create trigger trigger_return_old
+       before insert or delete or update on trigtest
+       for each row execute procedure trigger_return_old();
+insert into trigtest values(1, 'foo');
+select * from trigtest;
+ f1 | f2  
+----+-----
+  1 | foo
+(1 row)
+
+update trigtest set f2 = f2 || 'bar';
+select * from trigtest;
+ f1 | f2  
+----+-----
+  1 | foo
+(1 row)
+
+delete from trigtest;
+select * from trigtest;
+ f1 | f2 
+----+----
+(0 rows)
+
+drop table trigtest;
 create sequence ttdummy_seq increment 10 start 0 minvalue 0;
 create table tttest (
        price_id        int4,

diff --git a/src/test/regress/input/create_function_1.source b/src/test/regress/input/create_function_1.source
index 825974d2ac1ae6268c0ab5f5fa726c1a77c3da79..04511050b1422cfa883c5f2b15a9b8e347be74e2 100644 (file)
--- a/src/test/regress/input/create_function_1.source
+++ b/src/test/regress/input/create_function_1.source
@@ -37,6 +37,11 @@ CREATE FUNCTION autoinc ()
        AS '@libdir@/autoinc@DLSUFFIX@'
        LANGUAGE C;
 
+CREATE FUNCTION trigger_return_old ()
+        RETURNS trigger
+        AS '@libdir@/regress@DLSUFFIX@'
+        LANGUAGE C;
+
 CREATE FUNCTION ttdummy ()
         RETURNS trigger
         AS '@libdir@/regress@DLSUFFIX@'

diff --git a/src/test/regress/output/create_function_1.source b/src/test/regress/output/create_function_1.source
index 6c3f5e9c64bbcb9ad60a1d8c10e0fb47df68bd28..18d2a1501683a407f60966b869cc498a4120e2da 100644 (file)
--- a/src/test/regress/output/create_function_1.source
+++ b/src/test/regress/output/create_function_1.source
@@ -35,6 +35,10 @@ CREATE FUNCTION autoinc ()
        RETURNS trigger
        AS '@libdir@/autoinc@DLSUFFIX@'
        LANGUAGE C;
+CREATE FUNCTION trigger_return_old ()
+        RETURNS trigger
+        AS '@libdir@/regress@DLSUFFIX@'
+        LANGUAGE C;
 CREATE FUNCTION ttdummy ()
         RETURNS trigger
         AS '@libdir@/regress@DLSUFFIX@'

diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 064351f7b07d80060e4752a865754cb723cd531f..a0058ed6a8239dbbf94c4e403f1125217111bfc9 100644 (file)
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -203,6 +203,21 @@ reverse_name(PG_FUNCTION_ARGS)
        PG_RETURN_CSTRING(new_string);
 }
 
+PG_FUNCTION_INFO_V1(trigger_return_old);
+
+Datum
+trigger_return_old(PG_FUNCTION_ARGS)
+{
+       TriggerData *trigdata = (TriggerData *) fcinfo->context;
+       HeapTuple       tuple;
+
+       if (!CALLED_AS_TRIGGER(fcinfo))
+               elog(ERROR, "trigger_return_old: not fired by trigger manager");
+
+       tuple = trigdata->tg_trigtuple;
+
+       return PointerGetDatum(tuple);
+}
 
 #define TTDUMMY_INFINITY       999999
 

diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index b7643826e8fe10dc04f166e768cac3bbefa7db0f..3354f4899f75beef09a2e9d86a8791430f122cdf 100644 (file)
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -103,6 +103,22 @@ DROP TABLE pkeys;
 DROP TABLE fkeys;
 DROP TABLE fkeys2;
 
+-- Check behavior when trigger returns unmodified trigtuple
+create table trigtest (f1 int, f2 text);
+
+create trigger trigger_return_old
+       before insert or delete or update on trigtest
+       for each row execute procedure trigger_return_old();
+
+insert into trigtest values(1, 'foo');
+select * from trigtest;
+update trigtest set f2 = f2 || 'bar';
+select * from trigtest;
+delete from trigtest;
+select * from trigtest;
+
+drop table trigtest;
+
 create sequence ttdummy_seq increment 10 start 0 minvalue 0;
 
 create table tttest (