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
Attachments:
test-for-trigger-troubles.patchtext/x-diff; charset=us-ascii; name=test-for-trigger-troubles.patchDownload+105-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
Attachments:
0001-Fix-double-free-when-one-trigger-passes-OLD-to-anoth.patchapplication/octet-stream; name=0001-Fix-double-free-when-one-trigger-passes-OLD-to-anoth.patchDownload+57-2
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