Thread: BUG #1723: array_cat() bug when passed empty array

The following bug has been logged online:

Bug reference:      1723
Logged by:          Dave Chapeskie
Email address:      pgsql@ddm.wox.org
PostgreSQL version: 7.4.6
Operating system:   FreeBSD 5.x
Description:        array_cat() bug when passed empty array
Details:

I believe this bug still exists in the latest 8.0.x but
have not tested that version, just looked at the code.

array_cat() has a bug when passed an empty array.  The
code attempts to optimise/short-circuit this case and
returns a pointer to the non-empty argument.  This is
bad/wrong.  Especially when used in a construct like:
  foo := foo || <some_array>
since after array_cat() returns exec_assign_value()
will pfree() 'foo' and then attempt to assign the now
invalid result that points to 'foo'.

Turning on assertion checks (which turns on memory
checks) catches this.  Here is a simple example:

create or replace function array_test (text[],text[])
returns text[] as '
    declare
        tmp text[];
    begin
        tmp := $1;
        tmp := tmp || $2;
        return tmp;
    end'
    language plpgsql
    stable;
select array_test(array['foo','bar'],'{x}');
select array_test(array['foo','bar'],'{}');

The first call to array_test() returns
  {foo,bar,x}
as expected.  With debuging on the second call returns:
ERROR:  out of memory
DETAIL:  Failed on request of size 1065320319.
CONTEXT:  PL/pgSQL function "array_test" while casting return value to
function's return type
Note 1065320319 = 0x3F7F7F7F and with debugging pfree'd
memory is filled with 0x74 bytes.  The top 0x4000000
got cleared because the top bits of the length field
are flag bits.  I've also seen the error say
"Failed on request of size 2139062147.".

"Dave Chapeskie" <pgsql@ddm.wox.org> writes:
> array_cat() has a bug when passed an empty array.  The
> code attempts to optimise/short-circuit this case and
> returns a pointer to the non-empty argument.  This is
> bad/wrong.  Especially when used in a construct like:
>   foo := foo || <some_array>
> since after array_cat() returns exec_assign_value()
> will pfree() 'foo' and then attempt to assign the now
> invalid result that points to 'foo'.

Actually, I would say the bug is exec_assign_value's.  There is nothing
at all wrong with a function returning one of its input values; for
example the smaller/larger functions all do that.  Let's see...

regression=# create or replace function smal(text,text) returns text as $$
regression$# declare tmp text;
regression$# begin
regression$#   tmp := $1;
regression$#   tmp := text_smaller(tmp, $2);
regression$#   return tmp;
regression$# end$$ language plpgsql stable;
CREATE FUNCTION
regression=# select smal('abc', '123');
 smal
------
 123
(1 row)

regression=# select smal('123', 'abc');
ERROR:  out of memory
DETAIL:  Failed on request of size 1065320319.
CONTEXT:  PL/pgSQL function "smal" line 4 at assignment
regression=#

It's very surprising no one noticed this before.  Thanks for the report!

            regards, tom lane

I wrote:
> Actually, I would say the bug is exec_assign_value's.  There is nothing
> at all wrong with a function returning one of its input values; for
> example the smaller/larger functions all do that.

For that matter, you don't need a function at all:

regression=# create or replace function copyit(text) returns text as $$
regression$# declare tmp text;
regression$# begin
regression$#   tmp := $1;
regression$#   tmp := tmp;
regression$#   return tmp;
regression$# end$$ language plpgsql stable;
CREATE FUNCTION
regression=# select copyit('foo');
ERROR:  out of memory
DETAIL:  Failed on request of size 1065320319.
CONTEXT:  PL/pgSQL function "copyit" line 4 at assignment
regression=#

This makes it perfectly clear that the problem is that exec_assign_value
must copy the given value before it frees the old, just in case they're
the same.  (Hmm, I wonder if we can shortcircuit the whole thing ...)

            regards, tom lane

Dave Chapeskie <dchapeskie@sandvine.com> writes:
> On Mon, Jun 20, 2005 at 03:44:24PM -0400, Tom Lane wrote:
>> This makes it perfectly clear that the problem is that exec_assign_value
>> must copy the given value before it frees the old, just in case they're
>> the same.  (Hmm, I wonder if we can shortcircuit the whole thing ...)

> Anyone working on fixing this?  I'd have tried but I didn't want to dive
> into exec_assign_value() and exec_cast_value().

Done some time ago, I think [ digs in CVS log... ]  yeah, here it is:

2005-06-20 16:44  tgl

    * src/pl/plpgsql/src/: pl_exec.c (REL7_3_STABLE), pl_exec.c
    (REL7_4_STABLE), pl_exec.c (REL7_2_STABLE), pl_exec.c
    (REL8_0_STABLE), pl_exec.c: plpgsql's exec_assign_value() freed the
    old value of a variable before copying/converting the new value,
    which meant that it failed badly on "var := var" if var is of
    pass-by-reference type.  Fix this and a similar hazard in
    exec_move_row(); not sure that the latter can manifest before 8.0,
    but patch it all the way back anyway.  Per report from Dave
    Chapeskie.

You can get the patches from our CVS server.
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/pl/plpgsql/src/pl_exec.c

            regards, tom lane

On Mon, Jun 20, 2005 at 03:44:24PM -0400, Tom Lane wrote:

> I [Tom Lane] wrote:
> > Actually, I would say the bug is exec_assign_value's.  There is nothing
> > at all wrong with a function returning one of its input values; for
> > example the smaller/larger functions all do that.
>
> For that matter, you don't need a function at all:
>
> regression=# create or replace function copyit(text) returns text as $$
> regression$# declare tmp text;
> regression$# begin
> regression$#   tmp := $1;
> regression$#   tmp := tmp;
> regression$#   return tmp;
> regression$# end$$ language plpgsql stable;
> CREATE FUNCTION
> regression=# select copyit('foo');
> ERROR:  out of memory
> DETAIL:  Failed on request of size 1065320319.
> CONTEXT:  PL/pgSQL function "copyit" line 4 at assignment
> regression=#
>
> This makes it perfectly clear that the problem is that exec_assign_value
> must copy the given value before it frees the old, just in case they're
> the same.  (Hmm, I wonder if we can shortcircuit the whole thing ...)

Anyone working on fixing this?  I'd have tried but I didn't want to dive
into exec_assign_value() and exec_cast_value().

Given that this occurs for almost any assignment of the form
"x := any expression with x" it's hard to avoid without rewritting a lot
of basic assignments with "tmp:=x; x:=foo(tmp)" which is ugly.

We can't use memory checking with this and even without memory checking
I don't trust that memory isn't getting spamed in some situations.
After all it's referencing free'd memory.

--
Dave Chapeskie