Pointer Bounds Checker and trailing arrays (PR68270)

2016-11-26 0:28 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
> 2016-11-25 15:47 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>> Hi,
>>
>> The patch below addresses PR68270. could you please take a look?
>>
>> 2016-11-25  Alexander Ivchenko  <aivchenk@gmail.com>
>>
>>        * c-family/c.opt (flag_chkp_flexible_struct_trailing_arrays):
>>        Add new option.
>>        * tree-chkp.c (chkp_parse_array_and_component_ref): Forbid
>>        narrowing when chkp_parse_array_and_component_ref is used and
>>        the ARRAY_REF points to an array in the end of the struct.
>>
>>
>>
>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
>> index 7d8a726..e45d6a2 100644
>> --- a/gcc/c-family/c.opt
>> +++ b/gcc/c-family/c.opt
>> @@ -1166,6 +1166,11 @@ C ObjC C++ ObjC++ LTO RejectNegative Report
>> Var(flag_chkp_narrow_to_innermost_ar
>>  Forces Pointer Bounds Checker to use bounds of the innermost arrays in case of
>>  nested static arryas access.  By default outermost array is used.
>>
>> +fchkp-flexible-struct-trailing-arrays
>> +C ObjC C++ ObjC++ LTO RejectNegative Report
>> Var(flag_chkp_flexible_struct_trailing_arrays)
>> +Allow Pointer Bounds Checker to treat all trailing arrays in structures as
>> +possibly flexible.
>
> Words 'allow' and 'possibly' are confusing here. This option is about to force
> checker to do something, not to give him a choice.

Fixed

> New option has to be documented in invoke.texi. It would also be nice to reflect
> changes on GCC MPX wiki page.

Done
> We have an attribute to change compiler behavior when this option is not set.
> But we have no way to make exceptions when this option is used. Should we
> add one?
Something like "bnd_fixed_size" ? Could work. Although the customer
request did not mention the need for that.
Can I add it in a separate patch?


>> +
>>  fchkp-optimize
>>  C ObjC C++ ObjC++ LTO Report Var(flag_chkp_optimize) Init(-1)
>>  Allow Pointer Bounds Checker optimizations.  By default allowed
>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>> index 2769682..40f99c3 100644
>> --- a/gcc/tree-chkp.c
>> +++ b/gcc/tree-chkp.c
>> @@ -3425,7 +3425,9 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
>>    if (flag_chkp_narrow_bounds
>>        && !flag_chkp_narrow_to_innermost_arrray
>>        && (!last_comp
>> -  || chkp_may_narrow_to_field (TREE_OPERAND (last_comp, 1))))
>> +  || (chkp_may_narrow_to_field (TREE_OPERAND (last_comp, 1))
>> +      && !(flag_chkp_flexible_struct_trailing_arrays
>> +   && array_at_struct_end_p (var)))))
>
> This is incorrect place for fix. Consider code
>
> struct S {
>   int a;
>   int b[10];
> };
>
> struct S s;
> int *p = s.b;
>
> Here you need to compute bounds for p and you want your option to take effect
> but in this case you won't event reach your new check because there is no
> ARRAY_REF. And even if we change it to
>
> int *p = s.b[5];
>
> then it still would be narrowed because s.b would still be written
> into 'comp_to_narrow'
> variable. Correct place for fix is in chkp_may_narrow_to_field.

Done

> Also you should consider fchkp-narrow-to-innermost-arrray option. Should it
> be more powerfull or not? I think fchkp-narrow-to-innermost-arrray shouldn't
> narrow to variable sized fields. BTW looks like right now bnd_variable_size
> attribute is ignored by fchkp-narrow-to-innermost-arrray. This is another
> problem and may be fixed in another patch though.
The way code works in chkp_parse_array_and_component_ref seems to be
exactly like you say:  fchkp-narrow-to-innermost-arrray won't narrow
to variable sized fields. I will create a separate bug for
bnd_variable_size+ fchkp-narrow-to-innermost-arrray.

> Also patch lacks tests for various situations (with option and without, with
> ARRAY_REF and without). In case of new attribute and fix for
> fchkp-narrow-to-innermost-arrray behavior additional tests are required.
I added three tests for flag_chkp_flexible_struct_trailing_arrays



The patch is below:

