BUG #14174: Expanded-datum bug accessing freed memory

andrew@tao11.riddles.org.uk writes:

This is a testcase distilled from a problem reported on IRC:

create or replace function afn1(integer) returns int[] language plpgsql
as $f$ declare r int[]; begin select into r array[$1,$1]; return r; end;
$f$;
create or replace function afn2(int[]) returns integer language plpgsql
as $f$ begin return $1[1]; end; $f$;
create or replace view v_test as select i, a from (select afn1(1) as a
offset 0) s, lateral afn2(a) i;
create function tstfn(integer) returns setof v_test language plpgsql
as $f$ begin return query select * from v_test where i=$1; end; $f$;
select * from tstfn(1);

ERROR: cache lookup failed for type 2139062143

Hm, you don't need tstfn(), nor even the view:

regression=# select i, a from (select afn1(1) as a offset 0) s, lateral afn2(a) i;
ERROR: cache lookup failed for type 2139062143

The core of the problem here is that afn1() is returning a read-write
pointer to the expanded object holding "r", and then when that's passed
to afn2(), it supposes that it can take ownership of it as a read-write
local variable; which means the value gets destroyed when afn2() exits.
That'd be all right except there's another reference to "a" still to be
evaluated :-(

We should have forestalled this by applying MakeExpandedObjectReadOnly
to the output of the first sub-select. But we didn't because that's
currently only done by SubqueryNext, and the SubqueryScan node that
would have fixed this has been optimized away:

Nested Loop (cost=0.25..0.54 rows=1 width=36)
Output: i.i, (afn1(1))
-> Result (cost=0.00..0.26 rows=1 width=32)
Output: afn1(1)
-> Function Scan on public.afn2 i (cost=0.25..0.26 rows=1 width=4)
Output: i.i
Function Call: afn2((afn1(1)))

I am thinking maybe we need to have ExecProject do
MakeExpandedObjectReadOnly on each result, rather than assuming that
SubqueryScan is the place for that. This would slightly increase the
general overhead attributable to the expanded-object feature, which is
unfortunate, but right now it's not clear that anything less is safe.
Making Result nodes do that would fix this particular instance but
there are plenty of other node types that might appear at the top of
a sub-select.

A possible future improvement is to teach the planner to detect which
variables are actually multiply referenced, and force read-only-ness
for only those values. But that's clearly not something that would be
reasonable to back-patch into 9.5, or even 9.6 at this point.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

[CC'ing in the original reporter, edward.greve at gmail.com]

"Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

Tom> The core of the problem here is that afn1() is returning a
Tom> read-write pointer to the expanded object holding "r", and then
Tom> when that's passed to afn2(), it supposes that it can take
Tom> ownership of it as a read-write local variable; which means the
Tom> value gets destroyed when afn2() exits. That'd be all right
Tom> except there's another reference to "a" still to be evaluated :-(

Yeah, I figured it was something like that, but this is the first time
I've had to look at the expanded object stuff.

Tom> I am thinking maybe we need to have ExecProject do
Tom> MakeExpandedObjectReadOnly on each result, rather than assuming
Tom> that SubqueryScan is the place for that. This would slightly
Tom> increase the general overhead attributable to the expanded-object
Tom> feature, which is unfortunate, but right now it's not clear that
Tom> anything less is safe.

I concur.

Tom> Making Result nodes do that would fix this particular instance but
Tom> there are plenty of other node types that might appear at the top
Tom> of a sub-select.

The Result node here is certainly an artifact of the testcase
construction; the original report (which featured about 300 lines of
view and function definitions, some of them with additional subselects,
nested in various ways) would probably have had an Agg node at the
relevant spot, and could conceivably have had any projecting node type
AFAIK.

Tom> A possible future improvement is to teach the planner to detect
Tom> which variables are actually multiply referenced, and force
Tom> read-only-ness for only those values. But that's clearly not
Tom> something that would be reasonable to back-patch into 9.5, or even
Tom> 9.6 at this point.

Clearly.

--
Andrew (irc:RhodiumToad)

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs