[PATCH v3 5/5] avoid -Wuse-after-free [BZ #26779]
Martin Sebor
msebor@gmail.com
Tue Jan 25 22:50:26 GMT 2022
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");
>> }
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: glibc-26779-5.diff
Type: text/x-patch
Size: 5267 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20220125/15657bbf/attachment.bin>
More information about the Libc-alpha
mailing list