Re: Server may segfault when using slices on int2vector - Mailing list pgsql-bugs

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)));
      }