diff mbox series

avoid -Wuse-after-free [BZ #26779]

Message ID c9f4a1b4-193e-986c-bc21-5866b9e62e16@gmail.com
State New
Headers show
Series avoid -Wuse-after-free [BZ #26779] | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Martin Sebor Jan. 16, 2022, 12:21 a.m. UTC
GCC 12 features a couple of new warnings designed to detect uses
of pointers made invalid by the pointees lifetimes having ended.
Building Glibc with the enhanced GCC exposes a few such uses,
mostly after successful calls to realloc.  The attached patch
avoids the new warnings by converting the pointers to uintptr_t
first and using the converted integers instead.

The patch suppresses all instances of the warning at the strictest
setting (-Wuse-after-free=3), which includes even uses in equality
expressions.  The default setting approved for GCC 12 is
-Wuse-after-free=2, which doesn't warn on such uses to accommodate
the pointer-adjustment-after-realloc idiom.  At the default setting,
the changes to ldconfig.c and setenv are not necessary.

Martin

Comments

Paul Eggert Jan. 16, 2022, 2:25 a.m. UTC | #1
On 1/15/22 16:21, Martin Sebor via Libc-alpha wrote:

> +		      intptr_t ip_new_pool = (intptr_t)new_pool;
> +		      intptr_t ptr_diff = ip_new_pool - ip_string_space;

This sort of thing can have undefined behavior due to signed integer 
overflow. To avoid that problem on glibc platforms, you can use 
uintptr_t instead of intptr_t.

> -			      map[i].alias += new_pool - string_space;
> -			      map[i].value += new_pool - string_space;
> +			      map[i].alias += ptr_diff;
> +			      map[i].value += ptr_diff;

Although this might pacify GCC 12, it continues to have the undefined 
behavior that GCC is evidently warning about, since the old values of 
map[i].alias and map[i].value have been freed and this means programs 
cannot use those pointers even if only to add something to them. It's 
conceivable that future GCC versions will figure this out and generate 
code that doesn't do what we want here.

One simple workaround would be for the .alias and .value components to 
be offsets into the storage pool, instead of being absolute pointers.

> --- a/stdlib/setenv.c
> +++ b/stdlib/setenv.c
> @@ -150,7 +150,9 @@ __add_to_environ (const char *name, const char *value, const char *combined,
>      {
>        char **new_environ;
>  
> -      /* We allocated this space; we can extend it.  */
> +      /* We allocated this space; we can extend it.  Avoid using the raw
> +	 reallocated pointer to avoid GCC -Wuse-after-free.  */
> +      uintptr_t ip_last_environ = (uintptr_t)last_environ;
>        new_environ = (char **) realloc (last_environ,
>  				       (size + 2) * sizeof (char *));
>        if (new_environ == NULL)

Cleaner would be to leave the old code alone, except to add this before 
reallocating:

       bool we_allocated_environ = __environ == last_environ;

 > ...
> -      if (__environ != last_environ)
> +      if ((uintptr_t)__environ != ip_last_environ)

And then change the above line to "if (! we_allocated_environ)".

>  	memcpy ((char *) new_environ, (char *) __environ,
>  		size * sizeof (char *));
>
Florian Weimer Jan. 18, 2022, 9:48 a.m. UTC | #2
* Martin Sebor via Libc-alpha:

> GCC 12 features a couple of new warnings designed to detect uses
> of pointers made invalid by the pointees lifetimes having ended.
> Building Glibc with the enhanced GCC exposes a few such uses,
> mostly after successful calls to realloc.  The attached patch
> avoids the new warnings by converting the pointers to uintptr_t
> first and using the converted integers instead.
>
> The patch suppresses all instances of the warning at the strictest
> setting (-Wuse-after-free=3), which includes even uses in equality
> expressions.  The default setting approved for GCC 12 is
> -Wuse-after-free=2, which doesn't warn on such uses to accommodate
> the pointer-adjustment-after-realloc idiom.  At the default setting,
> the changes to ldconfig.c and setenv are not necessary.

Would you be able to split up this patch further?  It will help with
eventually backporting parts of it.

Thanks,
Florian
Martin Sebor Jan. 20, 2022, 9:50 p.m. UTC | #3
On 1/18/22 02:48, Florian Weimer wrote:
> * Martin Sebor via Libc-alpha:
> 
>> GCC 12 features a couple of new warnings designed to detect uses
>> of pointers made invalid by the pointees lifetimes having ended.
>> Building Glibc with the enhanced GCC exposes a few such uses,
>> mostly after successful calls to realloc.  The attached patch
>> avoids the new warnings by converting the pointers to uintptr_t
>> first and using the converted integers instead.
>>
>> The patch suppresses all instances of the warning at the strictest
>> setting (-Wuse-after-free=3), which includes even uses in equality
>> expressions.  The default setting approved for GCC 12 is
>> -Wuse-after-free=2, which doesn't warn on such uses to accommodate
>> the pointer-adjustment-after-realloc idiom.  At the default setting,
>> the changes to ldconfig.c and setenv are not necessary.
> 
> Would you be able to split up this patch further?  It will help with
> eventually backporting parts of it.

Sure.  Let me fix the issues Paul pointed out and post a diff for
each file separately.

Martin

> 
> Thanks,
> Florian
>
Martin Sebor Jan. 21, 2022, 11:14 p.m. UTC | #4
On 1/15/22 19:25, Paul Eggert wrote:
> On 1/15/22 16:21, Martin Sebor via Libc-alpha wrote:
> 
>> +              intptr_t ip_new_pool = (intptr_t)new_pool;
>> +              intptr_t ptr_diff = ip_new_pool - ip_string_space;
> 
> This sort of thing can have undefined behavior due to signed integer 
> overflow. To avoid that problem on glibc platforms, you can use 
> uintptr_t instead of intptr_t.

Good point.  But then the pointer addition in

     map[i].alias += ptr_diff;

might overflow (that's why I used the signed intptr_t).  Ugh.

> 
>> -                  map[i].alias += new_pool - string_space;
>> -                  map[i].value += new_pool - string_space;
>> +                  map[i].alias += ptr_diff;
>> +                  map[i].value += ptr_diff;
> 
> Although this might pacify GCC 12, it continues to have the undefined 
> behavior that GCC is evidently warning about, since the old values of 
> map[i].alias and map[i].value have been freed and this means programs 
> cannot use those pointers even if only to add something to them. It's 
> conceivable that future GCC versions will figure this out and generate 
> code that doesn't do what we want here.

I wondered if they might point into the allocated block.  Ugh again.

> 
> One simple workaround would be for the .alias and .value components to 
> be offsets into the storage pool, instead of being absolute pointers.

Right, that's also the recommendation mentioned in the GCC manual.
But it seems like a bigger change than I have the time to make.

So I think I'll leave this mess to someone else and simply suppress
the warning for this code until then.

> 
>> --- a/stdlib/setenv.c
>> +++ b/stdlib/setenv.c
>> @@ -150,7 +150,9 @@ __add_to_environ (const char *name, const char 
>> *value, const char *combined,
>>      {
>>        char **new_environ;
>>
>> -      /* We allocated this space; we can extend it.  */
>> +      /* We allocated this space; we can extend it.  Avoid using the raw
>> +     reallocated pointer to avoid GCC -Wuse-after-free.  */
>> +      uintptr_t ip_last_environ = (uintptr_t)last_environ;
>>        new_environ = (char **) realloc (last_environ,
>>                         (size + 2) * sizeof (char *));
>>        if (new_environ == NULL)
> 
> Cleaner would be to leave the old code alone, except to add this before 
> reallocating:
> 
>        bool we_allocated_environ = __environ == last_environ;
> 
>  > ...
>> -      if (__environ != last_environ)
>> +      if ((uintptr_t)__environ != ip_last_environ)
> 
> And then change the above line to "if (! we_allocated_environ)".

That does look cleaner although it wasn't entirely obvious to me
from looking at the code that it's the same.  Unfortunately, it
doesn't help.  GCC replaces the bool variable with the equality
test of the two pointers, and the warning points that out.
Pedantically speaking it's a bug in GCC that it does that but
I doubt anyone would care to do anything about it.  This only
happens at level 3 (i.e., above the default 2), so if you want
a clean build at that level you can either take the patch as is
or use #pragma GCC diagnostic to suppress the warning.  Otherwise,
if all you care about is -Wall, then you can drop this part of
the patch.

Let me post an updated version with the changes broken up by file
as Florian requested.

Thanks for your comments!
Martin

> 
>>      memcpy ((char *) new_environ, (char *) __environ,
>>          size * sizeof (char *));
>>
Paul Eggert Jan. 22, 2022, 12:42 a.m. UTC | #5
On 1/21/22 15:14, Martin Sebor wrote:
>>
> 
> That does look cleaner although it wasn't entirely obvious to me
> from looking at the code that it's the same.  Unfortunately, it
> doesn't help.  GCC replaces the bool variable with the equality
> test of the two pointers, and the warning points that out.

That's a GCC bug.

> Pedantically speaking it's a bug in GCC that it does that but
> I doubt anyone would care to do anything about it.

You might be surprised; they do fix bugs in this area. If I could 
reproduce the bug I'd file a bug report, but my GCC doesn't complain so 
I guess it's up to you....


> his only
> happens at level 3 (i.e., above the default 2), so if you want
> a clean build at that level you can either take the patch as is
> or use #pragma GCC diagnostic to suppress the warning.  Otherwise,
> if all you care about is -Wall, then you can drop this part of
> the patch.

We shouldn't drop the patch or use a pragma, since this is a genuine bug 
in glibc that should get fixed. If the only option is to use the patch 
as-is then let's do that. Though I do wish the GCC bug were fixed.
Martin Sebor Jan. 25, 2022, 12:42 a.m. UTC | #6
On 1/21/22 17:42, Paul Eggert wrote:
> On 1/21/22 15:14, Martin Sebor wrote:
>>>
>>
>> That does look cleaner although it wasn't entirely obvious to me
>> from looking at the code that it's the same.  Unfortunately, it
>> doesn't help.  GCC replaces the bool variable with the equality
>> test of the two pointers, and the warning points that out.
> 
> That's a GCC bug.
> 
>> Pedantically speaking it's a bug in GCC that it does that but
>> I doubt anyone would care to do anything about it.
> 
> You might be surprised; they do fix bugs in this area. If I could 
> reproduce the bug I'd file a bug report, but my GCC doesn't complain so 
> I guess it's up to you....

I opened a GCC bug for the record:
   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104215
GCC 12 is now in its regression-fixing stage so to get it fixed we'd
need to make it a regression.  That's usually only done for bugs that
trigger under conditions where they didn't before.

>> his only
>> happens at level 3 (i.e., above the default 2), so if you want
>> a clean build at that level you can either take the patch as is
>> or use #pragma GCC diagnostic to suppress the warning.  Otherwise,
>> if all you care about is -Wall, then you can drop this part of
>> the patch.
> 
> We shouldn't drop the patch or use a pragma, since this is a genuine bug 
> in glibc that should get fixed. If the only option is to use the patch 
> as-is then let's do that. Though I do wish the GCC bug were fixed.

As do I, but I'm not holding my breath.

I will post an updated/broken down patch series shortly.

Martin
Jeff Law Jan. 25, 2022, 1:08 a.m. UTC | #7
On 1/24/2022 5:42 PM, Martin Sebor via Libc-alpha wrote:
> On 1/21/22 17:42, Paul Eggert wrote:
>> On 1/21/22 15:14, Martin Sebor wrote:
>>>>
>>>
>>> That does look cleaner although it wasn't entirely obvious to me
>>> from looking at the code that it's the same.  Unfortunately, it
>>> doesn't help.  GCC replaces the bool variable with the equality
>>> test of the two pointers, and the warning points that out.
>>
>> That's a GCC bug.
>>
>>> Pedantically speaking it's a bug in GCC that it does that but
>>> I doubt anyone would care to do anything about it.
>>
>> You might be surprised; they do fix bugs in this area. If I could 
>> reproduce the bug I'd file a bug report, but my GCC doesn't complain 
>> so I guess it's up to you....
>
> I opened a GCC bug for the record:
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104215
> GCC 12 is now in its regression-fixing stage so to get it fixed we'd
> need to make it a regression.  That's usually only done for bugs that
> trigger under conditions where they didn't before.
The problem is there's no way for the compiler to know that it can't 
move the use of q past the realloc call -- there's nothing from a 
dataflow standpoint which would prevent such movement.

In some ways the builtin_realloc call would need to say "I'm going to 
clobber this argument" and it would have to do so before translating 
into SSA form I think.

Jeff
diff mbox series

Patch

diff --git a/elf/ldconfig.c b/elf/ldconfig.c
index d14633f5ec..57bb95ebc3 100644
--- a/elf/ldconfig.c
+++ b/elf/ldconfig.c
@@ -735,9 +735,9 @@  manual_link (char *library)
   create_links (real_path, path, libname, soname);
   free (soname);
 out:
-  free (path);
   if (path != real_path)
     free (real_path);
+  free (path);
 }
 
 
diff --git a/intl/localealias.c b/intl/localealias.c
index 3ae360f40d..e581ee4346 100644
--- a/intl/localealias.c
+++ b/intl/localealias.c
@@ -318,7 +318,9 @@  read_alias_file (const char *fname, int fname_len)
 
 		  if (string_space_act + alias_len + value_len > string_space_max)
 		    {
-		      /* Increase size of memory pool.  */
+		      /* Increase size of memory pool.  Avoid using the raw
+			 reallocated pointer to avoid GCC -Wuse-after-free.  */
+		      intptr_t ip_string_space = (intptr_t)string_space;
 		      size_t new_size = (string_space_max
 					 + (alias_len + value_len > 1024
 					    ? alias_len + value_len : 1024));
@@ -326,14 +328,16 @@  read_alias_file (const char *fname, int fname_len)
 		      if (new_pool == NULL)
 			goto out;
 
-		      if (__builtin_expect (string_space != new_pool, 0))
+		      intptr_t ip_new_pool = (intptr_t)new_pool;
+		      intptr_t ptr_diff = ip_new_pool - ip_string_space;
+		      if (__builtin_expect (ptr_diff == 0, 0))
 			{
 			  size_t i;
 
 			  for (i = 0; i < nmap; i++)
 			    {
-			      map[i].alias += new_pool - string_space;
-			      map[i].value += new_pool - string_space;
+			      map[i].alias += ptr_diff;
+			      map[i].value += ptr_diff;
 			    }
 			}
 
diff --git a/io/ftw.c b/io/ftw.c
index 2742541f36..08ccbdd523 100644
--- a/io/ftw.c
+++ b/io/ftw.c
@@ -323,8 +323,8 @@  open_dir_stream (int *dfdp, struct ftw_data *data, struct dir_data *dirp)
 	  buf[actsize++] = '\0';
 
 	  /* Shrink the buffer to what we actually need.  */
-	  data->dirstreams[data->actdir]->content = realloc (buf, actsize);
-	  if (data->dirstreams[data->actdir]->content == NULL)
+	  void *content = realloc (buf, actsize);
+	  if (content == NULL)
 	    {
 	      int save_err = errno;
 	      free (buf);
@@ -338,6 +338,7 @@  open_dir_stream (int *dfdp, struct ftw_data *data, struct dir_data *dirp)
 	      data->dirstreams[data->actdir]->streamfd = -1;
 	      data->dirstreams[data->actdir] = NULL;
 	    }
+	  data->dirstreams[data->actdir]->content = content;
 	}
     }
 
diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index c3d2cee7b6..2176cbac31 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -150,7 +150,9 @@  __add_to_environ (const char *name, const char *value, const char *combined,
     {
       char **new_environ;
 
-      /* We allocated this space; we can extend it.  */
+      /* We allocated this space; we can extend it.  Avoid using the raw
+	 reallocated pointer to avoid GCC -Wuse-after-free.  */
+      uintptr_t ip_last_environ = (uintptr_t)last_environ;
       new_environ = (char **) realloc (last_environ,
 				       (size + 2) * sizeof (char *));
       if (new_environ == NULL)
@@ -159,7 +161,7 @@  __add_to_environ (const char *name, const char *value, const char *combined,
 	  return -1;
 	}
 
-      if (__environ != last_environ)
+      if ((uintptr_t)__environ != ip_last_environ)
 	memcpy ((char *) new_environ, (char *) __environ,
 		size * sizeof (char *));