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

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

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

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

[-- Attachment #2: glibc-26779.diff --]
[-- Type: text/x-patch, Size: 3316 bytes --]

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

Re: [PATCH] avoid -Wuse-after-free [BZ #26779]

[Paul Eggert]

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

Re: [PATCH] avoid -Wuse-after-free [BZ #26779]

[Florian Weimer]

* 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

Re: [PATCH] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

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
> 

Re: [PATCH] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

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

Re: [PATCH] avoid -Wuse-after-free [BZ #26779]

[Paul Eggert]

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.

Re: [PATCH] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

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

[PATCH v2 0/5] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

This is a repost of the original patch but broken down by source
file and with some suppression done by #pragma GCC diagnostic
instead of conversion to intptr_t.  It also adds fixes for
the same problem in the test suite that I overlooked before.

On 1/15/22 17:21, Martin Sebor wrote:
> 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

[PATCH v2 1/5] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

On 1/24/22 17:52, Martin Sebor wrote:
> This is a repost of the original patch but broken down by source
> file and with some suppression done by #pragma GCC diagnostic
> instead of conversion to intptr_t.  It also adds fixes for
> the same problem in the test suite that I overlooked before.

The attached patch suppresses the -Wuse-after-free instance in
elf/ldconfig.c.

> 
> On 1/15/22 17:21, Martin Sebor wrote:
>> 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
> 

[-- Attachment #2: glibc-26779-1.diff --]
[-- Type: text/x-patch, Size: 335 bytes --]

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

Re: [PATCH v2 2/5] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

On 1/24/22 17:52, Martin Sebor wrote:
> This is a repost of the original patch but broken down by source
> file and with some suppression done by #pragma GCC diagnostic
> instead of conversion to intptr_t.  It also adds fixes for
> the same problem in the test suite that I overlooked before.

The attached patch suppresses the -Wuse-after-free instance in
intl/localealias.c.

> 
> On 1/15/22 17:21, Martin Sebor wrote:
>> 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
> 

[-- Attachment #2: glibc-26779-2.diff --]
[-- Type: text/x-patch, Size: 975 bytes --]

diff --git a/intl/localealias.c b/intl/localealias.c
index 3ae360f40d..b36092363a 100644
--- a/intl/localealias.c
+++ b/intl/localealias.c
@@ -318,7 +318,15 @@ 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.  */
+#pragma GCC diagnostic push
+
+#if defined __GNUC__ && __GNUC__ >= 12
+  /* Suppress the valid GCC 12 warning until the code below is changed
+     to avoid using pointers to the reallocated block.  */
+#  pragma GCC diagnostic ignored "-Wuse-after-free"
+#endif
+
+		    /* Increase size of memory pool.  */
 		      size_t new_size = (string_space_max
 					 + (alias_len + value_len > 1024
 					    ? alias_len + value_len : 1024));
@@ -351,6 +359,8 @@ read_alias_file (const char *fname, int fname_len)
 					   value, value_len);
 		  string_space_act += value_len;
 
+#pragma GCC diagnostic pop
+
 		  ++nmap;
 		  ++added;
 		}

Re: [PATCH v2 3/5] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

On 1/24/22 17:52, Martin Sebor wrote:
> This is a repost of the original patch but broken down by source
> file and with some suppression done by #pragma GCC diagnostic
> instead of conversion to intptr_t.  It also adds fixes for
> the same problem in the test suite that I overlooked before.

The attached patch suppresses the -Wuse-after-free instance in
io/ftw.c.

> 
> On 1/15/22 17:21, Martin Sebor wrote:
>> 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
> 

[-- Attachment #2: glibc-26779-3.diff --]
[-- Type: text/x-patch, Size: 803 bytes --]

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

Re: [PATCH v2 4/5] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]

On 1/24/22 17:52, Martin Sebor wrote:
> This is a repost of the original patch but broken down by source
> file and with some suppression done by #pragma GCC diagnostic
> instead of conversion to intptr_t.  It also adds fixes for
> the same problem in the test suite that I overlooked before.

The attached patch suppresses the -Wuse-after-free instance in
stdlib/setenv.c.

> 
> On 1/15/22 17:21, Martin Sebor wrote:
>> 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
> 

[-- Attachment #2: glibc-26779-4.diff --]
[-- Type: text/x-patch, Size: 928 bytes --]

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

Re: [PATCH v2 5/5] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

On 1/24/22 17:52, Martin Sebor wrote:
> This is a repost of the original patch but broken down by source
> file and with some suppression done by #pragma GCC diagnostic
> instead of conversion to intptr_t.  It also adds fixes for
> the same problem in the test suite that I overlooked before.

The attached patch suppresses the -Wuse-after-free instance in
the testsuite.

> 
> On 1/15/22 17:21, Martin Sebor wrote:
>> 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
> 

[-- Attachment #2: glibc-26779-5.diff --]
[-- Type: text/x-patch, Size: 4528 bytes --]

diff --git a/malloc/tst-malloc-backtrace.c b/malloc/tst-malloc-backtrace.c
index ea66da23ef..8a3f4a0b55 100644
--- a/malloc/tst-malloc-backtrace.c
+++ b/malloc/tst-malloc-backtrace.c
@@ -20,6 +20,7 @@
 #include <stdlib.h>
 
 #include <support/support.h>
+#include <libc-diag.h>
 
 #define SIZE 4096
 
@@ -29,7 +30,15 @@ __attribute__((noinline))
 call_free (void *ptr)
 {
   free (ptr);
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a valid warning about using a pointer made indeterminate
+     by a prior call to malloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
   *(size_t *)(ptr - sizeof (size_t)) = 1;
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
 }
 
 int
diff --git a/malloc/tst-malloc-check.c b/malloc/tst-malloc-check.c
index 46938c0dbb..eb46cf3bbb 100644
--- a/malloc/tst-malloc-check.c
+++ b/malloc/tst-malloc-check.c
@@ -86,7 +86,15 @@ do_test (void)
     merror ("errno is not set correctly.");
   DIAG_POP_NEEDS_COMMENT;
 
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a valid warning about using a pointer made indeterminate
+     by a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
   free (p);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
 
   p = malloc (512);
   if (p == NULL)
@@ -104,7 +112,15 @@ do_test (void)
     merror ("errno is not set correctly.");
   DIAG_POP_NEEDS_COMMENT;
 
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a valid warning about using a pointer made indeterminate
+     by a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
   free (p);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
   free (q);
 
   return errors != 0;
diff --git a/malloc/tst-malloc-too-large.c b/malloc/tst-malloc-too-large.c
index e23aa08e4f..dac3c8086c 100644
--- a/malloc/tst-malloc-too-large.c
+++ b/malloc/tst-malloc-too-large.c
@@ -95,7 +95,15 @@ test_large_allocations (size_t size)
   DIAG_POP_NEEDS_COMMENT;
 #endif
   TEST_VERIFY (errno == ENOMEM);
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a warning about using a pointer made indeterminate by
+     a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
   free (ptr_to_realloc);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
 
   for (size_t nmemb = 1; nmemb <= 8; nmemb *= 2)
     if ((size % nmemb) == 0)
@@ -113,14 +121,30 @@ test_large_allocations (size_t size)
         test_setup ();
         TEST_VERIFY (reallocarray (ptr_to_realloc, nmemb, size / nmemb) == NULL);
         TEST_VERIFY (errno == ENOMEM);
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a warning about using a pointer made indeterminate by
+     a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
         free (ptr_to_realloc);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
 
         ptr_to_realloc = malloc (16);
         TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
         test_setup ();
         TEST_VERIFY (reallocarray (ptr_to_realloc, size / nmemb, nmemb) == NULL);
         TEST_VERIFY (errno == ENOMEM);
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a warning about using a pointer made indeterminate by
+     a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
         free (ptr_to_realloc);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
       }
     else
       break;
diff --git a/support/tst-support-open-dev-null-range.c b/support/tst-support-open-dev-null-range.c
index 3ed3177d57..e7526597ce 100644
--- a/support/tst-support-open-dev-null-range.c
+++ b/support/tst-support-open-dev-null-range.c
@@ -26,6 +26,8 @@
 #include <sys/resource.h>
 #include <stdlib.h>
 
+#include <libc-diag.h>
+
 #ifndef PATH_MAX
 # define PATH_MAX 1024
 #endif
@@ -41,8 +43,18 @@ check_path (int fd)
     = readlink (proc_fd_path, file_path, sizeof (file_path));
   free (proc_fd_path);
   if (file_path_length < 0)
-    FAIL_EXIT1 ("readlink (%s, %p, %zu)", proc_fd_path, file_path,
-		sizeof (file_path));
+    {
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a valid warning about using a pointer made indeterminate
+     by a prior call to free().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
+      FAIL_EXIT1 ("readlink (%s, %p, %zu)", proc_fd_path, file_path,
+		  sizeof (file_path));
+#if __GNUC_PREREQ (12, 0)
+      DIAG_POP_NEEDS_COMMENT;
+#endif
+    }
   file_path[file_path_length] = '\0';
   TEST_COMPARE_STRING (file_path, "/dev/null");
 }

Re: [PATCH] avoid -Wuse-after-free [BZ #26779]

[Jeff Law]

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

Re: [PATCH v2 0/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/24/22 19:52, Martin Sebor via Libc-alpha wrote:
> This is a repost of the original patch but broken down by source
> file and with some suppression done by #pragma GCC diagnostic
> instead of conversion to intptr_t.  It also adds fixes for
> the same problem in the test suite that I overlooked before.

Thanks for the repost! We really want gcc 12 and glibc 2.35 to work together.

For future posts please review the contribution checklist, we have some
specific instructions to help reviewers and CI/CD that interacts with your
patch.

(1) Allow the reviewer to review all of what you will push.

Your current posts do not use git format-patch and so do not provide me
with the commit message for review.

The intent is that I as a reviewer can review your commit message as
expected to be pushed. I want to be able to see all of the work you
will push (like a PR/MR) and approve it all.

It should be possible for you to have pushed all 5 patches as distinct
commits with commit messages, use git format-patch --cover-letter HEAD~5
to generate 6 files to mail out, and then you fill in patch 0 and send.

(2) CI/CD

Your use of "Re:" in patches 2-5 has broken CI, and it sees these as
follow-ups to your original messages.

The contribution checklist has some notes about this:
~~~
In order for an in-reply-to with a new version of the patch to be 
treated as a new patch you must remove the "Re:" from the subject. 
If you leave the "Re:" then patchwork considers your reply a comment
to the original patch. This is important to support reviewers using
patchwork for pulling patches and for CI/CD systems testing your patches.
~~~

Is "Re:" common in other communities you are a part of?

-- 
Cheers,
Carlos.

Re: [PATCH v2 1/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/24/22 19:57, Martin Sebor via Libc-alpha wrote:
> On 1/24/22 17:52, Martin Sebor wrote:
>> This is a repost of the original patch but broken down by source
>> file and with some suppression done by #pragma GCC diagnostic
>> instead of conversion to intptr_t.  It also adds fixes for
>> the same problem in the test suite that I overlooked before.
> 
> The attached patch suppresses the -Wuse-after-free instance in
> elf/ldconfig.c.
> 
>>
>> On 1/15/22 17:21, Martin Sebor wrote:
>>> 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
>>

OK for glibc 2.35, please push this commit.

Expected commit message (three lines):
~~~
elf: Fix use-after-free in ldconfig [BZ #26779]

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
~~~

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

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

OK. real_path is set if opt_chroot is non-NULL, and is a distinct
pointer from path in that case and must be freed (since chroot_canon
was malloc'd).

> -  free (path);
>    if (path != real_path)
>      free (real_path);
> +  free (path);


OK. This is correct, and is the only case I can see where we touch
path after freeing it.

>  }
>  
>  



-- 
Cheers,
Carlos.

Re: [PATCH v2 2/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/24/22 19:58, Martin Sebor via Libc-alpha wrote:
> On 1/24/22 17:52, Martin Sebor wrote:
>> This is a repost of the original patch but broken down by source
>> file and with some suppression done by #pragma GCC diagnostic
>> instead of conversion to intptr_t.  It also adds fixes for
>> the same problem in the test suite that I overlooked before.
> 
> The attached patch suppresses the -Wuse-after-free instance in
> intl/localealias.c.
> 
>>
>> On 1/15/22 17:21, Martin Sebor wrote:
>>> 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
>>

OK for glibc 2.35, please push this commit.

This file is shared with GNU Gettext, and the upstream gettext code still uses
pointers into the reallocated block.

Expected commit message (three lines):
~~~
intl: Avoid -Wuse-after-free [BZ #26779]

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
~~~

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/intl/localealias.c b/intl/localealias.c
> index 3ae360f40d..b36092363a 100644
> --- a/intl/localealias.c
> +++ b/intl/localealias.c
> @@ -318,7 +318,15 @@ 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.  */
> +#pragma GCC diagnostic push
> +
> +#if defined __GNUC__ && __GNUC__ >= 12
> +  /* Suppress the valid GCC 12 warning until the code below is changed
> +     to avoid using pointers to the reallocated block.  */
> +#  pragma GCC diagnostic ignored "-Wuse-after-free"
> +#endif

OK. Need to use general pragma because this is shared with upstream GNU Gettext.

> +
> +		    /* Increase size of memory pool.  */
>  		      size_t new_size = (string_space_max
>  					 + (alias_len + value_len > 1024
>  					    ? alias_len + value_len : 1024));
> @@ -351,6 +359,8 @@ read_alias_file (const char *fname, int fname_len)
>  					   value, value_len);
>  		  string_space_act += value_len;
>  
> +#pragma GCC diagnostic pop

OK.

> +
>  		  ++nmap;
>  		  ++added;
>  		}

-- 
Cheers,
Carlos.

Re: [PATCH v2 3/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/24/22 19:58, Martin Sebor via Libc-alpha wrote:
> On 1/24/22 17:52, Martin Sebor wrote:
>> This is a repost of the original patch but broken down by source
>> file and with some suppression done by #pragma GCC diagnostic
>> instead of conversion to intptr_t.  It also adds fixes for
>> the same problem in the test suite that I overlooked before.
> 
> The attached patch suppresses the -Wuse-after-free instance in
> io/ftw.c.
> 
>>
>> On 1/15/22 17:21, Martin Sebor wrote:
>>> 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
>>

OK for glibc 2.35, please push this commit.

Expected commit message (three lines)
~~~
io: Fix use-after-free in ftw [BZ #26779]

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
~~~

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

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

OK. Add a new pointer, and use that instead because ->content and buf may be unspecified at failure.

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

OK. Then set the content.

>  	}
>      }
>  


-- 
Cheers,
Carlos.

Re: [PATCH v2 4/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/24/22 19:58, Martin Sebor via Libc-alpha wrote:
> On 1/24/22 17:52, Martin Sebor wrote:
>> This is a repost of the original patch but broken down by source
>> file and with some suppression done by #pragma GCC diagnostic
>> instead of conversion to intptr_t.  It also adds fixes for
>> the same problem in the test suite that I overlooked before.
> 
> The attached patch suppresses the -Wuse-after-free instance in
> stdlib/setenv.c.
> 
>>
>> On 1/15/22 17:21, Martin Sebor wrote:
>>> 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
>>

OK for glibc 2.35, please push this commit.

Expected commit message (three lines)
~~~
io: Fix use-after-free in ftw [BZ #26779]

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
~~~

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

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

OK. Create a temporary pointer.

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

OK. Lastly, use the temporary pointer for the comparison.

>  	memcpy ((char *) new_environ, (char *) __environ,
>  		size * sizeof (char *));
>  


-- 
Cheers,
Carlos.

Re: [PATCH v2 5/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/24/22 19:58, Martin Sebor via Libc-alpha wrote:
> On 1/24/22 17:52, Martin Sebor wrote:
>> This is a repost of the original patch but broken down by source
>> file and with some suppression done by #pragma GCC diagnostic
>> instead of conversion to intptr_t.  It also adds fixes for
>> the same problem in the test suite that I overlooked before.
> 
> The attached patch suppresses the -Wuse-after-free instance in
> the testsuite.
> 
>>
>> On 1/15/22 17:21, Martin Sebor wrote:
>>> 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
>>

This patch is not ready.

Some tests are going to do invalid things to test specific behaviour and we need
to possibly suppress those warnings. The malloc tests look correct.

The support/tst-support-open-dev-null-range.c doesn't look correct, please send v3
of just this *whole* patch as a new patch. I'll review again.

> diff --git a/malloc/tst-malloc-backtrace.c b/malloc/tst-malloc-backtrace.c
> index ea66da23ef..8a3f4a0b55 100644
> --- a/malloc/tst-malloc-backtrace.c
> +++ b/malloc/tst-malloc-backtrace.c
> @@ -20,6 +20,7 @@
>  #include <stdlib.h>
>  
>  #include <support/support.h>
> +#include <libc-diag.h>

OK. Add header required for DIAG_* macros.

>  
>  #define SIZE 4096
>  
> @@ -29,7 +30,15 @@ __attribute__((noinline))
>  call_free (void *ptr)
>  {
>    free (ptr);
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a valid warning about using a pointer made indeterminate
> +     by a prior call to malloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>    *(size_t *)(ptr - sizeof (size_t)) = 1;
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK. Specifically testing use-after-free write to chunk to corrupt memory.

>  }
>  
>  int
> diff --git a/malloc/tst-malloc-check.c b/malloc/tst-malloc-check.c
> index 46938c0dbb..eb46cf3bbb 100644
> --- a/malloc/tst-malloc-check.c
> +++ b/malloc/tst-malloc-check.c

OK. Already includes libc-diag.h.

> @@ -86,7 +86,15 @@ do_test (void)
>      merror ("errno is not set correctly.");
>    DIAG_POP_NEEDS_COMMENT;
>  
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a valid warning about using a pointer made indeterminate
> +     by a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>    free (p);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK. Previous realloc made p indeterminate.

>  
>    p = malloc (512);
>    if (p == NULL)
> @@ -104,7 +112,15 @@ do_test (void)
>      merror ("errno is not set correctly.");
>    DIAG_POP_NEEDS_COMMENT;
>  
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a valid warning about using a pointer made indeterminate
> +     by a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>    free (p);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK. Likewise.

>    free (q);
>  
>    return errors != 0;
> diff --git a/malloc/tst-malloc-too-large.c b/malloc/tst-malloc-too-large.c
> index e23aa08e4f..dac3c8086c 100644
> --- a/malloc/tst-malloc-too-large.c
> +++ b/malloc/tst-malloc-too-large.c

OK. Already includes libc-diag.h.

> @@ -95,7 +95,15 @@ test_large_allocations (size_t size)
>    DIAG_POP_NEEDS_COMMENT;
>  #endif
>    TEST_VERIFY (errno == ENOMEM);
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a warning about using a pointer made indeterminate by
> +     a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>    free (ptr_to_realloc);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK.

>  
>    for (size_t nmemb = 1; nmemb <= 8; nmemb *= 2)
>      if ((size % nmemb) == 0)
> @@ -113,14 +121,30 @@ test_large_allocations (size_t size)
>          test_setup ();
>          TEST_VERIFY (reallocarray (ptr_to_realloc, nmemb, size / nmemb) == NULL);
>          TEST_VERIFY (errno == ENOMEM);
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a warning about using a pointer made indeterminate by
> +     a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>          free (ptr_to_realloc);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK.

>  
>          ptr_to_realloc = malloc (16);
>          TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
>          test_setup ();
>          TEST_VERIFY (reallocarray (ptr_to_realloc, size / nmemb, nmemb) == NULL);
>          TEST_VERIFY (errno == ENOMEM);
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a warning about using a pointer made indeterminate by
> +     a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>          free (ptr_to_realloc);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK.

>        }
>      else
>        break;
> diff --git a/support/tst-support-open-dev-null-range.c b/support/tst-support-open-dev-null-range.c
> index 3ed3177d57..e7526597ce 100644
> --- a/support/tst-support-open-dev-null-range.c
> +++ b/support/tst-support-open-dev-null-range.c
> @@ -26,6 +26,8 @@
>  #include <sys/resource.h>
>  #include <stdlib.h>
>  
> +#include <libc-diag.h>

OK. New macros required.

> +
>  #ifndef PATH_MAX
>  # define PATH_MAX 1024
>  #endif
> @@ -41,8 +43,18 @@ check_path (int fd)
>      = readlink (proc_fd_path, file_path, sizeof (file_path));
>    free (proc_fd_path);
>    if (file_path_length < 0)
> -    FAIL_EXIT1 ("readlink (%s, %p, %zu)", proc_fd_path, file_path,
> -		sizeof (file_path));
> +    {
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a valid warning about using a pointer made indeterminate
> +     by a prior call to free().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
> +      FAIL_EXIT1 ("readlink (%s, %p, %zu)", proc_fd_path, file_path,
> +		  sizeof (file_path));
> +#if __GNUC_PREREQ (12, 0)
> +      DIAG_POP_NEEDS_COMMENT;
> +#endif
> +    }

We should move free (proc_fd_path) to after the check to correct the use-after-free.

>    file_path[file_path_length] = '\0';
>    TEST_COMPARE_STRING (file_path, "/dev/null");
>  }


-- 
Cheers,
Carlos.

Re: [PATCH v2 4/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/25/22 12:49, Carlos O'Donell wrote:
> On 1/24/22 19:58, Martin Sebor via Libc-alpha wrote:
>> On 1/24/22 17:52, Martin Sebor wrote:
>>> This is a repost of the original patch but broken down by source
>>> file and with some suppression done by #pragma GCC diagnostic
>>> instead of conversion to intptr_t.  It also adds fixes for
>>> the same problem in the test suite that I overlooked before.
>>
>> The attached patch suppresses the -Wuse-after-free instance in
>> stdlib/setenv.c.
>>
>>>
>>> On 1/15/22 17:21, Martin Sebor wrote:
>>>> 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
>>>
> 
> OK for glibc 2.35, please push this commit.
> 
> Expected commit message (three lines)
> ~~~
> io: Fix use-after-free in ftw [BZ #26779]


Should be:
~~~
stdlib: Avoid -Wuse-after-free [BZ #26779]

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
~~~

> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ~~~
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 
>> 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;
> 
> OK. Create a temporary pointer.
> 
>>        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)
> 
> OK. Lastly, use the temporary pointer for the comparison.
> 
>>  	memcpy ((char *) new_environ, (char *) __environ,
>>  		size * sizeof (char *));
>>  
> 
> 


-- 
Cheers,
Carlos.

Re: [PATCH v2 4/5] avoid -Wuse-after-free [BZ #26779]

[Florian Weimer]

* Carlos O'Donell via Libc-alpha:

> Should be:
> ~~~
> stdlib: Avoid -Wuse-after-free [BZ #26779]
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ~~~

Shouldn't the commit subject mention __add_to_environ (or maybe setenv)?

Thanks,
Florian

[PATCH v3 5/5] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 7354 bytes --]

On 1/25/22 10:49, Carlos O'Donell wrote:
> On 1/24/22 19:58, Martin Sebor via Libc-alpha wrote:
>> On 1/24/22 17:52, Martin Sebor wrote:
>>> This is a repost of the original patch but broken down by source
>>> file and with some suppression done by #pragma GCC diagnostic
>>> instead of conversion to intptr_t.  It also adds fixes for
>>> the same problem in the test suite that I overlooked before.
>>
>> The attached patch suppresses the -Wuse-after-free instance in
>> the testsuite.
>>
>>>
>>> On 1/15/22 17:21, Martin Sebor wrote:
>>>> 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
>>>
> 
> This patch is not ready.
> 
> Some tests are going to do invalid things to test specific behaviour and we need
> to possibly suppress those warnings. The malloc tests look correct.
> 
> The support/tst-support-open-dev-null-range.c doesn't look correct, please send v3
> of just this *whole* patch as a new patch. I'll review again.

Okay, I've moved the free() call after the FAIL_EXIT.  I've also
suppressed the same warning in a few more tests that I missed
before.

Martin

> 
>> diff --git a/malloc/tst-malloc-backtrace.c b/malloc/tst-malloc-backtrace.c
>> index ea66da23ef..8a3f4a0b55 100644
>> --- a/malloc/tst-malloc-backtrace.c
>> +++ b/malloc/tst-malloc-backtrace.c
>> @@ -20,6 +20,7 @@
>>   #include <stdlib.h>
>>   
>>   #include <support/support.h>
>> +#include <libc-diag.h>
> 
> OK. Add header required for DIAG_* macros.
> 
>>   
>>   #define SIZE 4096
>>   
>> @@ -29,7 +30,15 @@ __attribute__((noinline))
>>   call_free (void *ptr)
>>   {
>>     free (ptr);
>> +#if __GNUC_PREREQ (12, 0)
>> +  /* Ignore a valid warning about using a pointer made indeterminate
>> +     by a prior call to malloc().  */
>> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
>> +#endif
>>     *(size_t *)(ptr - sizeof (size_t)) = 1;
>> +#if __GNUC_PREREQ (12, 0)
>> +  DIAG_POP_NEEDS_COMMENT;
>> +#endif
> 
> OK. Specifically testing use-after-free write to chunk to corrupt memory.
> 
>>   }
>>   
>>   int
>> diff --git a/malloc/tst-malloc-check.c b/malloc/tst-malloc-check.c
>> index 46938c0dbb..eb46cf3bbb 100644
>> --- a/malloc/tst-malloc-check.c
>> +++ b/malloc/tst-malloc-check.c
> 
> OK. Already includes libc-diag.h.
> 
>> @@ -86,7 +86,15 @@ do_test (void)
>>       merror ("errno is not set correctly.");
>>     DIAG_POP_NEEDS_COMMENT;
>>   
>> +#if __GNUC_PREREQ (12, 0)
>> +  /* Ignore a valid warning about using a pointer made indeterminate
>> +     by a prior call to realloc().  */
>> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
>> +#endif
>>     free (p);
>> +#if __GNUC_PREREQ (12, 0)
>> +  DIAG_POP_NEEDS_COMMENT;
>> +#endif
> 
> OK. Previous realloc made p indeterminate.
> 
>>   
>>     p = malloc (512);
>>     if (p == NULL)
>> @@ -104,7 +112,15 @@ do_test (void)
>>       merror ("errno is not set correctly.");
>>     DIAG_POP_NEEDS_COMMENT;
>>   
>> +#if __GNUC_PREREQ (12, 0)
>> +  /* Ignore a valid warning about using a pointer made indeterminate
>> +     by a prior call to realloc().  */
>> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
>> +#endif
>>     free (p);
>> +#if __GNUC_PREREQ (12, 0)
>> +  DIAG_POP_NEEDS_COMMENT;
>> +#endif
> 
> OK. Likewise.
> 
>>     free (q);
>>   
>>     return errors != 0;
>> diff --git a/malloc/tst-malloc-too-large.c b/malloc/tst-malloc-too-large.c
>> index e23aa08e4f..dac3c8086c 100644
>> --- a/malloc/tst-malloc-too-large.c
>> +++ b/malloc/tst-malloc-too-large.c
> 
> OK. Already includes libc-diag.h.
> 
>> @@ -95,7 +95,15 @@ test_large_allocations (size_t size)
>>     DIAG_POP_NEEDS_COMMENT;
>>   #endif
>>     TEST_VERIFY (errno == ENOMEM);
>> +#if __GNUC_PREREQ (12, 0)
>> +  /* Ignore a warning about using a pointer made indeterminate by
>> +     a prior call to realloc().  */
>> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
>> +#endif
>>     free (ptr_to_realloc);
>> +#if __GNUC_PREREQ (12, 0)
>> +  DIAG_POP_NEEDS_COMMENT;
>> +#endif
> 
> OK.
> 
>>   
>>     for (size_t nmemb = 1; nmemb <= 8; nmemb *= 2)
>>       if ((size % nmemb) == 0)
>> @@ -113,14 +121,30 @@ test_large_allocations (size_t size)
>>           test_setup ();
>>           TEST_VERIFY (reallocarray (ptr_to_realloc, nmemb, size / nmemb) == NULL);
>>           TEST_VERIFY (errno == ENOMEM);
>> +#if __GNUC_PREREQ (12, 0)
>> +  /* Ignore a warning about using a pointer made indeterminate by
>> +     a prior call to realloc().  */
>> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
>> +#endif
>>           free (ptr_to_realloc);
>> +#if __GNUC_PREREQ (12, 0)
>> +  DIAG_POP_NEEDS_COMMENT;
>> +#endif
> 
> OK.
> 
>>   
>>           ptr_to_realloc = malloc (16);
>>           TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
>>           test_setup ();
>>           TEST_VERIFY (reallocarray (ptr_to_realloc, size / nmemb, nmemb) == NULL);
>>           TEST_VERIFY (errno == ENOMEM);
>> +#if __GNUC_PREREQ (12, 0)
>> +  /* Ignore a warning about using a pointer made indeterminate by
>> +     a prior call to realloc().  */
>> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
>> +#endif
>>           free (ptr_to_realloc);
>> +#if __GNUC_PREREQ (12, 0)
>> +  DIAG_POP_NEEDS_COMMENT;
>> +#endif
> 
> OK.
> 
>>         }
>>       else
>>         break;
>> diff --git a/support/tst-support-open-dev-null-range.c b/support/tst-support-open-dev-null-range.c
>> index 3ed3177d57..e7526597ce 100644
>> --- a/support/tst-support-open-dev-null-range.c
>> +++ b/support/tst-support-open-dev-null-range.c
>> @@ -26,6 +26,8 @@
>>   #include <sys/resource.h>
>>   #include <stdlib.h>
>>   
>> +#include <libc-diag.h>
> 
> OK. New macros required.
> 
>> +
>>   #ifndef PATH_MAX
>>   # define PATH_MAX 1024
>>   #endif
>> @@ -41,8 +43,18 @@ check_path (int fd)
>>       = readlink (proc_fd_path, file_path, sizeof (file_path));
>>     free (proc_fd_path);
>>     if (file_path_length < 0)
>> -    FAIL_EXIT1 ("readlink (%s, %p, %zu)", proc_fd_path, file_path,
>> -		sizeof (file_path));
>> +    {
>> +#if __GNUC_PREREQ (12, 0)
>> +  /* Ignore a valid warning about using a pointer made indeterminate
>> +     by a prior call to free().  */
>> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
>> +#endif
>> +      FAIL_EXIT1 ("readlink (%s, %p, %zu)", proc_fd_path, file_path,
>> +		  sizeof (file_path));
>> +#if __GNUC_PREREQ (12, 0)
>> +      DIAG_POP_NEEDS_COMMENT;
>> +#endif
>> +    }
> 
> We should move free (proc_fd_path) to after the check to correct the use-after-free.
> 
>>     file_path[file_path_length] = '\0';
>>     TEST_COMPARE_STRING (file_path, "/dev/null");
>>   }
> 
> 

[-- Attachment #2: glibc-26779-5.diff --]
[-- Type: text/x-patch, Size: 5267 bytes --]

commit c23cf7ff784186b8094b97a47a8445808145a69c
Author: Martin Sebor <msebor@redhat.com>
Date:   Tue Jan 25 15:39:38 2022 -0700

    Avoid -Wuse-after-free in tests.

diff --git a/malloc/tst-malloc-backtrace.c b/malloc/tst-malloc-backtrace.c
index ea66da23ef..65e1a1ffbc 100644
--- a/malloc/tst-malloc-backtrace.c
+++ b/malloc/tst-malloc-backtrace.c
@@ -20,6 +20,7 @@
 #include <stdlib.h>
 
 #include <support/support.h>
+#include <libc-diag.h>
 
 #define SIZE 4096
 
@@ -29,7 +30,15 @@ __attribute__((noinline))
 call_free (void *ptr)
 {
   free (ptr);
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a valid warning about using a pointer made indeterminate
+     by a prior call to free().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
   *(size_t *)(ptr - sizeof (size_t)) = 1;
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
 }
 
 int
diff --git a/malloc/tst-malloc-check.c b/malloc/tst-malloc-check.c
index 46938c0dbb..eb46cf3bbb 100644
--- a/malloc/tst-malloc-check.c
+++ b/malloc/tst-malloc-check.c
@@ -86,7 +86,15 @@ do_test (void)
     merror ("errno is not set correctly.");
   DIAG_POP_NEEDS_COMMENT;
 
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a valid warning about using a pointer made indeterminate
+     by a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
   free (p);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
 
   p = malloc (512);
   if (p == NULL)
@@ -104,7 +112,15 @@ do_test (void)
     merror ("errno is not set correctly.");
   DIAG_POP_NEEDS_COMMENT;
 
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a valid warning about using a pointer made indeterminate
+     by a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
   free (p);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
   free (q);
 
   return errors != 0;
diff --git a/malloc/tst-malloc-too-large.c b/malloc/tst-malloc-too-large.c
index e23aa08e4f..dac3c8086c 100644
--- a/malloc/tst-malloc-too-large.c
+++ b/malloc/tst-malloc-too-large.c
@@ -95,7 +95,15 @@ test_large_allocations (size_t size)
   DIAG_POP_NEEDS_COMMENT;
 #endif
   TEST_VERIFY (errno == ENOMEM);
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a warning about using a pointer made indeterminate by
+     a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
   free (ptr_to_realloc);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
 
   for (size_t nmemb = 1; nmemb <= 8; nmemb *= 2)
     if ((size % nmemb) == 0)
@@ -113,14 +121,30 @@ test_large_allocations (size_t size)
         test_setup ();
         TEST_VERIFY (reallocarray (ptr_to_realloc, nmemb, size / nmemb) == NULL);
         TEST_VERIFY (errno == ENOMEM);
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a warning about using a pointer made indeterminate by
+     a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
         free (ptr_to_realloc);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
 
         ptr_to_realloc = malloc (16);
         TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
         test_setup ();
         TEST_VERIFY (reallocarray (ptr_to_realloc, size / nmemb, nmemb) == NULL);
         TEST_VERIFY (errno == ENOMEM);
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a warning about using a pointer made indeterminate by
+     a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
         free (ptr_to_realloc);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
       }
     else
       break;
diff --git a/malloc/tst-obstack.c b/malloc/tst-obstack.c
index 18af8ea62f..d80f471fa0 100644
--- a/malloc/tst-obstack.c
+++ b/malloc/tst-obstack.c
@@ -20,8 +20,8 @@ verbose_malloc (size_t size)
 static void
 verbose_free (void *buf)
 {
-  free (buf);
   printf ("free (%p)\n", buf);
+  free (buf);
 }
 
 static int
diff --git a/malloc/tst-realloc.c b/malloc/tst-realloc.c
index 83f165516a..8ce59f2602 100644
--- a/malloc/tst-realloc.c
+++ b/malloc/tst-realloc.c
@@ -138,8 +138,16 @@ do_test (void)
   if (ok == 0)
     merror ("first 16 bytes were not correct after failed realloc");
 
+#if __GNUC_PREREQ (12, 0)
+  /* Ignore a valid warning about using a pointer made indeterminate
+     by a prior call to realloc().  */
+  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
+#endif
   /* realloc (p, 0) frees p (C89) and returns NULL (glibc).  */
   p = realloc (p, 0);
+#if __GNUC_PREREQ (12, 0)
+  DIAG_POP_NEEDS_COMMENT;
+#endif
   if (p != NULL)
     merror ("realloc (p, 0) returned non-NULL.");
 
diff --git a/support/tst-support-open-dev-null-range.c b/support/tst-support-open-dev-null-range.c
index 3ed3177d57..690b9d30b7 100644
--- a/support/tst-support-open-dev-null-range.c
+++ b/support/tst-support-open-dev-null-range.c
@@ -39,10 +39,11 @@ check_path (int fd)
   char file_path[PATH_MAX];
   ssize_t file_path_length
     = readlink (proc_fd_path, file_path, sizeof (file_path));
-  free (proc_fd_path);
   if (file_path_length < 0)
     FAIL_EXIT1 ("readlink (%s, %p, %zu)", proc_fd_path, file_path,
 		sizeof (file_path));
+
+  free (proc_fd_path);
   file_path[file_path_length] = '\0';
   TEST_COMPARE_STRING (file_path, "/dev/null");
 }

Re: [PATCH v2 0/5] avoid -Wuse-after-free [BZ #26779]

[Martin Sebor]

On 1/25/22 10:46, Carlos O'Donell wrote:
> On 1/24/22 19:52, Martin Sebor via Libc-alpha wrote:
>> This is a repost of the original patch but broken down by source
>> file and with some suppression done by #pragma GCC diagnostic
>> instead of conversion to intptr_t.  It also adds fixes for
>> the same problem in the test suite that I overlooked before.
> 
> Thanks for the repost! We really want gcc 12 and glibc 2.35 to work together.
> 
> For future posts please review the contribution checklist, we have some
> specific instructions to help reviewers and CI/CD that interacts with your
> patch.
> 
> (1) Allow the reviewer to review all of what you will push.
> 
> Your current posts do not use git format-patch and so do not provide me
> with the commit message for review.
> 
> The intent is that I as a reviewer can review your commit message as
> expected to be pushed. I want to be able to see all of the work you
> will push (like a PR/MR) and approve it all.
> 
> It should be possible for you to have pushed all 5 patches as distinct
> commits with commit messages, use git format-patch --cover-letter HEAD~5
> to generate 6 files to mail out, and then you fill in patch 0 and send.

Thanks for the review!  I've pushed the changes as distinct commits
with the adjusted descriptions (including Florian suggestion) after
rerunning the tests.  Some of the tests failed so I fixed those up
and posted an update.

> 
> (2) CI/CD
> 
> Your use of "Re:" in patches 2-5 has broken CI, and it sees these as
> follow-ups to your original messages.
> 
> The contribution checklist has some notes about this:
> ~~~
> In order for an in-reply-to with a new version of the patch to be
> treated as a new patch you must remove the "Re:" from the subject.
> If you leave the "Re:" then patchwork considers your reply a comment
> to the original patch. This is important to support reviewers using
> patchwork for pulling patches and for CI/CD systems testing your patches.
> ~~~
> 
> Is "Re:" common in other communities you are a part of?
> 

In GCC it doesn't matter.  Other things that don't matter here
are enforced there (e.g., like the minute details of ChangeLog
entries, the exact form of the PR reference and where it goes).

Martin

Re: [PATCH v2 4/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/25/22 16:47, Florian Weimer wrote:
> * Carlos O'Donell via Libc-alpha:
> 
>> Should be:
>> ~~~
>> stdlib: Avoid -Wuse-after-free [BZ #26779]
>>
>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>> ~~~
> 
> Shouldn't the commit subject mention __add_to_environ (or maybe setenv)?

Yes, it could have, that would have been a higher quality commit
message. Thanks for the review here, I'll work to add functional
name references here in the future.

-- 
Cheers,
Carlos.

Re: [PATCH v3 5/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/25/22 17:50, Martin Sebor wrote:
> On 1/25/22 10:49, Carlos O'Donell wrote:
>> On 1/24/22 19:58, Martin Sebor via Libc-alpha wrote:
>>> On 1/24/22 17:52, Martin Sebor wrote:
>>>> This is a repost of the original patch but broken down by source
>>>> file and with some suppression done by #pragma GCC diagnostic
>>>> instead of conversion to intptr_t.  It also adds fixes for
>>>> the same problem in the test suite that I overlooked before.
>>>
>>> The attached patch suppresses the -Wuse-after-free instance in
>>> the testsuite.
>>>
>>>>
>>>> On 1/15/22 17:21, Martin Sebor wrote:
>>>>> 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
>>>>
>>
>> This patch is not ready.
>>
>> Some tests are going to do invalid things to test specific behaviour and we need
>> to possibly suppress those warnings. The malloc tests look correct.
>>
>> The support/tst-support-open-dev-null-range.c doesn't look correct, please send v3
>> of just this *whole* patch as a new patch. I'll review again.
> 
> Okay, I've moved the free() call after the FAIL_EXIT.  I've also
> suppressed the same warning in a few more tests that I missed
> before.
OK for glibc 2.35 with commit message including bug #.

Please push.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> commit c23cf7ff784186b8094b97a47a8445808145a69c
> Author: Martin Sebor <msebor@redhat.com>
> Date:   Tue Jan 25 15:39:38 2022 -0700
> 
>     Avoid -Wuse-after-free in tests.
> 

Suggest:

Avoid -Wuse-after-free in tests [BZ #26779]

> diff --git a/malloc/tst-malloc-backtrace.c b/malloc/tst-malloc-backtrace.c
> index ea66da23ef..65e1a1ffbc 100644
> --- a/malloc/tst-malloc-backtrace.c
> +++ b/malloc/tst-malloc-backtrace.c
> @@ -20,6 +20,7 @@
>  #include <stdlib.h>
>  
>  #include <support/support.h>
> +#include <libc-diag.h>
>  
>  #define SIZE 4096
>  
> @@ -29,7 +30,15 @@ __attribute__((noinline))
>  call_free (void *ptr)
>  {
>    free (ptr);
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a valid warning about using a pointer made indeterminate
> +     by a prior call to free().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>    *(size_t *)(ptr - sizeof (size_t)) = 1;
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK.

>  }
>  
>  int
> diff --git a/malloc/tst-malloc-check.c b/malloc/tst-malloc-check.c
> index 46938c0dbb..eb46cf3bbb 100644
> --- a/malloc/tst-malloc-check.c
> +++ b/malloc/tst-malloc-check.c
> @@ -86,7 +86,15 @@ do_test (void)
>      merror ("errno is not set correctly.");
>    DIAG_POP_NEEDS_COMMENT;
>  
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a valid warning about using a pointer made indeterminate
> +     by a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>    free (p);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK.

>  
>    p = malloc (512);
>    if (p == NULL)
> @@ -104,7 +112,15 @@ do_test (void)
>      merror ("errno is not set correctly.");
>    DIAG_POP_NEEDS_COMMENT;
>  
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a valid warning about using a pointer made indeterminate
> +     by a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>    free (p);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif


OK.

>    free (q);
>  
>    return errors != 0;
> diff --git a/malloc/tst-malloc-too-large.c b/malloc/tst-malloc-too-large.c
> index e23aa08e4f..dac3c8086c 100644
> --- a/malloc/tst-malloc-too-large.c
> +++ b/malloc/tst-malloc-too-large.c
> @@ -95,7 +95,15 @@ test_large_allocations (size_t size)
>    DIAG_POP_NEEDS_COMMENT;
>  #endif
>    TEST_VERIFY (errno == ENOMEM);
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a warning about using a pointer made indeterminate by
> +     a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>    free (ptr_to_realloc);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK.

>  
>    for (size_t nmemb = 1; nmemb <= 8; nmemb *= 2)
>      if ((size % nmemb) == 0)
> @@ -113,14 +121,30 @@ test_large_allocations (size_t size)
>          test_setup ();
>          TEST_VERIFY (reallocarray (ptr_to_realloc, nmemb, size / nmemb) == NULL);
>          TEST_VERIFY (errno == ENOMEM);
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a warning about using a pointer made indeterminate by
> +     a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>          free (ptr_to_realloc);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK.

>  
>          ptr_to_realloc = malloc (16);
>          TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
>          test_setup ();
>          TEST_VERIFY (reallocarray (ptr_to_realloc, size / nmemb, nmemb) == NULL);
>          TEST_VERIFY (errno == ENOMEM);
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a warning about using a pointer made indeterminate by
> +     a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>          free (ptr_to_realloc);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif

OK.

>        }
>      else
>        break;
> diff --git a/malloc/tst-obstack.c b/malloc/tst-obstack.c
> index 18af8ea62f..d80f471fa0 100644
> --- a/malloc/tst-obstack.c
> +++ b/malloc/tst-obstack.c
> @@ -20,8 +20,8 @@ verbose_malloc (size_t size)
>  static void
>  verbose_free (void *buf)
>  {
> -  free (buf);
>    printf ("free (%p)\n", buf);
> +  free (buf);

OK. Correct fix. Thanks.

>  }
>  
>  static int
> diff --git a/malloc/tst-realloc.c b/malloc/tst-realloc.c
> index 83f165516a..8ce59f2602 100644
> --- a/malloc/tst-realloc.c
> +++ b/malloc/tst-realloc.c
> @@ -138,8 +138,16 @@ do_test (void)
>    if (ok == 0)
>      merror ("first 16 bytes were not correct after failed realloc");
>  
> +#if __GNUC_PREREQ (12, 0)
> +  /* Ignore a valid warning about using a pointer made indeterminate
> +     by a prior call to realloc().  */
> +  DIAG_IGNORE_NEEDS_COMMENT (12, "-Wuse-after-free");
> +#endif
>    /* realloc (p, 0) frees p (C89) and returns NULL (glibc).  */
>    p = realloc (p, 0);
> +#if __GNUC_PREREQ (12, 0)
> +  DIAG_POP_NEEDS_COMMENT;
> +#endif


OK.

>    if (p != NULL)
>      merror ("realloc (p, 0) returned non-NULL.");
>  
> diff --git a/support/tst-support-open-dev-null-range.c b/support/tst-support-open-dev-null-range.c
> index 3ed3177d57..690b9d30b7 100644
> --- a/support/tst-support-open-dev-null-range.c
> +++ b/support/tst-support-open-dev-null-range.c
> @@ -39,10 +39,11 @@ check_path (int fd)
>    char file_path[PATH_MAX];
>    ssize_t file_path_length
>      = readlink (proc_fd_path, file_path, sizeof (file_path));
> -  free (proc_fd_path);
>    if (file_path_length < 0)
>      FAIL_EXIT1 ("readlink (%s, %p, %zu)", proc_fd_path, file_path,
>  		sizeof (file_path));
> +
> +  free (proc_fd_path);

OK. Correct fix. Thanks.

>    file_path[file_path_length] = '\0';
>    TEST_COMPARE_STRING (file_path, "/dev/null");
>  }


-- 
Cheers,
Carlos.

Re: [PATCH v3 5/5] avoid -Wuse-after-free [BZ #26779]

[Joseph Myers]

Note that there are still -Wuse-after-free build failures in the testsuite 
for many 32-bit platforms.  E.g., on i686-linux-gnu:

tst-mallocalign1.c: In function 'do_test':
tst-mallocalign1.c:69:1: error: pointer 'p' used after 'free' [-Werror=use-after-free]
   69 | }
      | ^
tst-mallocalign1.c:42:3: note: call to 'free' here
   42 |   free (p);
      |   ^~~~~~~~

Also, the s390x-linux-gnu-O3 build-many-glibcs.py configuration shows 
failures:

In function 'do_test',
    inlined from 'legacy_test_function' at ../test-skeleton.c:55:10:
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~
tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v3 5/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/28/22 08:10, Joseph Myers wrote:
> Note that there are still -Wuse-after-free build failures in the testsuite 
> for many 32-bit platforms.  E.g., on i686-linux-gnu:
> 
> tst-mallocalign1.c: In function 'do_test':
> tst-mallocalign1.c:69:1: error: pointer 'p' used after 'free' [-Werror=use-after-free]
>    69 | }
>       | ^
> tst-mallocalign1.c:42:3: note: call to 'free' here
>    42 |   free (p);
>       |   ^~~~~~~~
> 
> Also, the s390x-linux-gnu-O3 build-many-glibcs.py configuration shows 
> failures:
> 
> In function 'do_test',
>     inlined from 'legacy_test_function' at ../test-skeleton.c:55:10:
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
>   134 |       if (c[i] != 0xff)
>       |           ~^~~
> tst-realloc.c:124:7: note: call to 'realloc' here
>   124 |   c = realloc (p, -1);
>       |       ^~~~~~~~~~~~~~~
> 

I'm going to look at these right now. I want clean gcc 12 glibc 2.35 results before
we release.

-- 
Cheers,
Carlos.

Re: [PATCH v3 5/5] avoid -Wuse-after-free [BZ #26779]

[Joseph Myers]

On Fri, 28 Jan 2022, Carlos O'Donell via Libc-alpha wrote:

> I'm going to look at these right now. I want clean gcc 12 glibc 2.35 
> results before we release.

There are also three ICE bugs in GCC 12 preventing glibc from building, so 
you won't get fully clean build-many-glibcs.py results with just glibc 
fixes.  GCC bug 103722 on sh4, bug 104153 on or1k and 104154 on arc (the 
last two reported by Jeff based on ICEs building newlib, but the ICEs I 
see building glibc are in the same place in the compiler, so probably the 
same bugs).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v3 5/5] avoid -Wuse-after-free [BZ #26779]

[Jeff Law]

On 1/28/2022 10:51 AM, Joseph Myers wrote:
> On Fri, 28 Jan 2022, Carlos O'Donell via Libc-alpha wrote:
>
>> I'm going to look at these right now. I want clean gcc 12 glibc 2.35
>> results before we release.
> There are also three ICE bugs in GCC 12 preventing glibc from building, so
> you won't get fully clean build-many-glibcs.py results with just glibc
> fixes.  GCC bug 103722 on sh4, bug 104153 on or1k and 104154 on arc (the
> last two reported by Jeff based on ICEs building newlib, but the ICEs I
> see building glibc are in the same place in the compiler, so probably the
> same bugs).
FYI, I got a good build with the patch in 104153 + an unrelated newlib 
fix on or1k-elf.  So I'll take a deeper look at Robin's patch over the 
weekend if I can.

Jeff

Re: [PATCH v3 5/5] avoid -Wuse-after-free [BZ #26779]

[Carlos O'Donell]

On 1/28/22 12:51, Joseph Myers wrote:
> On Fri, 28 Jan 2022, Carlos O'Donell via Libc-alpha wrote:
> 
>> I'm going to look at these right now. I want clean gcc 12 glibc 2.35 
>> results before we release.
> 
> There are also three ICE bugs in GCC 12 preventing glibc from building, so 
> you won't get fully clean build-many-glibcs.py results with just glibc 
> fixes.  GCC bug 103722 on sh4, bug 104153 on or1k and 104154 on arc (the 
> last two reported by Jeff based on ICEs building newlib, but the ICEs I 
> see building glibc are in the same place in the compiler, so probably the 
> same bugs).
 
Thanks.

I just fixed the last -Wuse-after-free bug.

The ICEs are compiler-side fixes and I don't think we'd want to work around them
in glibc unless the workaround was trivial. I dislike putting -O0 or -O1 in clfags
just to build things, but that's what the downstream distributions need to do
sometimes :-)

-- 
Cheers,
Carlos.

Re: [PATCH v3 5/5] avoid -Wuse-after-free [BZ #26779]

[Joseph Myers]

On Mon, 31 Jan 2022, Carlos O'Donell via Libc-alpha wrote:

> On 1/28/22 12:51, Joseph Myers wrote:
> > On Fri, 28 Jan 2022, Carlos O'Donell via Libc-alpha wrote:
> > 
> >> I'm going to look at these right now. I want clean gcc 12 glibc 2.35 
> >> results before we release.
> > 
> > There are also three ICE bugs in GCC 12 preventing glibc from building, so 
> > you won't get fully clean build-many-glibcs.py results with just glibc 
> > fixes.  GCC bug 103722 on sh4, bug 104153 on or1k and 104154 on arc (the 
> > last two reported by Jeff based on ICEs building newlib, but the ICEs I 
> > see building glibc are in the same place in the compiler, so probably the 
> > same bugs).
>  
> Thanks.
> 
> I just fixed the last -Wuse-after-free bug.

I think there are still some more -Wuse-after-free issues building the 
testsuite.

https://sourceware.org/pipermail/libc-testresults/2022q1/009283.html

s390x-linux-gnu-O3 failures are as shown at 
<https://sourceware.org/pipermail/libc-alpha/2022-January/135797.html>.

hppa-linux-gnu, microblaze-linux-gnu, microblazeel-linux-gnu, 
glibcs-sparcv8-linux-gnu-leon3, sparcv9-linux-gnu, 
sparcv9-linux-gnu-disable-multi-arch:

In function 'do_test',
    inlined from 'legacy_test_function' at ../test-skeleton.c:55:10:tst-realloc.c:134:12: error: pointer 'p' may be used after 'realloc' [-Werror=use-after-free]
  134 |       if (c[i] != 0xff)
      |           ~^~~
tst-realloc.c:124:7: note: call to 'realloc' here
  124 |   c = realloc (p, -1);
      |       ^~~~~~~~~~~~~~~

-- 
Joseph S. Myers
joseph@codesourcery.com