[PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters

Joseph Myers joseph@codesourcery.com
Wed Aug 12 23:19:49 GMT 2020


On Fri, 7 Aug 2020, Martin Sebor via Gcc-patches wrote:

> > I don't see anything in the tests in this patch to cover this sort of case
> > (arrays of pointers, including arrays of pointers to arrays etc.).
> 
> I've added a few test cases and reworked the declarator parsing
> (get_parm_array_spec) a bit, fixing some bugs.

I don't think get_parm_array_spec is yet logically right (and I don't see 
tests of the sort of cases I'm concerned about, such as arrays of pointers 
to arrays, or pointers with attributes applied to them).

You have logic

+      if (pd->kind == cdk_pointer
+         && (!next || next->kind == cdk_id))
+       {
+         /* Do nothing for the common case of a pointer.  The fact that
+            the parameter is one can be deduced from the absence of
+            an arg spec for it.  */
+         return attrs;
+       }

which is correct as far as it goes (when it returns with nothing done, 
it's correct to do so, because the argument is indeed a pointer), but 
incomplete:

* Maybe cdk_pointer is followed by cdk_attrs before cdk_id.  In this case 
the code won't return.

* Maybe the code is correct to continue because we're in the case of an 
array of pointers (cdk_array follows).  But as I understand it, the intent 
is to set up an "arg spec" that describes only the (multidimensional) 
array that is the parameter itself - not any array pointed to.  And it 
looks to me like, in the case of an array of pointers to arrays, both sets 
of array bounds would end up in the spec constructed.

What I think is correct is for both cdk_pointer and cdk_function to result 
in the spec built up so far being cleared (regardless of what follows 
cdk_pointer or cdk_function), rather than early return, so that the spec 
present at the end is for the innermost sequence of array declarators 
(possibly with attributes involved as well).  (cdk_function shouldn't 
actually be an issue, since functions can't return arrays or functions, 
but logically it seems appropriate to treat it like cdk_pointer.)

Then, the code

+      if (pd->kind == cdk_id)
+       {
+         /* Extract the upper bound from a parameter of an array type.  */

also seems misplaced.  If the type specifiers for the parameter are a 
typedef for an array type, that array type should be processed *before* 
the declarator to get the correct semantics (as if the bounds from those 
type specifiers were given in the declarator), not at the end which gets 
that type out of order with respect to array declarators.  (Processing 
before the declarator also means clearing the results of that processing 
if a pointer declarator is encountered at any point, because in that case 
the array type in the type specifiers is irrelevant.)

The logic

+         /* Skip all constant bounds except the most significant one.
+            The interior ones are included in the array type.  */
+         if (next && (next->kind == cdk_array || next->kind == cdk_pointer))
+           continue;

is another example of code that fails to look past cdk_attrs.

-- 
Joseph S. Myers
joseph@codesourcery.com


More information about the Gcc-patches mailing list