use pointer size rather than array size when storing the former (PR 93829)

The buffer overflow detection for multi-char stores uses the size
of a source array even when what's actually being accessed (read
and stored) is a pointer to the array.  That leads to incorrect
warnings in some cases.

The attached patch corrects the function that computes the size of
the access to set it to that of a pointer instead if the source is
an address expression.

Tested on x86_64-linux.

Martin

On 20 February 2020 01:26:58 CET, Martin Sebor <msebor@gmail.com> wrote:

+   Sets *NULTREM if the representation contains a zero byte, and sets

s/NULTREM/NULTERM/

thanks,

On Wed, 2020-02-19 at 17:26 -0700, Martin Sebor wrote:
> The buffer overflow detection for multi-char stores uses the size
> of a source array even when what's actually being accessed (read
> and stored) is a pointer to the array.  That leads to incorrect
> warnings in some cases.
> 
> The attached patch corrects the function that computes the size of
> the access to set it to that of a pointer instead if the source is
> an address expression.
> 
> Tested on x86_64-linux.

>    if (TREE_CODE (exp) == ADDR_EXPR)
> -    exp = TREE_OPERAND (exp, 0);
> +    {
> +      /* If the size of the access hasn't been determined yet it's that
> +	 of a pointer.  */
> +      if (!nbytes)
> +	nbytes = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (exp)));
> +      exp = TREE_OPERAND (exp, 0);
> +    }
>  
This doesn't make any sense to me.  You're always going to get the size of a
pointer here.  Don't you want the size of the TYPE of the operand?


Jeff

On 2/26/20 3:09 PM, Jeff Law wrote:
> On Wed, 2020-02-19 at 17:26 -0700, Martin Sebor wrote:
>> The buffer overflow detection for multi-char stores uses the size
>> of a source array even when what's actually being accessed (read
>> and stored) is a pointer to the array.  That leads to incorrect
>> warnings in some cases.
>>
>> The attached patch corrects the function that computes the size of
>> the access to set it to that of a pointer instead if the source is
>> an address expression.
>>
>> Tested on x86_64-linux.
> 
>>     if (TREE_CODE (exp) == ADDR_EXPR)
>> -    exp = TREE_OPERAND (exp, 0);
>> +    {
>> +      /* If the size of the access hasn't been determined yet it's that
>> +	 of a pointer.  */
>> +      if (!nbytes)
>> +	nbytes = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (exp)));
>> +      exp = TREE_OPERAND (exp, 0);
>> +    }
>>   
> This doesn't make any sense to me.  You're always going to get the size of a
> pointer here.  Don't you want the size of the TYPE of the operand?

The function correctly handles cases when the expression is an array
because the array is what's being stored.  The bug is in stripping
the ADDR_EXPR and then using the size of the operand when what's
being stored is actually a pointer.

This test case illustrates the difference:

   struct S { char *p, *q, r[8]; } a;

   void f (void)
   {
     struct S b = { 0, "1234567", "abcdefgh" };
     __builtin_memcpy (&a, &b, sizeof a);
   }

The memcpy results in:

   MEM <char *> [(char * {ref-all})&b] = 0B;
   MEM <char *> [(char * {ref-all})&b + 8B] = "1234567";
   MEM <unsigned char[24]> [(char * {ref-all})&a] = MEM <unsigned 
char[24]> [(char * {ref-all})&b];

