diff mbox series

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

Message ID 7pq3onsr-r66r-s43o-7p4-9n5o5672q1q@fhfr.qr
State Committed
Commit 1374d4b963a6ac2e0ec1645c09e5162e68b009d6
Headers show
Series middle-end/101292 - invalid memory access with warning control | expand

Commit Message

Richard Biener Jan. 17, 2022, 2:32 p.m. UTC
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(-)

Comments

Martin Sebor Jan. 17, 2022, 6:12 p.m. UTC | #1
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
>   	{
Richard Biener Jan. 18, 2022, 8:36 a.m. UTC | #2
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
> >    {
> 
>
Martin Sebor Jan. 18, 2022, 4:22 p.m. UTC | #3
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
>>>     {
>>
>>
>
Richard Biener Jan. 19, 2022, 7:22 a.m. UTC | #4
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.
diff mbox series

Patch

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
 	{