From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 4F1B13858411 for ; Wed, 26 Jan 2022 14:56:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4F1B13858411 Received: from mail-io1-f71.google.com (mail-io1-f71.google.com [209.85.166.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-630-tQuaEtGvMuWS2s9F9wsHww-1; Wed, 26 Jan 2022 09:56:49 -0500 X-MC-Unique: tQuaEtGvMuWS2s9F9wsHww-1 Received: by mail-io1-f71.google.com with SMTP id z28-20020a056602081c00b00611596ef96fso5924519iow.12 for ; Wed, 26 Jan 2022 06:56:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:organization:in-reply-to :content-transfer-encoding; bh=ESKcbwNGbgRJ1SfZ7m9RAWJYylRNP1yaQZA+JuUr+xQ=; b=2JL4QaElx9+5dPJdESoxCE2N1caZgXlFiW9osHifO0mRx6GmeJJb25zNR0uw2+0QZL cB9egPXf7HsWebS76NeymXKDB5hFXbdpbdZU4Z8fMHW6xfjXPtbgYW7SuiuerPAkxGPN RD71qGRHruS8amd6Xs2d+m2pEj7pTj5xvGdGFom0B3J6VwgBtKSVzTKC+82cx6prd6Ws dq2dLI8oB8ghteekX1PWBoa09+VzKVFJl2omMDr/y6vP61tpVL9DR+FG1+x+Jw2+Dt2x Le/xH0JFmExFJ2wr/Exw9rRpKHiXEvD8d14bp299eEm2BU9aevaW3iZAPWqHtrGEI6m1 NQ5A== X-Gm-Message-State: AOAM531aQ4kO6ANEG+vGOmzTIS4lWD0JmNPdeSLoDeNIp2nsyMiYb/wv 4ATxXnaGX1Hs6icpeDkm36Ff6zrp+vNA4RZyxYSxUrlTwcPjcyYN4MBemKX+iwNhw+ZFkG4qSoW OGn3CBJdQheOF9ML/K81f X-Received: by 2002:a02:2207:: with SMTP id o7mr12093086jao.99.1643209008817; Wed, 26 Jan 2022 06:56:48 -0800 (PST) X-Google-Smtp-Source: ABdhPJwUXP/GMX0WsPu8bSz2Pkw1XaX+f456VqNQrOICbK6tGCejp9P37y0Y6GgBKNQa4LAY8m4TJQ== X-Received: by 2002:a02:2207:: with SMTP id o7mr12093072jao.99.1643209008494; Wed, 26 Jan 2022 06:56:48 -0800 (PST) Received: from [192.168.0.241] (135-23-175-80.cpe.pppoe.ca. [135.23.175.80]) by smtp.gmail.com with ESMTPSA id e17sm10440196ilm.67.2022.01.26.06.56.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 Jan 2022 06:56:47 -0800 (PST) Message-ID: <7b1e2fda-b440-ad77-4586-7e73b7c927b0@redhat.com> Date: Wed, 26 Jan 2022 09:56:46 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH v3 5/5] avoid -Wuse-after-free [BZ #26779] To: Martin Sebor , libc-alpha@sourceware.org References: <53f20975-a2c9-674d-2a43-b1b323ee545c@gmail.com> <50baa5fb-c1ee-f7dc-b7d6-cf4587782062@gmail.com> <3ad704bb-a061-1dc2-3b6d-8343f70a3c92@gmail.com> From: Carlos O'Donell Organization: Red Hat In-Reply-To: <3ad704bb-a061-1dc2-3b6d-8343f70a3c92@gmail.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Jan 2022 14:56:53 -0000 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 > commit c23cf7ff784186b8094b97a47a8445808145a69c > Author: Martin Sebor > 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 > > #include > +#include > > #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.