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

[Carlos O'Donell <carlos@redhat.com>]

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.