2016-12-20 17:44 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
> 2016-11-26 0:28 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>> 2016-11-25 15:47 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
>>> Hi,
>>>
>>> The patch below addresses PR68270. could you please take a look?
>>>
>>> 2016-11-25  Alexander Ivchenko  <aivchenk@gmail.com>
>>>
>>>        * c-family/c.opt (flag_chkp_flexible_struct_trailing_arrays):
>>>        Add new option.
>>>        * tree-chkp.c (chkp_parse_array_and_component_ref): Forbid
>>>        narrowing when chkp_parse_array_and_component_ref is used and
>>>        the ARRAY_REF points to an array in the end of the struct.
>>>
>>>
>>>
>>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
>>> index 7d8a726..e45d6a2 100644
>>> --- a/gcc/c-family/c.opt
>>> +++ b/gcc/c-family/c.opt
>>> @@ -1166,6 +1166,11 @@ C ObjC C++ ObjC++ LTO RejectNegative Report
>>> Var(flag_chkp_narrow_to_innermost_ar
>>>  Forces Pointer Bounds Checker to use bounds of the innermost arrays in case of
>>>  nested static arryas access.  By default outermost array is used.
>>>
>>> +fchkp-flexible-struct-trailing-arrays
>>> +C ObjC C++ ObjC++ LTO RejectNegative Report
>>> Var(flag_chkp_flexible_struct_trailing_arrays)
>>> +Allow Pointer Bounds Checker to treat all trailing arrays in structures as
>>> +possibly flexible.
>>
>> Words 'allow' and 'possibly' are confusing here. This option is about to force
>> checker to do something, not to give him a choice.
>
> Fixed
>
>> New option has to be documented in invoke.texi. It would also be nice to reflect
>> changes on GCC MPX wiki page.
>
> Done
>> We have an attribute to change compiler behavior when this option is not set.
>> But we have no way to make exceptions when this option is used. Should we
>> add one?
> Something like "bnd_fixed_size" ? Could work. Although the customer
> request did not mention the need for that.
> Can I add it in a separate patch?
>

Yes.

>
>>> +
>>>  fchkp-optimize
>>>  C ObjC C++ ObjC++ LTO Report Var(flag_chkp_optimize) Init(-1)
>>>  Allow Pointer Bounds Checker optimizations.  By default allowed
>>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
>>> index 2769682..40f99c3 100644
>>> --- a/gcc/tree-chkp.c
>>> +++ b/gcc/tree-chkp.c
>>> @@ -3425,7 +3425,9 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
>>>    if (flag_chkp_narrow_bounds
>>>        && !flag_chkp_narrow_to_innermost_arrray
>>>        && (!last_comp
>>> -  || chkp_may_narrow_to_field (TREE_OPERAND (last_comp, 1))))
>>> +  || (chkp_may_narrow_to_field (TREE_OPERAND (last_comp, 1))
>>> +      && !(flag_chkp_flexible_struct_trailing_arrays
>>> +   && array_at_struct_end_p (var)))))
>>
>> This is incorrect place for fix. Consider code
>>
>> struct S {
>>   int a;
>>   int b[10];
>> };
>>
>> struct S s;
>> int *p = s.b;
>>
>> Here you need to compute bounds for p and you want your option to take effect
>> but in this case you won't event reach your new check because there is no
>> ARRAY_REF. And even if we change it to
>>
>> int *p = s.b[5];
>>
>> then it still would be narrowed because s.b would still be written
>> into 'comp_to_narrow'
>> variable. Correct place for fix is in chkp_may_narrow_to_field.
>
> Done
>
>> Also you should consider fchkp-narrow-to-innermost-arrray option. Should it
>> be more powerfull or not? I think fchkp-narrow-to-innermost-arrray shouldn't
>> narrow to variable sized fields. BTW looks like right now bnd_variable_size
>> attribute is ignored by fchkp-narrow-to-innermost-arrray. This is another
>> problem and may be fixed in another patch though.
> The way code works in chkp_parse_array_and_component_ref seems to be
> exactly like you say:  fchkp-narrow-to-innermost-arrray won't narrow
> to variable sized fields. I will create a separate bug for
> bnd_variable_size+ fchkp-narrow-to-innermost-arrray.
>
>> Also patch lacks tests for various situations (with option and without, with
>> ARRAY_REF and without). In case of new attribute and fix for
>> fchkp-narrow-to-innermost-arrray behavior additional tests are required.
> I added three tests for flag_chkp_flexible_struct_trailing_arrays
>
>
>
> The patch is below:
>
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index 2d47d54..21ad6aa 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -1188,7 +1188,13 @@ narrowing is on, field bounds are used.
> Otherwise full object bounds are used.
>  fchkp-narrow-to-innermost-array
>  C ObjC C++ ObjC++ LTO RejectNegative Report
> Var(flag_chkp_narrow_to_innermost_arrray)
>  Forces Pointer Bounds Checker to use bounds of the innermost arrays in case of
> -nested static arryas access.  By default outermost array is used.
> +nested static arrays access.  By default outermost array is used.
> +
> +fchkp-flexible-struct-trailing-arrays
> +C ObjC C++ ObjC++ LTO RejectNegative Report
> Var(flag_chkp_flexible_struct_trailing_arrays)
> +Forces Pointer Bounds Checker to treat all trailing arrays in structures as
> +possibly flexible.  By default only arrays fields with zero length or that are
> +marked with attribute bnd_variable_size are treated as flexible.
>
>  fchkp-optimize
>  C ObjC C++ ObjC++ LTO Report Var(flag_chkp_optimize) Init(-1)
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 034ae98..2372c22a 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -10886,6 +10886,13 @@ Forces Pointer Bounds Checker to use narrowed
> bounds for the address of the
>  first field in the structure.  By default a pointer to the first field has
>  the same bounds as a pointer to the whole structure.
>
> +@item -fchkp-flexible-struct-trailing-arrays
> +@opindex fchkp-flexible-struct-trailing-arrays
> +@opindex fno-chkp-flexible-struct-trailing-arrays
> +Forces Pointer Bounds Checker to treat all trailing arrays in structures as
> +possibly flexible.  By default only arrays fields with zero length or that are
> +marked with attribute bnd_variable_size are treated as flexible.
> +
>  @item -fchkp-narrow-to-innermost-array
>  @opindex fchkp-narrow-to-innermost-array
>  @opindex fno-chkp-narrow-to-innermost-array
> diff --git a/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-lbv.c
> b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-lbv.c
> new file mode 100644
> index 0000000..9739920
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-lbv.c
> @@ -0,0 +1,29 @@
> +/* { dg-do run } */
> +/* { dg-shouldfail "bounds violation" } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx
> -fchkp-flexible-struct-trailing-arrays" } */
> +
> +
> +#define SHOULDFAIL
> +
> +#include "mpx-check.h"
> +
> +struct S
> +{
> +  int a;
> +  int p[10];
> +};
> +
> +int rd (int *p, int i)
> +{
> +  int res = p[i];
> +  printf ("%d\n", res);
> +  return res;
> +}
> +
> +int mpx_test (int argc, const char **argv)
> +{
> +  struct S *s = (struct S *)alloca (sizeof(struct S) + sizeof (int)*100);
> +  rd (s->p, -2);
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-nov.c
> b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-nov.c
> new file mode 100644
> index 0000000..f5c8f95
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-nov.c
> @@ -0,0 +1,29 @@
> +/* { dg-do run } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx
> -fchkp-flexible-struct-trailing-arrays" } */
> +
> +
> +#include "mpx-check.h"
> +
> +struct S
> +{
> +  int a;
> +  int p[10];
> +};
> +
> +int rd (int *p, int i)
> +{
> +  int res = p[i];
> +  printf ("%d\n", res);
> +  return res;
> +}
> +
> +int mpx_test (int argc, const char **argv)
> +{
> +  struct S *s = (struct S *)alloca (sizeof(struct S) + sizeof (int)*100);
> +  rd (s->p, 0);
> +  rd (s->p, 99);
> +  s->p[0];
> +  s->p[99];
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-ubv.c
> b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-ubv.c
> new file mode 100644
> index 0000000..8385a5a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/mpx/vla-trailing-1-ubv.c
> @@ -0,0 +1,29 @@
> +/* { dg-do run } */
> +/* { dg-shouldfail "bounds violation" } */
> +/* { dg-options "-fcheck-pointer-bounds -mmpx
> -fchkp-flexible-struct-trailing-arrays" } */
> +
> +
> +#define SHOULDFAIL
> +
> +#include "mpx-check.h"
> +
> +struct S
> +{
> +  int a;
> +  int p[10];
> +};
> +
> +int rd (int *p, int i)
> +{
> +  int res = p[i];
> +  printf ("%d\n", res);
> +  return res;
> +}
> +
> +int mpx_test (int argc, const char **argv)
> +{
> +  struct S *s = (struct S *)alloca (sizeof(struct S) + sizeof (int)*100);
> +  rd (s->p, 110);
> +
> +  return 0;
> +}
> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
> index 2769682..6c5e541 100644
> --- a/gcc/tree-chkp.c
> +++ b/gcc/tree-chkp.c
> @@ -3272,6 +3272,8 @@ chkp_may_narrow_to_field (tree field)
>  {
>    return DECL_SIZE (field) && TREE_CODE (DECL_SIZE (field)) == INTEGER_CST
>      && tree_to_uhwi (DECL_SIZE (field)) != 0
> +    && (!flag_chkp_flexible_struct_trailing_arrays
> + || DECL_CHAIN (field))

What if it's not array?

Thanks,
Ilya

>      && (!DECL_FIELD_OFFSET (field)
>   || TREE_CODE (DECL_FIELD_OFFSET (field)) == INTEGER_CST)
>      && (!DECL_FIELD_BIT_OFFSET (field)