middle-end/101292 - invalid memory access with warning control

  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.

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

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.

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

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

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

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

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

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

On Tue, 18 Jan 2022, Martin Sebor wrote:

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

Huh, that's a quite special case I'm not sure "fixing" will avoid
the mistake.

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

Sure, that's possible I guess.

Richard.