Re: [PATCH] middle-end/101292 - invalid memory access with warning control

[Martin Sebor <msebor@gmail.com>]

On 1/18/22 01:36, Richard Biener wrote:
> On Mon, 17 Jan 2022, Martin Sebor wrote:
> 
>> On 1/17/22 07:32, Richard Biener via Gcc-patches wrote:
>>> The warning control falls into the C++ trap of using a reference
>>> to old hashtable contents for a put operation which can end up
>>> re-allocating that before reading from the old freed referenced to
>>> source.  Fixed by introducing a temporary.
>>
>> I think a better place to fix this and avoid the gotcha once and
>> for all is in the GCC hash_map: C++ containers are expected to
>> handle the insertion of own elements gracefully.
> 
> I don't think that's reasonably possible if you consider
> 
>    T *a = map.get (X);
>    T *b = map.get (Y);
>    map.put (Z, *a);
>    map.put (W, *b);

This case is up to the caller to handle, the same as anything else
involving pointers or references into reallocated storage (it's no
different in C than it is in C++).

The specific case I'm referring to is passing a pointer or reference
to a single element in a container to the first modifying call on
the container.

> 
> the only way to "fix" it would be to change the API to not
> return by reference for get, remove get_or_insert (or change
> its API to also require passing the new value).

No, the fix is to have the modifying function create a copy of
the element being inserted before reallocating the container.

> 
> Note the above shows that making 'put' take the value by
> value instead of by reference doesn't work either.
> 
> IMHO the issue is that C++ doesn't make it obvious that 'put'
> gets a pointer to the old element (stupid references).

The problem isn't specific to references, it can come up with
pointers just as easily.  Pointers might just make it more obvious.

Martin

> 
> Richard.
> 
>> Martin
>>
>>>
>>> Bootstrap & regtest running on x86_64-unknown-linux-gnu.
>>>
>>> 2022-01-17  Richard Biener  <rguenther@suse.de>
>>>
>>>   PR middle-end/101292
>>>   * diagnostic-spec.c (copy_warning): Make sure to not
>>>   reference old hashtable content on possible resize.
>>>   * warning-control.cc (copy_warning): Likewise.
>>> ---
>>>    gcc/diagnostic-spec.c  | 5 ++++-
>>>    gcc/warning-control.cc | 3 ++-
>>>    2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gcc/diagnostic-spec.c b/gcc/diagnostic-spec.c
>>> index a8af229d677..4341ccfaae9 100644
>>> --- a/gcc/diagnostic-spec.c
>>> +++ b/gcc/diagnostic-spec.c
>>> @@ -195,7 +195,10 @@ copy_warning (location_t to, location_t from)
>>>      else
>>>        {
>>>          if (from_spec)
>>> -	nowarn_map->put (to, *from_spec);
>>> +	{
>>> +	  nowarn_spec_t tem = *from_spec;
>>> +	  nowarn_map->put (to, tem);
>>> +	}
>>>           else
>>>     nowarn_map->remove (to);
>>>        }
>>> diff --git a/gcc/warning-control.cc b/gcc/warning-control.cc
>>> index f9808bf4392..fa39ecab421 100644
>>> --- a/gcc/warning-control.cc
>>> +++ b/gcc/warning-control.cc
>>> @@ -206,7 +206,8 @@ void copy_warning (ToType to, FromType from)
>>>       gcc_assert (supp);
>>>    
>>>    	  gcc_checking_assert (nowarn_map);
>>> -	  nowarn_map->put (to_loc, *from_spec);
>>> +	  nowarn_spec_t tem = *from_spec;
>>> +	  nowarn_map->put (to_loc, tem);
>>>     }
>>>           else
>>>     {
>>
>>
>