Thread: Server may segfault when using slices on int2vector
Hello. While building a query on the pg_index relation, I came accross a bug which simplest form is manifested as this: select a.indkey[1:3], a.indkey[1:2] from pg_index as a This can result either in a segfault, a failed memory allocation or gibberish results. For example, this is a backtrace I could produce while running the above query. It turns out that the int2vector->dim1 member has a dummy value. #0 int2vectorout (fcinfo=<optimized out>) at int.c:192 #1 0x000000000071b445 in FunctionCall1Coll (flinfo=flinfo@entry=0x1ec1360, collation=collation@entry=0, arg1=arg1@entry=32251408) at fmgr.c:1297 #2 0x000000000071c58e in OutputFunctionCall (flinfo=0x1ec1360, val=32251408) at fmgr.c:1950 #3 0x000000000046977d in printtup (slot=0x1ec0300, self=0x1e34c28) at printtup.c:359 #4 0x000000000057eae2 in ExecutePlan (dest=0x1e34c28, direction=<optimized out>, numberTuples=0, sendTuples=1 '\001', operation=CMD_SELECT, planstate=0x1ebff10, estate=0x1ebfe00) at execMain.c:1499 #5 standard_ExecutorRun (queryDesc=0x1e96320, direction=<optimized out>, count=0) at execMain.c:308 #6 0x0000000000652fc8 in PortalRunSelect (portal=portal@entry=0x1ee2680, forward=forward@entry=1 '\001', count=0, count@entry=9223372036854775807, dest=dest@entry=0x1e34c28) at pquery.c:946 #7 0x000000000065432f in PortalRun (portal=portal@entry=0x1ee2680, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=1 '\001', dest=dest@entry=0x1e34c28, altdest=altdest@entry=0x1e34c28, completionTag=completionTag@entry=0x7fff90242090 "") at pquery.c:790 #8 0x00000000006520e5 in exec_simple_query (query_string=0x1e7cfa0 "select \n a.indkey[1:3],\n a.indkey[1:2]\nfrom pg_index as a;") at postgres.c:1048 #9 PostgresMain (argc=<optimized out>, argv=argv@entry=0x1e1b8e8, dbname=0x1e1b798 "postgres", username=<optimized out>) at postgres.c:3992 #10 0x000000000046607d in BackendRun (port=0x1e39b30) at postmaster.c:4085 #11 BackendStartup (port=0x1e39b30) at postmaster.c:3774 #12 ServerLoop () at postmaster.c:1585 #13 0x00000000006123b1 in PostmasterMain (argc=argc@entry=3, argv=argv@entry=0x1e19550) at postmaster.c:1240 #14 0x00000000004669f5 in main (argc=3, argv=0x1e19550) at main.c:196 -- Ronan Dunklau http://dalibo.com - http://dalibo.org
On 19.11.2013 16:24, Ronan Dunklau wrote: > Hello. > > While building a query on the pg_index relation, I came accross a bug which > simplest form is manifested as this: > > select > a.indkey[1:3], > a.indkey[1:2] > from pg_index as a > > This can result either in a segfault, a failed memory allocation or gibberish > results. Hmm. int2vectorout expects the int2vector to have a single dimension, but array_get_slice() returns a zero-dimension array if the result is empty. I don't think it's safe to allow slicing int2vectors (nor oidvectors). It seems all too likely that the result violates the limitations of int2vector. In addition to that segfault, the array returned is 1-based, not 0-based as we assume for int2vectors. One consequence of that is that if you COPY the value out in binary format and try to read it back, you'll get an error. So I think we should just not allow slicing oidvectors, and throw an error. You can cast from int2vector to int2[], and slice and dice that as much as you want, so it's not a big loss in functionality. Another solution would to provide a specialized slice-function for int2vector and oidvector, but it's probably not worth the effort. Thanks for the report! - Heikki
Le mercredi 20 novembre 2013 13:43:48 Heikki Linnakangas a =E9crit : > On 19.11.2013 16:24, Ronan Dunklau wrote: > > Hello. > >=20 > > While building a query on the pg_index relation, I came accross a b= ug > > which > > simplest form is manifested as this: > >=20 > > select > >=20 > > a.indkey[1:3], > > a.indkey[1:2] > >=20 > > from pg_index as a > >=20 > > This can result either in a segfault, a failed memory allocation or= > > gibberish results. >=20 > Hmm. int2vectorout expects the int2vector to have a single dimension,= > but array_get_slice() returns a zero-dimension array if the result is= empty. >=20 > I don't think it's safe to allow slicing int2vectors (nor oidvectors)= . > It seems all too likely that the result violates the limitations of > int2vector. In addition to that segfault, the array returned is 1-bas= ed, > not 0-based as we assume for int2vectors. One consequence of that is > that if you COPY the value out in binary format and try to read it ba= ck, > you'll get an error. >=20 > So I think we should just not allow slicing oidvectors, and throw an > error. You can cast from int2vector to int2[], and slice and dice tha= t > as much as you want, so it's not a big loss in functionality. Another= > solution would to provide a specialized slice-function for int2vector= > and oidvector, but it's probably not worth the effort. >=20 > Thanks for the report! >=20 > - Heikki Are the differences between int2vector and a regular array documented s= omewhere=20 ? What is the purpose of using this datatype instead of int2[] ?=20 On a sidenote, I would probably never have stumbled upon this bug if it= was=20 clear that an int2vector was 0 indexed.=20 --=20 Ronan Dunklau http://dalibo.com - http://dalibo.org
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 19.11.2013 16:24, Ronan Dunklau wrote:
>> [ this crashes: ]
>> select
>> a.indkey[1:3],
>> a.indkey[1:2]
>> from pg_index as a
> I don't think it's safe to allow slicing int2vectors (nor oidvectors).
> It seems all too likely that the result violates the limitations of
> int2vector. In addition to that segfault, the array returned is 1-based,
> not 0-based as we assume for int2vectors. One consequence of that is
> that if you COPY the value out in binary format and try to read it back,
> you'll get an error.
> So I think we should just not allow slicing oidvectors, and throw an
> error. You can cast from int2vector to int2[], and slice and dice that
> as much as you want, so it's not a big loss in functionality.
I think more useful is to automatically cast up to int2[], rather than
make the user write something as ugly as "(a.indkey::int2[])[1:3]".
This case is really pretty similar to what we have to do when handling
domains over arrays; int2vector could be thought of as a domain over
int2[] that constrains the allowed subscripting. And what we do for
those is automatically cast up.
With that thought in mind, I tried tweaking transformArrayType() to
auto-cast int2vector and oidvector to int2[] and oid[]. That resolves
this crash without throwing an error. On the other hand, it causes
assignment to an element or slice of these types to throw an error, which
strikes me as a Good Thing because otherwise such an assignment could
likewise result in a value violating the subscript constraints of these
types. We could if we liked fix that by providing a cast from int2[] to
int2vector that checks the subscript constraints, but like you I'm not
thinking it's worth the trouble. There are certainly no cases in the
system catalogs where we want to support such manual assignments.
While testing that I discovered an independent bug in
transformAssignmentSubscripts: the "probably shouldn't fail" error
reporting code doesn't work because "result" has already been set to NULL.
We should fix that as per attached whether or not we choose to resolve
Ronan's bug this way.
The attached patch is just a quick draft for testing; it needs more work
on the nearby comments, and the OIDs of int2[] and oid[] should be named
by #define's in pg_type.h not by literal constants. If there are no
objections, I'll clean it up and commit.
regards, tom lane
diff --git a/src/backend/parser/parse_node.c b/src/backend/parser/parse_node.c
index 6ffbd76..cba1a19 100644
*** a/src/backend/parser/parse_node.c
--- b/src/backend/parser/parse_node.c
*************** transformArrayType(Oid *arrayType, int32
*** 226,231 ****
--- 226,242 ----
*/
*arrayType = getBaseTypeAndTypmod(*arrayType, arrayTypmod);
+ /*
+ * We treat int2vector and oidvector as though they were domains over
+ * regular int2[] and oid[]. This is because array slicing could create
+ * an array that doesn't necessarily satisfy the dimensionality
+ * constraints of those types.
+ */
+ if (*arrayType == INT2VECTOROID)
+ *arrayType = 1005;
+ else if (*arrayType == OIDVECTOROID)
+ *arrayType = 1028;
+
/* Get the type tuple for the array */
type_tuple_array = SearchSysCache1(TYPEOID, ObjectIdGetDatum(*arrayType));
if (!HeapTupleIsValid(type_tuple_array))
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 9c6c202..24bb090 100644
*** a/src/backend/parser/parse_target.c
--- b/src/backend/parser/parse_target.c
*************** transformAssignmentSubscripts(ParseState
*** 839,846 ****
/* If target was a domain over array, need to coerce up to the domain */
if (arrayType != targetTypeId)
{
result = coerce_to_target_type(pstate,
! result, exprType(result),
targetTypeId, targetTypMod,
COERCION_ASSIGNMENT,
COERCE_IMPLICIT_CAST,
--- 839,848 ----
/* If target was a domain over array, need to coerce up to the domain */
if (arrayType != targetTypeId)
{
+ Oid resulttype = exprType(result);
+
result = coerce_to_target_type(pstate,
! result, resulttype,
targetTypeId, targetTypMod,
COERCION_ASSIGNMENT,
COERCE_IMPLICIT_CAST,
*************** transformAssignmentSubscripts(ParseState
*** 850,856 ****
ereport(ERROR,
(errcode(ERRCODE_CANNOT_COERCE),
errmsg("cannot cast type %s to %s",
! format_type_be(exprType(result)),
format_type_be(targetTypeId)),
parser_errposition(pstate, location)));
}
--- 852,858 ----
ereport(ERROR,
(errcode(ERRCODE_CANNOT_COERCE),
errmsg("cannot cast type %s to %s",
! format_type_be(resulttype),
format_type_be(targetTypeId)),
parser_errposition(pstate, location)));
}
Le samedi 23 novembre 2013 15:39:39 Tom Lane a =E9crit :
> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> > On 19.11.2013 16:24, Ronan Dunklau wrote:
> >> [ this crashes: ]
> >> select
> >> a.indkey[1:3],
> >> a.indkey[1:2]
> >> from pg_index as a
> >=20
> > I don't think it's safe to allow slicing int2vectors (nor oidvector=
s).
> > It seems all too likely that the result violates the limitations of=
> > int2vector. In addition to that segfault, the array returned is 1-b=
ased,
> > not 0-based as we assume for int2vectors. One consequence of that i=
s
> > that if you COPY the value out in binary format and try to read it =
back,
> > you'll get an error.
> >=20
> > So I think we should just not allow slicing oidvectors, and throw a=
n
> > error. You can cast from int2vector to int2[], and slice and dice t=
hat
> > as much as you want, so it's not a big loss in functionality.
>=20
> I think more useful is to automatically cast up to int2[], rather tha=
n
> make the user write something as ugly as "(a.indkey::int2[])[1:3]".
> This case is really pretty similar to what we have to do when handlin=
g
> domains over arrays; int2vector could be thought of as a domain over
> int2[] that constrains the allowed subscripting. And what we do for
> those is automatically cast up.
>=20
> With that thought in mind, I tried tweaking transformArrayType() to
> auto-cast int2vector and oidvector to int2[] and oid[]. That resolve=
s
> this crash without throwing an error. On the other hand, it causes
> assignment to an element or slice of these types to throw an error, w=
hich
> strikes me as a Good Thing because otherwise such an assignment could=
> likewise result in a value violating the subscript constraints of the=
se
> types. We could if we liked fix that by providing a cast from int2[]=
to
> int2vector that checks the subscript constraints, but like you I'm no=
t
> thinking it's worth the trouble. There are certainly no cases in the=
> system catalogs where we want to support such manual assignments.
>=20
> While testing that I discovered an independent bug in
> transformAssignmentSubscripts: the "probably shouldn't fail" error
> reporting code doesn't work because "result" has already been set to =
NULL.
> We should fix that as per attached whether or not we choose to resolv=
e
> Ronan's bug this way.
>=20
> The attached patch is just a quick draft for testing; it needs more w=
ork
> on the nearby comments, and the OIDs of int2[] and oid[] should be na=
med
> by #define's in pg_type.h not by literal constants. If there are no
> objections, I'll clean it up and commit.
>=20
> =09=09=09regards, tom lane
Thank you for the match, I saw it got commited.
I have some more questions, however not directly related to this bug.
Casting from int2vector to int2[] returns a zero-indexed array, but sli=
cing an=20
int2vector returns a one-indexed array. This behavior is consistent wit=
h=20
slicing in general, which always returns 1 indexed arrays, but I found =
it=20
surprising, especially when writing a query like this:
select ('1 2'::int2vector)[0:1] =3D ('1 2'::int2vector)::int2[];
I would have expected to return true, but it actually isn't because one=
array=20
is zero-indexed and the other one is not.
Is there a more convenient way to change the lower bound of an array th=
an to=20
slice it ?
Thank you very much.
--=20
Ronan Dunklau
http://dalibo.com - http://dalibo.org