The RHS of the second MEM_REF is ADDR_EXPR (STRING_CST).  The RHS
of the third MEM_REF is the  array case (that's handled correctly).

I think computing the size of the store could be simplified (it
seems it should be the same as type of either the RHS or the LHS)
but I didn't want to mess with things too much this late.

Martin

On Wed, 2020-02-26 at 16:18 -0700, Martin Sebor wrote:
> On 2/26/20 3:09 PM, Jeff Law wrote:
> > On Wed, 2020-02-19 at 17:26 -0700, Martin Sebor wrote:
> > > The buffer overflow detection for multi-char stores uses the size
> > > of a source array even when what's actually being accessed (read
> > > and stored) is a pointer to the array.  That leads to incorrect
> > > warnings in some cases.
> > > 
> > > The attached patch corrects the function that computes the size of
> > > the access to set it to that of a pointer instead if the source is
> > > an address expression.
> > > 
> > > Tested on x86_64-linux.
> > >     if (TREE_CODE (exp) == ADDR_EXPR)
> > > -    exp = TREE_OPERAND (exp, 0);
> > > +    {
> > > +      /* If the size of the access hasn't been determined yet it's that
> > > +	 of a pointer.  */
> > > +      if (!nbytes)
> > > +	nbytes = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (exp)));
> > > +      exp = TREE_OPERAND (exp, 0);
> > > +    }
> > >   
> > This doesn't make any sense to me.  You're always going to get the size of
> > a
> > pointer here.  Don't you want the size of the TYPE of the operand?
> 
> The function correctly handles cases when the expression is an array
> because the array is what's being stored.  The bug is in stripping
> the ADDR_EXPR and then using the size of the operand when what's
> being stored is actually a pointer.
> 
> This test case illustrates the difference:
> 
>    struct S { char *p, *q, r[8]; } a;
> 
>    void f (void)
>    {
>      struct S b = { 0, "1234567", "abcdefgh" };
>      __builtin_memcpy (&a, &b, sizeof a);
>    }
> 
> The memcpy results in:
> 
>    MEM <char *> [(char * {ref-all})&b] = 0B;
>    MEM <char *> [(char * {ref-all})&b + 8B] = "1234567";
>    MEM <unsigned char[24]> [(char * {ref-all})&a] = MEM <unsigned 
> char[24]> [(char * {ref-all})&b];
> 
> The RHS of the second MEM_REF is ADDR_EXPR (STRING_CST).  The RHS
> of the third MEM_REF is the  array case (that's handled correctly).
> 
> I think computing the size of the store could be simplified (it
> seems it should be the same as type of either the RHS or the LHS)
> but I didn't want to mess with things too much this late.
Sorry, I should have just thrown this under the debugger, but I was short of
time.

ISTM like the problem is we have an RHS like:


> 
> >  <addr_expr 0x7fffea93f360
> >     type <pointer_type 0x7fffea939d20
> >         type <array_type 0x7fffea939690 type <integer_type 0x7fffea8193f0
> > char>
> >             BLK
> >             size <integer_cst 0x7fffea81e3a8 constant 328>
> >             unit-size <integer_cst 0x7fffea935f48 constant 41>
> >             align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> > 0x7fffea939690 domain <integer_type 0x7fffea9395e8>
> >             pointer_to_this <pointer_type 0x7fffea939d20>>
> >         unsigned DI
> >         size <integer_cst 0x7fffea800cd8 constant 64>
> >         unit-size <integer_cst 0x7fffea800cf0 constant 8>
> >         align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> > 0x7fffea939d20>
> >     readonly constant
> >     arg:0 <string_cst 0x7fffea814100 type <array_type 0x7fffea939690>
> >         readonly constant static
> > "0123456789012345678901234567890123456789\000">
> >     j.c:20:10 start: j.c:20:10 finish: j.c:20:10>
> > 

The current code unconditionally strips the ADDR_EXPR at this point and as a
result will use the length of STRING_CST node, which is obviously wrong.

With your change, if we haven't already determined a length, we extract the
length of the pointer then strip the ADDR_EXPR.

Yea, this is all ripe for some cleanup.  For example it's not at all clear that
just because we don't have a size at this point in count_nonzero_bytes and
we've got an ADDR_EXPR that we want the size of the pointer.    That may in
fact be true, but it's not easy to ascertain with the current spaghetti code.

OK for the trunk, but let's queue this area for some cleanup in gcc-11.

jeff






> Martin
>