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 2d61dc4818d52dc65abcb4615f748acf9629b964..b949bc9de0117ebef1e34a13527db6744e748488 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2446,7 +2446,8 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
                        return NULL;            /* "do nothing" */
                }
        }
-       heap_freetuple(trigtuple);
+       if (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 6706021051ad54125c414de2c24e4688d9db8f5e..6ca48dd9bb120017f96c0ee9827cff67fefab1a2 100644 (file)
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -133,6 +133,32 @@ DROP TABLE fkeys2;
 -- select count(*) from dup17 where x = 13;
 --
 -- DROP TABLE dup17;
+-- 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 a72dd9861c54708d812b8ec4581705a532b728e0..de1e7054eafdc59b522767892fe31cef9faf5c15 100644 (file)
--- a/src/test/regress/input/create_function_1.source
+++ b/src/test/regress/input/create_function_1.source
@@ -42,6 +42,11 @@ CREATE FUNCTION funny_dup17 ()
         AS '@libdir@/regress@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 61b87ed953aa6b0a14ee0c96eb9f6f616924947e..8563aa0b8f9ce5f4be4b91a4c75dc8fc4281cb18 100644 (file)
--- a/src/test/regress/output/create_function_1.source
+++ b/src/test/regress/output/create_function_1.source
@@ -39,6 +39,10 @@ CREATE FUNCTION funny_dup17 ()
         RETURNS trigger
         AS '@libdir@/regress@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 8d0eec95a8f0690c236631eab1d677fca19e6f2e..8c54ceccb8e6c5331055b095a1aa5bf6af24e8ac 100644 (file)
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -456,6 +456,24 @@ funny_dup17(PG_FUNCTION_ARGS)
        return PointerGetDatum(tuple);
 }
 
+extern Datum trigger_return_old(PG_FUNCTION_ARGS);
+
+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);
+}
+
 extern Datum ttdummy(PG_FUNCTION_ARGS);
 extern Datum set_ttdummy(PG_FUNCTION_ARGS);
 

diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 0ea2c314dee49735cc0fce4b35a8d5e057626532..c4ff1f3f80e711dd4056804c9971731269c1153e 100644 (file)
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -131,6 +131,22 @@ DROP TABLE fkeys2;
 --
 -- DROP TABLE dup17;
 
+-- 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 (