From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS17314 8.43.84.0/22 X-Spam-Status: No, score=-2.8 required=3.0 tests=AWL,BAYES_00,BODY_8BITS, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, NICE_REPLY_A,RCVD_IN_DNSWL_MED,RDNS_DYNAMIC,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 57ADE1F5AE for ; Thu, 6 May 2021 23:54:32 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id F101138618F9; Thu, 6 May 2021 23:54:30 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org F101138618F9 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1620345271; bh=DkJvDULRjRlYULWgMWgVaV32rj2kXLEVxxc74R1T9eQ=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=UGwdMpxmok6iX9l0tSnrVIPAG7WqDSlUjJswnOw5Blk4Q6Fzz8XNxqK9y+vT4MuNe KuJGRINlAqi4IXOm0AJU5eJSg8LSazTZcHfvNcfOkoI1cSM03Pax+T7IgNmXdDhafP n6YlklU6o4emjwxrujCESIVbHwuXRyQ8y33iNpwk= Received: from mail-qt1-x833.google.com (mail-qt1-x833.google.com [IPv6:2607:f8b0:4864:20::833]) by sourceware.org (Postfix) with ESMTPS id 4C9543857816 for ; Thu, 6 May 2021 23:54:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4C9543857816 Received: by mail-qt1-x833.google.com with SMTP id y12so5436660qtx.11 for ; Thu, 06 May 2021 16:54:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=DkJvDULRjRlYULWgMWgVaV32rj2kXLEVxxc74R1T9eQ=; b=IIXu2QFbKyfdJkegO+rMr5IldhA/+rSSHYqCSeoPpqh6cjboVYJCcFKvETnhAIryd5 4TTYPb5Xl3q97ZmA/XgOqP04mvWdm1fwUQ29qhaA/wCY4dZD1+0H1irYsYHCKUP9m2Bt l+hfxlAGObglwa9LqFdBtQeZKJJyJItq9hEmHZGlQaiA0XcEccrD0ocovGasgz5gbhNs NtvO7MkDv4LBsbl2L4eDb8Qw8hG5WzlBYOnHezYUdzpTbb2xzTDPZs6s+iAAfx+slWJi 54yxl9z4FgH58PvbhXBMf0O/LBTC1cSzOD0M/GAmxTr8cv+no0IQ4NDOnWB1Y8+uyDSd ybCQ== X-Gm-Message-State: AOAM531YPdJtuMEIpvM01jC9b7APA0IgZ8ZqXtvvbKZcSoUvlh7RrDEK +TMSrIvipYz1W9aZOLP9yHI= X-Google-Smtp-Source: ABdhPJwexnHidMFGcnIE4RgsW0S3OJCJ8Ivz48m5B2KLXyijCDLGSl9g3QERw/O0OsN8lrFVU8/57w== X-Received: by 2002:ac8:7dc1:: with SMTP id c1mr6637644qte.327.1620345267872; Thu, 06 May 2021 16:54:27 -0700 (PDT) Received: from [192.168.0.41] (71-218-14-121.hlrn.qwest.net. [71.218.14.121]) by smtp.gmail.com with ESMTPSA id c5sm3157337qkl.7.2021.05.06.16.54.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 06 May 2021 16:54:27 -0700 (PDT) Subject: Re: [PATCH] add support for -Wmismatched-dealloc To: Florian Weimer References: <74efece7-9a4b-83ee-7fdd-475c0d514378@gmail.com> <758e723b-67cf-a211-7bc2-2ccd3fc744e5@gmail.com> <2555516b-4583-21fc-e844-fd44619488cd@gmail.com> <655918b2-16c6-74b1-6a49-505a7607007f@gmail.com> <87mtxok7ob.fsf@oldenburg2.str.redhat.com> <0aae9006-6001-8fc8-ad6d-c8e3ee60f82c@gmail.com> <87turwiqqw.fsf@oldenburg2.str.redhat.com> <87czybsuoe.fsf@oldenburg2.str.redhat.com> <5ed3fe4d-c0e4-632f-2569-b60d29cf4995@gmail.com> Message-ID: Date: Thu, 6 May 2021 17:54:25 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <5ed3fe4d-c0e4-632f-2569-b60d29cf4995@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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: , From: Martin Sebor via Libc-alpha Reply-To: Martin Sebor Cc: libc-alpha@sourceware.org, Joseph Myers Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" If there is no further discussion I'll retest this patch with the top of the trunk and a few recent GCC releases and if no problems turn up plan to commit it next week. Please let me know if you have any concerns. (I already know about the bug in the dummy definition of __attr_dealloc(x) with GCC versions prior: it only takes one argument instead of two.) On 4/22/21 6:00 PM, Martin Sebor wrote: > Attached is a revised patch that does not apply the new attribute > malloc to realpath.  It also adds a new test to verify that calling > a deallocator other than free on the result of realpath() with > the resolved_path obtained from the corresponding allocator doesn't > trigger a warning.  This is a trade-off between false positives and > negatives: there new attribute isn't expressive enough to specify > that a function like realpath returns a pointer allocated by malloc > (when the second argument is null) or its argument otherwise. > > I also made sure that in the modified declarations __THROW comes > before attributes and not after (as Florian pointed out is required > in C++) and verified by compiling the modified headers with G++ 11. > > Otherwise this patch is unchanged from the last version. > > Martin > > On 1/11/21 5:01 PM, Martin Sebor wrote: >> On 1/11/21 2:13 AM, Florian Weimer wrote: >> ... >> >> The attached revision has the changes below. >> >>>> diff --git a/libio/tst-freopen.c b/libio/tst-freopen.c >>>> index baa13166fe..c4df29d171 100644 >>>> --- a/libio/tst-freopen.c >>>> +++ b/libio/tst-freopen.c >>>> @@ -81,6 +81,36 @@ do_test_basic (void) >>>>     fclose (f); >>>>   } >>>> +#if defined __GNUC__ && __GNUC__ >= 11 >>>> +#pragma GCC diagnostic push >>>> +#pragma GCC diagnostic error "-Wmismatched-dealloc" >>>> +#endif >>> >>> Please add a comment referencing the fclose in the test below. >> >> Done. >> >>> >>>> +  /* This shouldn't trigger -Wmismatched-dealloc.  */ >>>> +  fclose (f1); >>> >>>> +#if defined __GNUC__ && __GNUC__ >= 11 >>>> +# pragma GCC diagnostic pop >>>> +#endif >>> >>> Likewise. >> >> Ditto. >> >>> >>>> diff --git a/libio/tst-wmemstream1.c b/libio/tst-wmemstream1.c >>>> index f8b308bc6c..8a0e253862 100644 >>>> --- a/libio/tst-wmemstream1.c >>>> +++ b/libio/tst-wmemstream1.c >>>> @@ -1,5 +1,40 @@ >>>>   #include >>>> +/* Verify that calling fclose on the result of open_wmemstream doesn't >>>> +   trigger GCC -Wmismatched-dealloc with fclose forward-declared and >>>> +   without included first (it is included later, in. >>>> +   "tst-memstream1.c").  */ >>>> + >>>> +extern int fclose (FILE*); >>>> + >>>> +#if defined __GNUC__ && __GNUC__ >= 11 >>>> +#pragma GCC diagnostic push >>>> +#pragma GCC diagnostic error "-Wmismatched-dealloc" >>>> +#endif >>> >>> Likewise. >> >> The comment is just above so I moved it down to the #if block. >> >>> >>>> +#if defined __GNUC__ && __GNUC__ >= 11 >>>> +# pragma GCC diagnostic pop >>>> +#endif >>> >>> Likewise. >>> >>>> diff --git a/libio/tst-wmemstream5.c b/libio/tst-wmemstream5.c >>>> new file mode 100644 >>>> index 0000000000..64461dbe48 >>>> --- /dev/null >>>> +++ b/libio/tst-wmemstream5.c >>>> @@ -0,0 +1,40 @@ >>> >>> Missing file header. >> >> I copied tst-wmemstream1.c which doesn't have a header either, but >> I found one elsewhere so I added it to the new test. >> >>> >>>> +#include >>>> + >>>> +/* Verify that calling fclose on the result of open_wmemstream doesn't >>>> +   trigger GCC -Wmismatched-dealloc with fclose forward-declared and >>>> +   without included in the same translation unit.  */ >>>> + >>>> +extern int fclose (FILE*); >>>> + >>>> +#if defined __GNUC__ && __GNUC__ >= 11 >>>> +#pragma GCC diagnostic push >>>> +#pragma GCC diagnostic error "-Wmismatched-dealloc" >>>> +#endif >>> >>> Likewise. >> >> Okay. >> >>> >>>> diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h >>>> index 3aa27a9d25..f88f8e55b3 100644 >>>> --- a/stdlib/stdlib.h >>>> +++ b/stdlib/stdlib.h >>> >>>>   #if defined __USE_MISC || defined __USE_XOPEN_EXTENDED >>>> @@ -798,7 +804,8 @@ extern char *canonicalize_file_name (const char >>>> *__name) >>>>      ENAMETOOLONG; if the name fits in fewer than PATH_MAX chars, >>>>      returns the name in RESOLVED.  */ >>>>   extern char *realpath (const char *__restrict __name, >>>> -               char *__restrict __resolved) __THROW __wur; >>>> +               char *__restrict __resolved) __THROW >>>> +     __attr_dealloc_free __wur; >>>>   #endif >>> >>> realpath only returns a pointer to the heap if RESOLVED is null, so the >>> annotation is wrong here. >>> >> >> This is intentional.  When realpath() returns the last argument >> (when it's nonnull) passing the returned pointer to free will not >> be diagnosed but passing it to some other deallocator not associated >> with the function will be.  That means for example that passing >> a pointer allocated by C++ operator new() to realpath() and then >> deleting the pointer returned from the function as opposed to >> the argument will trigger a false positive.  I decided this was >> an okay trade-off because unless the function allocates memory >> I expect the returned pointer to be ignored (similarly to how >> the pointer returned from memcpy is ignored).  If you don't like >> the odds I can remove the attribute from the function until we >> have one that captures this conditional return value (I'd like >> to add one in GCC 12). >> >>>> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h >>>> index 9cf8b05a87..e31734158c 100644 >>>> --- a/wcsmbs/wchar.h >>>> +++ b/wcsmbs/wchar.h >>>> @@ -151,7 +151,8 @@ extern size_t wcsxfrm_l (wchar_t *__s1, const >>>> wchar_t *__s2, >>>>                size_t __n, locale_t __loc) __THROW; >>>>   /* Duplicate S, returning an identical malloc'd string.  */ >>>> -extern wchar_t *wcsdup (const wchar_t *__s) __THROW >>>> __attribute_malloc__; >>>> +extern wchar_t *wcsdup (const wchar_t *__s) __THROW >>>> +  __attribute_malloc__ __attr_dealloc_free; >>>>   #endif >>>>   /* Find the first occurrence of WC in WCS.  */ >>>> @@ -562,9 +563,23 @@ extern wchar_t *wcpncpy (wchar_t *__restrict >>>> __dest, >>>>   /* Wide character I/O functions.  */ >>>>   #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2) >>>> +# ifndef __attr_dealloc_fclose >>>> +#   if defined __has_builtin >>>> +#     if __has_builtin (__builtin_fclose) >>>> +/* If the attribute macro hasn't been defined yet (by ) and >>>> +   fclose is a built-in, use it.  */ >>>> +#      define __attr_dealloc_fclose __attr_dealloc >>>> (__builtin_fclose, 1) >>>> +#     endif >>>> +#   endif >>>> +# endif >>>> +# ifndef __attr_dealloc_fclose >>>> +#  define __attr_dealloc_fclose /* empty */ >>>> +# endif >>>> + >>>>   /* Like OPEN_MEMSTREAM, but the stream is wide oriented and produces >>>>      a wide character string.  */ >>>> -extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t >>>> *__sizeloc) __THROW; >>>> +extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t >>>> *__sizeloc) __THROW >>>> +  __attribute_malloc__ __attr_dealloc_fclose; >>>>   #endif >>>>   #if defined __USE_ISOC95 || defined __USE_UNIX98 >>> >>> Does this mean that if one includes first and then >>> to get the declaration of fclose, fclose is not registered as a >>> deallocator for open_wmemstream? >> >> Yes. >> >>> >>> Maybe it is possible to redeclare open_wmemstream in if >>> has already been included? >> >> It is, and I suggested that in my last reply but didn't implement >> it because I didn't expect it to be a palatable.  I've added it >> in the updated revision. >> >> Martin >