Обсуждение: 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