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 08D89385C407 for ; Tue, 25 Jan 2022 17:49:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 08D89385C407 Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-30-DRwPtC_SNjOBaIQIOcsp9Q-1; Tue, 25 Jan 2022 12:49:39 -0500 X-MC-Unique: DRwPtC_SNjOBaIQIOcsp9Q-1 Received: by mail-qt1-f197.google.com with SMTP id y1-20020ac87041000000b002c3db9c25f8so12566005qtm.5 for ; Tue, 25 Jan 2022 09:49:39 -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=l5LTMcO4e04wlDHixBPpl+8M2Tajj2C9J2Ay2aj+fb4=; b=7s9X/Q5azLdXk/AFcehIsxMM5EhYo0/mXc23U3xdGFooCJv2uNBqNSlkTkMjiqoBQT 9nMk2SKgFWAzW1YcEZoldILenoVU7vgGWz2kpaHEd8qYqy4UNo53ASrany/bqjeCIqsg mmjcFGOCiMkSkvQ99gwzIpPsdCbE0zezQBiatccU2llsl0SvpbS+tZ+QjDCT7Ekt36qI 71Z4iUlgM5Ishv9PcUsu6IInutiQnmB6PIr2Vd+ySqmWzpmEA4Bts5kc87JG5vFU12Ky UfcKpvXtc++hSAk7w8pNf/zugL3bIKdXTkj2ENiEJwe3QrC5Wqrc7/DjHA4B0LelkNwO LeLQ== X-Gm-Message-State: AOAM530ob1G31V2wzLmKYGBP+BoJ+Df4uSe3fVOehi789TP1AOnZzycF MTIeZmW2LuCaGQ5WucpukNPHGq2i1kvjdXIOn3BRDKew9O5aXpZ0iSf9OAdj0TbJ3NyOBfheF7x +7+XATo8kvBFpaYkG4Ub7 X-Received: by 2002:a05:6214:21c8:: with SMTP id d8mr8896303qvh.40.1643132978582; Tue, 25 Jan 2022 09:49:38 -0800 (PST) X-Google-Smtp-Source: ABdhPJxg7EYvoJQC+5LRP4j2RpNEbc7mhI6041MeJC3Xw58oK1Fy8nOy5/sXuM0SS5T3rAFXs4htEw== X-Received: by 2002:a05:6214:21c8:: with SMTP id d8mr8896282qvh.40.1643132978267; Tue, 25 Jan 2022 09:49:38 -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 66sm4677070qte.42.2022.01.25.09.49.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 25 Jan 2022 09:49:37 -0800 (PST) Message-ID: Date: Tue, 25 Jan 2022 12:49:36 -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 v2 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> From: Carlos O'Donell Organization: Red Hat In-Reply-To: <50baa5fb-c1ee-f7dc-b7d6-cf4587782062@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: Tue, 25 Jan 2022 17:49:43 -0000 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 > > #include > +#include 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 > #include > > +#include 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.