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

Martin Sebor msebor@gmail.com
Thu Aug 13 23:04:39 GMT 2020


On 8/12/20 5:19 PM, Joseph Myers wrote:
> 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).

Please have a look at the tests at the end of Warrary-parameter-4.c.
They should exercise the cases you brought up (as I understand them).
There are some tests for attributes in Warray-parameter.c.

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

I think I see the problem you're pointing out (I just don't see how
to trigger it or test that it doesn't happen).  If the tweak in
the attached update doesn't fix it a test case would be helpful.

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

Ideally, I'd like to check even pointers to arrays and so they should
be recorded somewhere.  The middle end code doesn't do any checking
of those yet for out-of-bounds accesses.  It wasn't a goal for
the first iteration so I've tweaked the code to avoid recording them.

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

I've added a test for cdk_function to the one for 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.)

I'm not sure I follow you here.  Can you show me what you mean on
a piece of code?  This test case (which IIUC does what you described)
works as expected:

$ cat q.c && gcc -O2 -S -Wall q.c
typedef int A[7][9];

void f (A[3][5]);
void f (A[1][5]);

void g (void)
{
   A a[2][5];
   f (a);
}
q.c:4:9: warning: argument 1 of type ‘int[1][5][7][9]’ with mismatched 
bound [-Warray-parameter=]
     4 | void f (A[1][5]);
       |         ^~~~~~~
q.c:3:9: note: previously declared as ‘int[3][5][7][9]’
     3 | void f (A[3][5]);
       |         ^~~~~~~
q.c: In function ‘g’:
q.c:9:3: warning: ‘f’ accessing 3780 bytes in a region of size 2520 
[-Wstringop-overflow=]
     9 |   f (a);
       |   ^~~~~
q.c:9:3: note: referencing argument 1 of type ‘int (*)[5][7][9]’

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

It should be handled by the tweak I added in the attached revision.

Martin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-50584-2.diff
Type: text/x-patch
Size: 59645 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200813/3d4a5c47/attachment-0001.bin>


More information about the Gcc-patches mailing list