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

  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
  

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
>
  

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
>
  

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
>
  

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
>
  

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?
  

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

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.

>  	}
>      }
>
  

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

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

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

* 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
  

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
  

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.