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 e75a59d29958ef99e323cbb9133d4bc19b1025e9..2886aebef4be5e50f16abf1b94f53701e75e7a31 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 1d5405aad9e87774250fd8aa909f252a5ec3d548..007fa74bb62dca4d506307e46fcd2c7c1a72d30f 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 f2b1561cc24d41d263e9bcfcd8717f4aec8f6d50..f89038eb5e91d4600845a9cabad47da1ab4b80be 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 957595c51e477c262501d804e5ef2f9afee31701..557d629ad81d8afad65b7a6cb13f54c8e9805d25 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 b73bccec3d4fbf835a6bf9126e04bd553e55f2c9..a6af6d59c39b12167e29d69c8bc655ac565dd047 100644 (file)
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -446,6 +446,22 @@ funny_dup17(PG_FUNCTION_ARGS)
        return PointerGetDatum(tuple);
 }
 
+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
 
 static SPIPlanPtr splan = NULL;

diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 2b2236ed7d9362696d47dfad5b90642c69afd191..434710ff202417c5b409cd58d3cb5f0cb4051f5d 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 (