diff mbox series

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

Message ID 3ad704bb-a061-1dc2-3b6d-8343f70a3c92@gmail.com
State New
Headers show
Series avoid -Wuse-after-free [BZ #26779] | expand

Commit Message

Martin Sebor Jan. 25, 2022, 10:50 p.m. UTC 
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");
>>   }
> 
>

Comments

Carlos O'Donell Jan. 26, 2022, 2:56 p.m. UTC | #1 
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");
>  }
Joseph Myers Jan. 28, 2022, 1:10 p.m. UTC | #2 
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);
      |       ^~~~~~~~~~~~~~~
Carlos O'Donell Jan. 28, 2022, 5:33 p.m. UTC | #3 
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.
Joseph Myers Jan. 28, 2022, 5:51 p.m. UTC | #4 
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).
Jeff Law Jan. 28, 2022, 11:21 p.m. UTC | #5 
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
Carlos O'Donell Jan. 31, 2022, 3:12 p.m. UTC | #6 
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 :-)
Joseph Myers Feb. 4, 2022, 8:40 p.m. UTC | #7 
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);
      |       ^~~~~~~~~~~~~~~
diff mbox series

Patch

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