Обсуждение: Segmentation fault during update inside ExecBRUpdateTriggers

Hello,

The following code causes a segmentation fault (confirmed in versions
11.4 on Debian Buster, 11.5 on Debian Buster and Arch Linux 64-bit):

CREATE TABLE foo (id SERIAL NOT NULL PRIMARY KEY, bar INTEGER, baz
INTEGER, ud TIMESTAMPTZ, ud2 TIMESTAMPTZ);

CREATE OR REPLACE FUNCTION udu() RETURNS TRIGGER AS $$
    BEGIN
        NEW.ud := current_timestamp;
        RETURN NEW;
    END;
$$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION ud2u() RETURNS TRIGGER AS $$
BEGIN
   IF row(NEW.bar) IS DISTINCT FROM row(OLD.bar) THEN
      NEW.ud2 := current_timestamp;
      RETURN NEW;
   ELSE
      RETURN OLD;
   END IF;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER udt
BEFORE UPDATE ON foo
FOR EACH ROW EXECUTE PROCEDURE udu();

CREATE TRIGGER ud2t
BEFORE UPDATE ON foo
FOR EACH ROW EXECUTE PROCEDURE ud2u();

INSERT INTO foo (bar, baz) VALUES (1, 2);

UPDATE foo SET baz = 5 WHERE id = 1;

Backtrace on Debian Buster:

#0  0x000055c9e358b0c0 in ?? ()
#1  0x000055c9e133d144 in ExecBRUpdateTriggers
(estate=estate@entry=0x55c9e3583190,
epqstate=epqstate@entry=0x55c9e35845c0,
relinfo=relinfo@entry=0x55c9e3583420,
tupleid=tupleid@entry=0x7fff0e1565da,
    fdw_trigtuple=fdw_trigtuple@entry=0x0, slot=0x55c9e3589688) at
./build/../src/backend/commands/trigger.c:3065
#2  0x000055c9e138258e in ExecUpdate
(mtstate=mtstate@entry=0x55c9e3584500, tupleid=0x7fff0e1565da,
oldtuple=0x0, slot=<optimized out>, planSlot=0x55c9e3584dd0,
epqstate=epqstate@entry=0x55c9e35845c0,
    estate=0x55c9e3583190, canSetTag=true) at
./build/../src/backend/executor/nodeModifyTable.c:974
#3  0x000055c9e1382f72 in ExecModifyTable (pstate=0x55c9e3584500) at
./build/../src/backend/executor/nodeModifyTable.c:2166
#4  0x000055c9e135df3b in ExecProcNode (node=0x55c9e3584500) at
./build/../src/include/executor/executor.h:247
#5  ExecutePlan (execute_once=<optimized out>, dest=0x55c9e357ede0,
direction=<optimized out>, numberTuples=0, sendTuples=<optimized out>,
operation=CMD_UPDATE, use_parallel_mode=<optimized out>,
    planstate=0x55c9e3584500, estate=0x55c9e3583190) at
./build/../src/backend/executor/execMain.c:1723
#6  standard_ExecutorRun (queryDesc=0x55c9e35780c0,
direction=<optimized out>, count=0, execute_once=<optimized out>) at
./build/../src/backend/executor/execMain.c:364
#7  0x000055c9e14b7fc7 in ProcessQuery (plan=<optimized out>,
sourceText=0x55c9e348c180 "UPDATE foo SET baz = 5 WHERE id = 1;",
params=0x0, queryEnv=0x0, dest=0x55c9e357ede0,
    completionTag=0x7fff0e156920 "") at ./build/../src/backend/tcop/pquery.c:161
#8  0x000055c9e14b820b in PortalRunMulti
(portal=portal@entry=0x55c9e3525c60, isTopLevel=isTopLevel@entry=true,
setHoldSnapshot=setHoldSnapshot@entry=false,
dest=dest@entry=0x55c9e357ede0,
    altdest=altdest@entry=0x55c9e357ede0,
completionTag=completionTag@entry=0x7fff0e156920 "") at
./build/../src/backend/tcop/pquery.c:1286
#9  0x000055c9e14b8e0f in PortalRun
(portal=portal@entry=0x55c9e3525c60,
count=count@entry=9223372036854775807,
isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
dest=dest@entry=0x55c9e357ede0,
    altdest=altdest@entry=0x55c9e357ede0, completionTag=0x7fff0e156920
"") at ./build/../src/backend/tcop/pquery.c:799
#10 0x000055c9e14b4cce in exec_simple_query
(query_string=0x55c9e348c180 "UPDATE foo SET baz = 5 WHERE id = 1;")
at ./build/../src/backend/tcop/postgres.c:1145
#11 0x000055c9e14b6527 in PostgresMain (argc=<optimized out>,
argv=argv@entry=0x55c9e34ec2c8, dbname=<optimized out>,
username=<optimized out>) at
./build/../src/backend/tcop/postgres.c:4182
#12 0x000055c9e14402d2 in BackendRun (port=0x55c9e34e3a80) at
./build/../src/backend/postmaster/postmaster.c:4358
#13 BackendStartup (port=0x55c9e34e3a80) at
./build/../src/backend/postmaster/postmaster.c:4030
#14 ServerLoop () at ./build/../src/backend/postmaster/postmaster.c:1707
#15 0x000055c9e1441176 in PostmasterMain (argc=5, argv=0x55c9e3486c30)
at ./build/../src/backend/postmaster/postmaster.c:1380
#16 0x000055c9e11bddc9 in main (argc=5, argv=0x55c9e3486c30) at
./build/../src/backend/main/main.c:228

