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

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

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.