Обсуждение: SEGV in contrib/array/array_iterator.c

Hi,

I have been looking at the functions in array_iterator.so.  So far they have
proved to be very useful.  However, I have manage to find a very serious bug
where the array_iterator() function causes some very bad stack corruption.
The stack corruption appears to be caused because pointer datums are not
checked for NULL before use.

The following SQL will quickly reproduce the problem (assumes contrib/array
stuff has been installed).

   CREATE TABLE person (name VARCHAR(255));
   CREATE TABLE family (name VARCHAR(255), members VARCHAR(255)[]);

   INSERT INTO person VALUES ('bob');
   INSERT INTO person VALUES ('bill');
   INSERT INTO person VALUES ('jim');
   INSERT INTO family VALUES ('Stooges',{"moe","curly","larry"}');

   SELECT name FROM family WHERE members *= (SELECT name FROM person WHERE
   name='jack');

A quick run through GDB shows that when the subselect does not return any
values the *= operator is called with a NULL value which eventually  calls
the array_iterator() function with NULL value==0 which ultimately causes the
segv.

The following patch appears to fix the problem with all supported data types:

Yes, I did verify that int4 and Oid (which can have a 0 value) are not broken.


--- /tmp/postgresql-7.2.1.orig/contrib/array/array_iterator.c
+++ /tmp/postgresql-7.2.1/contrib/array/array_iterator.c
***************
*** 46,65 ****
--- 46,71 ----
        char       *p;
        FmgrInfo        finfo;

        /* Sanity checks */
        if (array == (ArrayType *) NULL)
        {
                /* elog(NOTICE, "array_iterator: array is null"); */
                return (0);
        }

+     if(value == 0)
+     {
+         /* elog(NOTICE, "array_iterator: value is null"); */
+               return (0);
+     }
+
        /* detoast input if necessary */
        array = DatumGetArrayTypeP(PointerGetDatum(array));




--
Matt Peterson
Sr. Software Engineer
Caldera, Inc
matt@caldera.com

Matt Peterson <matt@caldera.com> writes:

> +     if(value == 0)
> +     {
> +         /* elog(NOTICE, "array_iterator: value is null"); */
> +               return (0);
> +     }

This patch is certainly wrong, as it will break array_iterator for
non-pointer datatypes (no, I do not believe your assertion to the
contrary).

More to the point, inserting explicit tests for null is an obsolete
approach; the preferred way to do things these days is to mark the SQL
declarations of such functions with "isStrict".  Would you instead
update contrib/array to rely on isStrict?

It'd be even nicer to update contrib/array to V1 calling conventions,
but unless you are hitting 64-bit porting problems that may not do
anything useful for you...

            regards, tom lane

On Tue, 2 Apr 2002, Tom Lane wrote:

> Matt Peterson <matt@caldera.com> writes:
>
> > +     if(value == 0)
> > +     {
> > +         /* elog(NOTICE, "array_iterator: value is null"); */
> > +               return (0);
> > +     }
>
> This patch is certainly wrong, as it will break array_iterator for
> non-pointer datatypes (no, I do not believe your assertion to the
> contrary).
>
> More to the point, inserting explicit tests for null is an obsolete
> approach; the preferred way to do things these days is to mark the SQL
> declarations of such functions with "isStrict".  Would you instead
> update contrib/array to rely on isStrict?

Are the array iterator functions supposed to act sort of like
=ANY/=ALL except across an array instead of a subselect?  If so,
isStrict probably isn't right, since for an empty subselect the return
value does not depend on the element being searched for.  If not,
that would probably be the easiest fix.

Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
> Are the array iterator functions supposed to act sort of like
> =ANY/=ALL except across an array instead of a subselect?

Seems like a reasonable definition.

> If so,
> isStrict probably isn't right, since for an empty subselect the return
> value does not depend on the element being searched for.

Hm ... isn't it NULL anyway, if the left side is NULL?

But if you're right, then the correct fix involves updating the
functions to V1 calling conventions, so that they can make a correct
test for NULL inputs (rather than bogusly checking for zero value).

            regards, tom lane

On Tue, 2 Apr 2002, Tom Lane wrote:

> Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
> > Are the array iterator functions supposed to act sort of like
> > =ANY/=ALL except across an array instead of a subselect?
>
> Seems like a reasonable definition.
>
> > If so,
> > isStrict probably isn't right, since for an empty subselect the return
> > value does not depend on the element being searched for.
>
> Hm ... isn't it NULL anyway, if the left side is NULL?
>
> But if you're right, then the correct fix involves updating the
> functions to V1 calling conventions, so that they can make a correct
> test for NULL inputs (rather than bogusly checking for zero value).

I think the empty case is special, due to the rules on <quantified
comparison predicate>s.  It looks like no comparison predicates
need to be run.

I think the applicable parts of 8.7 are
General Rule 2a
 If T is empty or if the implied <comparison predicate> is true
 for every row RT in T, then "R <comp op> <all> T" is true.
General Rule 2d
 If T is empty or if the implied <comparison predicate> is false
 for every row RT in T, then "R <comp op> <some> T" is false.

Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
> On Tue, 2 Apr 2002, Tom Lane wrote:
>> Hm ... isn't it NULL anyway, if the left side is NULL?

> I think the empty case is special, due to the rules on <quantified
> comparison predicate>s.

[ reads spec... ]  Yeah, I think you are right.

Looks like our subplan implementation gets it right, too.

            regards, tom lane