Server may segfault when using slices on int2vector

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

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

Le mercredi 20 novembre 2013 13:43:48 Heikki Linnakangas a écrit :

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

Are the differences between int2vector and a regular array documented somewhere
? What is the purpose of using this datatype instead of int2[] ?

On a sidenote, I would probably never have stumbled upon this bug if it was
clear that an int2vector was 0 indexed.

--
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

Le samedi 23 novembre 2013 15:39:39 Tom Lane a écrit :

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

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 slicing an
int2vector returns a one-indexed array. This behavior is consistent with
slicing in general, which always returns 1 indexed arrays, but I found it
surprising, especially when writing a query like this:

select ('1 2'::int2vector)[0:1] = ('1 2'::int2vector)::int2[];

I would have expected to return true, but it actually isn't because one array
is zero-indexed and the other one is not.

Is there a more convenient way to change the lower bound of an array than to
slice it ?

Thank you very much.

--
Ronan Dunklau
http://dalibo.com - http://dalibo.org

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