From aed967d697de19a78a653926c72604f9b04c3b1e Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 15 Aug 2019 20:04:19 -0400
Subject: [PATCH] Prevent possible double-free when update trigger returns old
 tuple.

This is a variant of the problem fixed in commit 25b692568, which
unfortunately we failed to detect at the time.  If an update trigger
returns the "old" tuple, as it's entitled to do, then a subsequent
iteration of the loop in ExecBRUpdateTriggers would have "oldtuple"
equal to "trigtuple" and would fail to notice that it shouldn't
free that.

In addition to fixing the code, extend the test case added by
25b692568 so that it covers multiple-trigger-iterations cases.

This problem does not manifest in v12/HEAD, as a result of the
relevant code having been largely rewritten for slotification.
However, include the test case into v12/HEAD anyway, since this
is clearly an area that someone could break again in future.

Per report from Piotr Gabriel Kosinski.  Back-patch into all
supported branches, since the bug seems quite old.

Diagnosis and code fix by Thomas Munro, test case by me.

Discussion: https://postgr.es/m/CAFMLSdP0rd7LqC3j-H6Fh51FYSt5A10DDh-3=W4PPc4LLUQ8YQ@mail.gmail.com
---
 src/backend/commands/trigger.c         |  4 +-
 src/test/regress/expected/triggers.out | 70 ++++++++++++++++++++++++++
 src/test/regress/sql/triggers.sql      | 35 +++++++++++++
 3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index a716827c77a..97ab357a248 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3052,7 +3052,9 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
 									   relinfo->ri_TrigFunctions,
 									   relinfo->ri_TrigInstrument,
 									   GetPerTupleMemoryContext(estate));
-		if (oldtuple != newtuple && oldtuple != slottuple)
+		if (oldtuple != newtuple &&
+			oldtuple != slottuple &&
+			oldtuple != trigtuple)
 			heap_freetuple(oldtuple);
 		if (newtuple == NULL)
 		{
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 5be0009f00f..8d8f4f2f153 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -144,6 +144,76 @@ select * from trigtest;
 ----+----
 (0 rows)
 
+-- Also check what happens when such a trigger runs before or after others
+create function f1_times_10() returns trigger as
+$$ begin new.f1 := new.f1 * 10; return new; end $$ language plpgsql;
+create trigger trigger_alpha
+	before insert or update on trigtest
+	for each row execute procedure f1_times_10();
+insert into trigtest values(1, 'foo');
+select * from trigtest;
+ f1 | f2  
+----+-----
+ 10 | foo
+(1 row)
+
+update trigtest set f2 = f2 || 'bar';
+select * from trigtest;
+ f1 | f2  
+----+-----
+ 10 | foo
+(1 row)
+
+delete from trigtest;
+select * from trigtest;
+ f1 | f2 
+----+----
+(0 rows)
+
+create trigger trigger_zed
+	before insert or update on trigtest
+	for each row execute procedure f1_times_10();
+insert into trigtest values(1, 'foo');
+select * from trigtest;
+ f1  | f2  
+-----+-----
+ 100 | foo
+(1 row)
+
+update trigtest set f2 = f2 || 'bar';
+select * from trigtest;
+  f1  | f2  
+------+-----
+ 1000 | foo
+(1 row)
+
+delete from trigtest;
+select * from trigtest;
+ f1 | f2 
+----+----
+(0 rows)
+
+drop trigger trigger_alpha on trigtest;
+insert into trigtest values(1, 'foo');
+select * from trigtest;
+ f1 | f2  
+----+-----
+ 10 | foo
+(1 row)
+
+update trigtest set f2 = f2 || 'bar';
+select * from trigtest;
+ f1  | f2  
+-----+-----
+ 100 | 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 (
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 6376046c5da..337fbb5e4da 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -117,6 +117,41 @@ select * from trigtest;
 delete from trigtest;
 select * from trigtest;
 
+-- Also check what happens when such a trigger runs before or after others
+create function f1_times_10() returns trigger as
+$$ begin new.f1 := new.f1 * 10; return new; end $$ language plpgsql;
+
+create trigger trigger_alpha
+	before insert or update on trigtest
+	for each row execute procedure f1_times_10();
+
+insert into trigtest values(1, 'foo');
+select * from trigtest;
+update trigtest set f2 = f2 || 'bar';
+select * from trigtest;
+delete from trigtest;
+select * from trigtest;
+
+create trigger trigger_zed
+	before insert or update on trigtest
+	for each row execute procedure f1_times_10();
+
+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 trigger trigger_alpha on trigtest;
+
+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;