Regards,
Piotr Kosinski

On Fri, Aug 16, 2019 at 9:55 AM Piotr Gabriel Kosinski
<pg.kosinski@gmail.com> wrote:
> Backtrace on Debian Buster:
>
> #0  0x000055c9e358b0c0 in ?? ()
> #1  0x000055c9e133d144 in ExecBRUpdateTriggers
> (estate=estate@entry=0x55c9e3583190,
> epqstate=epqstate@entry=0x55c9e35845c0,
> relinfo=relinfo@entry=0x55c9e3583420,
> tupleid=tupleid@entry=0x7fff0e1565da,
>     fdw_trigtuple=fdw_trigtuple@entry=0x0, slot=0x55c9e3589688) at
> ./build/../src/backend/commands/trigger.c:3065

Hi,

Right, this happens on REL_11_STABLE but not on master (which rewrote
the relevant code quite a bit in the "slotification" project).  It's a
double free, here:

        for (i = 0; i < trigdesc->numtriggers; i++)
        {
...
               if (oldtuple != newtuple && oldtuple != slottuple)
                        heap_freetuple(oldtuple);
...
        }
        if (trigtuple != fdw_trigtuple && trigtuple != newtuple)
                heap_freetuple(trigtuple);

In a very quick test, the following change fixes the problem and
passes regression tests, but I'm not sure if it's the right fix.

-               if (oldtuple != newtuple && oldtuple != slottuple)
+               if (oldtuple != newtuple && oldtuple != slottuple &&
oldtuple != trigtuple)

-- 
Thomas Munro
https://enterprisedb.com

Thomas Munro <thomas.munro@gmail.com> writes:
> Right, this happens on REL_11_STABLE but not on master (which rewrote
> the relevant code quite a bit in the "slotification" project).

We should probably trace back why it doesn't happen before v11.
I have a vague memory of having touched this code a year or two
back, so likely this is my fault, but I wonder why it doesn't
fail before.

> In a very quick test, the following change fixes the problem and
> passes regression tests, but I'm not sure if it's the right fix.

> -               if (oldtuple != newtuple && oldtuple != slottuple)
> +               if (oldtuple != newtuple && oldtuple != slottuple &&
> oldtuple != trigtuple)

My thoughts were headed in the same direction.  It looks like the
issue is that the first trigger returns OLD, ie "trigtuple",
which gets assigned to "newtuple", and then the second iteration
of the loop fails to deal with the aliasing.

[ thinks some more... ]  Actually, I'm beginning to recall that
we made changes here because v11 plpgsql is capable of actually
returning "trigtuple" where before it would always have made a copy.
If that's accurate, then very likely the bug exists further back
but requires some other PL than plpgsql to manifest.

I'd be inclined to put a test case exercising this into all branches,
even ones not currently showing the bug, because it's clearly a
fragile area.

            regards, tom lane

On 2019-08-15 18:39:24 -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > Right, this happens on REL_11_STABLE but not on master (which rewrote
> > the relevant code quite a bit in the "slotification" project).
> 
> We should probably trace back why it doesn't happen before v11.
> I have a vague memory of having touched this code a year or two
> back, so likely this is my fault, but I wonder why it doesn't
> fail before.

There's

commit 25b692568f429436f89ff203c1413e9670d0ad67
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   2018-02-27 13:27:38 -0500

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

But that shouldn't itself have caused it. But the referenced 4b93f5799
might have.

Greetings,

Andres Freund

I wrote:
> [ thinks some more... ]  Actually, I'm beginning to recall that
> we made changes here because v11 plpgsql is capable of actually
> returning "trigtuple" where before it would always have made a copy.
> If that's accurate, then very likely the bug exists further back
> but requires some other PL than plpgsql to manifest.

As I suspected ... the attached test case crashes 9.4 through 11.
We already had some problems in this area, which is why a suitable
trigger is already at hand in regress.c.

            regards, tom lane

diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index c64151b..1e4053c 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 4534dc9..c21b6c1 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;

On Fri, Aug 16, 2019 at 11:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > [ thinks some more... ]  Actually, I'm beginning to recall that
> > we made changes here because v11 plpgsql is capable of actually
> > returning "trigtuple" where before it would always have made a copy.
> > If that's accurate, then very likely the bug exists further back
> > but requires some other PL than plpgsql to manifest.
>
> As I suspected ... the attached test case crashes 9.4 through 11.
> We already had some problems in this area, which is why a suitable
> trigger is already at hand in regress.c.

Ah, I see.  I had written a test patch that uses plpgsql (attached for
posterity) but yours is better because it crashes more releases.  I
will now get out of your way :-)

-- 
Thomas Munro
https://enterprisedb.com

Andres Freund <andres@anarazel.de> writes:
> There's
> commit 25b692568f429436f89ff203c1413e9670d0ad67

> But that shouldn't itself have caused it. But the referenced 4b93f5799
> might have.

Yeah, the problem is that that fix didn't fully close the hole.

            regards, tom lane

Thomas Munro <thomas.munro@gmail.com> writes:
> Ah, I see.  I had written a test patch that uses plpgsql (attached for
> posterity) but yours is better because it crashes more releases.  I
> will now get out of your way :-)

OK, pushed.

            regards, tom lane