Message ID | e77eb212-503f-1032-80ee-211346c33610@gmail.com |
---|---|
State | Committed |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5F79A3851C17; Tue, 8 Dec 2020 22:53:04 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5F79A3851C17 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1607467984; bh=fXngpkYqK3nMNneG0yAKh4Oy0TkyD44E7r8DNsLKsVE=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=ys59wjpSiUNe3A8aM7w+74TSxoXTVxFNRRRfu2H8DpXv/gnx9eOCtlPvClJXZGhLa rtIJkDlujpM7Lp3JR+xAiC7Yxkl7o/I2ELT1NpKHv3ppI2m7F/bhR3VzrHdMaPwuDn NnOgMmxxAb6KQttXaTMCbPm1ossm3gdqefuYEhX0= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-qt1-x82c.google.com (mail-qt1-x82c.google.com [IPv6:2607:f8b0:4864:20::82c]) by sourceware.org (Postfix) with ESMTPS id 8F7923857003 for <libc-alpha@sourceware.org>; Tue, 8 Dec 2020 22:53:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8F7923857003 Received: by mail-qt1-x82c.google.com with SMTP id k4so12047qtj.10 for <libc-alpha@sourceware.org>; Tue, 08 Dec 2020 14:53:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:from:subject:message-id:date:user-agent :mime-version:content-language; bh=fXngpkYqK3nMNneG0yAKh4Oy0TkyD44E7r8DNsLKsVE=; b=nT44pNKwYtCWbP6kIESkhHZWq5koZC0zJ93/PTfC6NHRuQYWdUX7hZwCXbGd0cVWp8 b0ABmNpcBQWFvMTcA1UiTsDMIYsS28O6kXRF+Uqy3fd34887EbcNAUoGLd5aFCff20V9 4x5hEp1Ru9zUuvtLjaei4dKtd6sg/JYS4sgpBsvbVI0WJuRlg6XDuQ7JeXBKXC/jXXtX WX/7WkGiPkuHToOTFsMnc3o0iEWugdN7okvx3aLnFRma1JSrNJz0OJaAmQ3HaDm210P/ cj7N4tOdYFSkwV88kk0sADWWlXFzCJ9fTeWdsdA+3UUoYHBFEYizHZf6YV/Pqt1tQFu1 gc9A== X-Gm-Message-State: AOAM532Rgu6nFYlVX1HCjkfBsmOKkMhM7XPfgzUdDzPg8nabi8xiyQwI MpOm/B1F8+7NTt21QKRpPUCN5wZa0ME= X-Google-Smtp-Source: ABdhPJwHFvo4Guor3V1SrRpE/1LwvRAVAIPaDMYlnvMH+dlIdTNrl6RS0XwcHtmrjzQZh2r61XHCzw== X-Received: by 2002:aed:2084:: with SMTP id 4mr393551qtb.81.1607467980961; Tue, 08 Dec 2020 14:53:00 -0800 (PST) Received: from [192.168.0.41] (174-16-97-231.hlrn.qwest.net. [174.16.97.231]) by smtp.gmail.com with ESMTPSA id 6sm124213qko.3.2020.12.08.14.53.00 for <libc-alpha@sourceware.org> (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 08 Dec 2020 14:53:00 -0800 (PST) To: GNU C Library <libc-alpha@sourceware.org> Subject: [PATCH] add support for -Wmismatched-dealloc Message-ID: <e77eb212-503f-1032-80ee-211346c33610@gmail.com> Date: Tue, 8 Dec 2020 15:52:59 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------85E58D6BC741430B71D29B0B" Content-Language: en-US X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: Martin Sebor via Libc-alpha <libc-alpha@sourceware.org> Reply-To: Martin Sebor <msebor@gmail.com> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
add support for -Wmismatched-dealloc
|
|
Commit Message
Martin Sebor
Dec. 8, 2020, 10:52 p.m. UTC
To help detect common kinds of memory (and other resource) management bugs, GCC 11 adds support for the detection of mismatched calls to allocation and deallocation functions. At each call site to a known deallocation function GCC checks the set of allocation functions the former can be paired with and, if the two don't match, issues a -Wmismatched-dealloc warning (something similar happens in C++ for mismatched calls to new and delete). GCC also uses the same mechanism to detect attempts to deallocate objects not allocated by any allocation function (or pointers past the first byte into allocated objects) by -Wfree-nonheap-object. This support is enabled for built-in functions like malloc and free. To extend it beyond those, GCC extends attribute malloc to designate a deallocation function to which pointers returned from the allocation function may be passed to deallocate the allocated objects. Another, optional argument designates the positional argument to which the pointer must be passed. The attached change is the first step in enabling this extended support for Glibc. It enables GCC to diagnose mismatched calls such as in tst-popen.c1: FILE *f = popen ("foo", "r"); ... fclose (f); warning: ‘fclose’ called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc] 83 | fclose (f); | ^~~~~~~~~~ note: returned from ‘popen’ 81 | FILE *f = popen ("foo", "r"); | ^~~~~~~~~~~~~~~~~~ Since <stdio.h> doesn't declare free() and realloc() the patch uses __builtin_free and __builtin_realloc as the deallocators. This isn't pretty but I couldn't think of a more elegant way to do it. (I also missed this bit in the initial implementation of the attribute so current trunk doesn't handle the __builtin_xxx forms. I submitted a patch to handle it just today, but to avoid delays, I post this solution for comments ahead of its acceptance). Since the deallocation function referenced by the malloc attribute must have been declared, the changes require rearranging the order of the declarations a bit. I tested the changes by rebuilding Glibc on x86_64 and by verifying that warnings are issued where appropriate for my little test program but not much else. Neither set of changes (either in GCC or in Glibc) impacts code generation and should make no difference in testing (I didn't see any.) If there is support for this approach I'll work on extending it to other headers, hopefully before the upcoming Glibc release. Thanks Martin
Comments
I don't see any definition of __attr_dealloc (presumably should be a macro in misc/sys/cdefs.h) in this patch (or in the glibc source tree). Given all the functions in stdio.h with the same list of deallocation functions, there should probably be another macro there to expand to __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) __attr_dealloc_freopen64. (That would also apply to open_wmemstream in wchar.h, but I suppose you have the issue there with functions not being declared in wchar.h, only in stdio.h?)
On 12/8/20 5:07 PM, Joseph Myers wrote: > I don't see any definition of __attr_dealloc (presumably should be a macro > in misc/sys/cdefs.h) in this patch (or in the glibc source tree). Whoops! > > Given all the functions in stdio.h with the same list of deallocation > functions, there should probably be another macro there to expand to > __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, > 3) __attr_dealloc_freopen64. (That would also apply to open_wmemstream in > wchar.h, but I suppose you have the issue there with functions not being > declared in wchar.h, only in stdio.h?) You're right that adding the attribute to open_wmemstream runs into the same problem as declaring the ctermid argument with L_ctermid elements in <unistd.h>: fclose isn't declared in <wchar.h>. I did some more work on adding the attribute to other functions and found out that the same problem also happens between tempnam() in <stdio.h> and reallocarray() in <stdlib.h>. I spent some time working around this but in the end it turned out to be too convoluted so I decided to make the attribute a little smarter. Instead of associating all allocation functions with all deallocation functions (such as fdopen, fopen, fopen64, etc. with fclose, freopen, and freopen64) I changed it so that an allocator only needs to be associated with a single deallocator (a reallocator also needs to be associated with itself). That makes things quite a bit simpler. The attached patch implements this for <stdio.h>, <stdlib.h>, and <wchar.h>. To get around the <wchar.h> dependency on <stdio.h> it uses __REDIRECT to introduce a reserved alias for fclose. Besides running the test suite I tested it with my own test and also by adding the same declarations to the GCC test suite and verifying it triggers warnings as expected. The GCC patches needed to make this simpler scheme work haven't been reviewed yet so this work has a dependency on them getting approved. I grepped for __attribute_malloc__ in Glibc headers to see if there are other APIs that would benefit from the same annotation but found none. At the same time, I don't have the impression that malloc is used on all the APIs it could be. Are there any that you or anyone else can think of that might be worth looking at? Martin
On 12/11/20 7:25 PM, Martin Sebor wrote: > On 12/8/20 5:07 PM, Joseph Myers wrote: >> I don't see any definition of __attr_dealloc (presumably should be a >> macro >> in misc/sys/cdefs.h) in this patch (or in the glibc source tree). > > Whoops! > >> >> Given all the functions in stdio.h with the same list of deallocation >> functions, there should probably be another macro there to expand to >> __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, >> 3) __attr_dealloc_freopen64. (That would also apply to >> open_wmemstream in >> wchar.h, but I suppose you have the issue there with functions not being >> declared in wchar.h, only in stdio.h?) > > You're right that adding the attribute to open_wmemstream runs into > the same problem as declaring the ctermid argument with L_ctermid > elements in <unistd.h>: fclose isn't declared in <wchar.h>. I did > some more work on adding the attribute to other functions and found > out that the same problem also happens between tempnam() in <stdio.h> > and reallocarray() in <stdlib.h>. > > I spent some time working around this but in the end it turned out > to be too convoluted so I decided to make the attribute a little > smarter. Instead of associating all allocation functions with all > deallocation functions (such as fdopen, fopen, fopen64, etc. with > fclose, freopen, and freopen64) I changed it so that an allocator > only needs to be associated with a single deallocator (a reallocator > also needs to be associated with itself). That makes things quite > a bit simpler. > > The attached patch implements this for <stdio.h>, <stdlib.h>, and > <wchar.h>. To get around the <wchar.h> dependency on <stdio.h> it > uses __REDIRECT to introduce a reserved alias for fclose. > > Besides running the test suite I tested it with my own test and also > by adding the same declarations to the GCC test suite and verifying > it triggers warnings as expected. > > The GCC patches needed to make this simpler scheme work haven't been > reviewed yet so this work has a dependency on them getting approved. The GCC patches have now been committed and the dependency resolved. Florian asked this morning if getaddrinfo() and freeaddrinfo() are covered by this change. They're not because getaddrinfo() returns the allocated memory indirectly, via an argument. To handle those kinds of APIs that return a pointer to the allocated object indirectly, through an argument, the attribute will need to be enhanced somehow. > > I grepped for __attribute_malloc__ in Glibc headers to see if there > are other APIs that would benefit from the same annotation but found > none. At the same time, I don't have the impression that malloc is > used on all the APIs it could be. Are there any that you or anyone > else can think of that might be worth looking at? > > Martin
* Martin Sebor: > Florian asked this morning if getaddrinfo() and freeaddrinfo() are > covered by this change. They're not because getaddrinfo() returns > the allocated memory indirectly, via an argument. To handle those > kinds of APIs that return a pointer to the allocated object > indirectly, through an argument, the attribute will need to be > enhanced somehow. asprintf is another such function, perhaps slightly more commonly used. It would be nice if this interface pattern could be covered as well. Thanks, Florian
On Mon, 14 Dec 2020, Martin Sebor via Libc-alpha wrote: > > I spent some time working around this but in the end it turned out > > to be too convoluted so I decided to make the attribute a little > > smarter. Instead of associating all allocation functions with all > > deallocation functions (such as fdopen, fopen, fopen64, etc. with > > fclose, freopen, and freopen64) I changed it so that an allocator > > only needs to be associated with a single deallocator (a reallocator > > also needs to be associated with itself). That makes things quite > > a bit simpler. [...] > The GCC patches have now been committed and the dependency resolved. I've looked at the attribute documentation now in GCC, but I'm afraid I'm unable to understand from that documentation why the proposed glibc patch constitutes a valid way of specifying that, for example, it's valid to use freopen as a deallocator for FILE pointers opened by functions whose attribute only mentions fclose. Unless there's something I'm missing in the documentation or a separate documentation patch that's not yet committed, I think more work is needed on the GCC documentation to make clear the semantics the glibc patch is asserting for valid combinations of allocators and deallocators, so that those semantics can be reviewed for correctness.
On 12/14/20 6:01 PM, Joseph Myers wrote: > On Mon, 14 Dec 2020, Martin Sebor via Libc-alpha wrote: > >>> I spent some time working around this but in the end it turned out >>> to be too convoluted so I decided to make the attribute a little >>> smarter. Instead of associating all allocation functions with all >>> deallocation functions (such as fdopen, fopen, fopen64, etc. with >>> fclose, freopen, and freopen64) I changed it so that an allocator >>> only needs to be associated with a single deallocator (a reallocator >>> also needs to be associated with itself). That makes things quite >>> a bit simpler. > [...] >> The GCC patches have now been committed and the dependency resolved. > > I've looked at the attribute documentation now in GCC, but I'm afraid I'm > unable to understand from that documentation why the proposed glibc patch > constitutes a valid way of specifying that, for example, it's valid to use > freopen as a deallocator for FILE pointers opened by functions whose > attribute only mentions fclose. Unless there's something I'm missing in > the documentation or a separate documentation patch that's not yet > committed, I think more work is needed on the GCC documentation to make > clear the semantics the glibc patch is asserting for valid combinations of > allocators and deallocators, so that those semantics can be reviewed for > correctness. I flip-flopped with freopen. Initially I wanted to mark it up as both an allocator and a deallocator, analogously to realloc (which is implicitly both) or reallocarray (which is annotated as both in the latest Glibc patch). Both the initial Glibc and GCC patches (the manual for the latter) reflected this and had freopen annotated that way. But because freopen doesn't actually deallocate or allocate a stream the markup wouldn't be correct. It would cause false positives with -Wmismatched-dealloc as well with other warnings like the future -Wuse-after-free (or with -Wanalyzer-use-after-free when the GCC analyzer adds support for the attribute that David Malcolm is working on for GCC 11). I've added a test case to the test suite: void f (FILE *f1) { FILE *f2 = freopen ("", "", f1); fclose (f1); // must not warn } To answer your question, without the attribute freopen is seen by GCC as an ordinary function that happens to take a FILE* and return another FILE*. It neither allocates it nor deallocates it. For GCC 12, I'd like us to consider adding attribute returns_arg(position) to improve the analysis here. The GCC manual also doesn't mention freopen anymore but I'd be happy to change the example there to show an API that does include a reallocator (e.g., reallocarray). Having said all this, after double-checking the latest Glibc patch I see it still has the attribute on freopen by mistake (as well as the ordinary attribute malloc, which would make it even worse). I've removed both in the attached revision. Sorry if this confused you -- freopen obviously confused me. Martin
More testing made me realize that further changes are needed: 1) correct the return value of the __fclose() alias to int, 2) declare and use the same alias for fclose in both <stdio.h> and <wchar.h>. In addition, I noticed a few more opportunities to use the new attribute: * in include/programs/xmalloc.h, * in malloc/malloc.h, * and in wcsdup in <wchar.h>. I also simplified the new macro definitions a bit, and added a new test to verify that the warning doesn't cause false positives for open_wmemstream. Attached is a patch with these updates. On 12/15/20 9:52 AM, Martin Sebor wrote: > On 12/14/20 6:01 PM, Joseph Myers wrote: >> On Mon, 14 Dec 2020, Martin Sebor via Libc-alpha wrote: >> >>>> I spent some time working around this but in the end it turned out >>>> to be too convoluted so I decided to make the attribute a little >>>> smarter. Instead of associating all allocation functions with all >>>> deallocation functions (such as fdopen, fopen, fopen64, etc. with >>>> fclose, freopen, and freopen64) I changed it so that an allocator >>>> only needs to be associated with a single deallocator (a reallocator >>>> also needs to be associated with itself). That makes things quite >>>> a bit simpler. >> [...] >>> The GCC patches have now been committed and the dependency resolved. >> >> I've looked at the attribute documentation now in GCC, but I'm afraid I'm >> unable to understand from that documentation why the proposed glibc patch >> constitutes a valid way of specifying that, for example, it's valid to >> use >> freopen as a deallocator for FILE pointers opened by functions whose >> attribute only mentions fclose. Unless there's something I'm missing in >> the documentation or a separate documentation patch that's not yet >> committed, I think more work is needed on the GCC documentation to make >> clear the semantics the glibc patch is asserting for valid >> combinations of >> allocators and deallocators, so that those semantics can be reviewed for >> correctness. > > I flip-flopped with freopen. Initially I wanted to mark it up as > both an allocator and a deallocator, analogously to realloc (which > is implicitly both) or reallocarray (which is annotated as both in > the latest Glibc patch). Both the initial Glibc and GCC patches > (the manual for the latter) reflected this and had freopen annotated > that way. > > But because freopen doesn't actually deallocate or allocate a stream > the markup wouldn't be correct. It would cause false positives with > -Wmismatched-dealloc as well with other warnings like the future > -Wuse-after-free (or with -Wanalyzer-use-after-free when the GCC > analyzer adds support for the attribute that David Malcolm is > working on for GCC 11). I've added a test case to the test suite: > > void f (FILE *f1) > { > FILE *f2 = freopen ("", "", f1); > fclose (f1); // must not warn > } > > To answer your question, without the attribute freopen is seen by > GCC as an ordinary function that happens to take a FILE* and return > another FILE*. It neither allocates it nor deallocates it. For > GCC 12, I'd like us to consider adding attribute returns_arg(position) > to improve the analysis here. The GCC manual also doesn't mention > freopen anymore but I'd be happy to change the example there to > show an API that does include a reallocator (e.g., reallocarray). > > Having said all this, after double-checking the latest Glibc patch > I see it still has the attribute on freopen by mistake (as well as > the ordinary attribute malloc, which would make it even worse). > I've removed both in the attached revision. Sorry if this confused > you -- freopen obviously confused me. > > Martin
Florian/Joseph and/or others: is the latest patch okay to commit? https://sourceware.org/pipermail/libc-alpha/2020-December/121121.html On 12/27/20 4:13 PM, Martin Sebor wrote: > More testing made me realize that further changes are needed: > 1) correct the return value of the __fclose() alias to int, > 2) declare and use the same alias for fclose in both <stdio.h> > and <wchar.h>. > > In addition, I noticed a few more opportunities to use the new > attribute: > * in include/programs/xmalloc.h, > * in malloc/malloc.h, > * and in wcsdup in <wchar.h>. > > I also simplified the new macro definitions a bit, and added > a new test to verify that the warning doesn't cause false > positives for open_wmemstream. > > Attached is a patch with these updates. > > On 12/15/20 9:52 AM, Martin Sebor wrote: >> On 12/14/20 6:01 PM, Joseph Myers wrote: >>> On Mon, 14 Dec 2020, Martin Sebor via Libc-alpha wrote: >>> >>>>> I spent some time working around this but in the end it turned out >>>>> to be too convoluted so I decided to make the attribute a little >>>>> smarter. Instead of associating all allocation functions with all >>>>> deallocation functions (such as fdopen, fopen, fopen64, etc. with >>>>> fclose, freopen, and freopen64) I changed it so that an allocator >>>>> only needs to be associated with a single deallocator (a reallocator >>>>> also needs to be associated with itself). That makes things quite >>>>> a bit simpler. >>> [...] >>>> The GCC patches have now been committed and the dependency resolved. >>> >>> I've looked at the attribute documentation now in GCC, but I'm afraid >>> I'm >>> unable to understand from that documentation why the proposed glibc >>> patch >>> constitutes a valid way of specifying that, for example, it's valid >>> to use >>> freopen as a deallocator for FILE pointers opened by functions whose >>> attribute only mentions fclose. Unless there's something I'm missing in >>> the documentation or a separate documentation patch that's not yet >>> committed, I think more work is needed on the GCC documentation to make >>> clear the semantics the glibc patch is asserting for valid >>> combinations of >>> allocators and deallocators, so that those semantics can be reviewed for >>> correctness. >> >> I flip-flopped with freopen. Initially I wanted to mark it up as >> both an allocator and a deallocator, analogously to realloc (which >> is implicitly both) or reallocarray (which is annotated as both in >> the latest Glibc patch). Both the initial Glibc and GCC patches >> (the manual for the latter) reflected this and had freopen annotated >> that way. >> >> But because freopen doesn't actually deallocate or allocate a stream >> the markup wouldn't be correct. It would cause false positives with >> -Wmismatched-dealloc as well with other warnings like the future >> -Wuse-after-free (or with -Wanalyzer-use-after-free when the GCC >> analyzer adds support for the attribute that David Malcolm is >> working on for GCC 11). I've added a test case to the test suite: >> >> void f (FILE *f1) >> { >> FILE *f2 = freopen ("", "", f1); >> fclose (f1); // must not warn >> } >> >> To answer your question, without the attribute freopen is seen by >> GCC as an ordinary function that happens to take a FILE* and return >> another FILE*. It neither allocates it nor deallocates it. For >> GCC 12, I'd like us to consider adding attribute returns_arg(position) >> to improve the analysis here. The GCC manual also doesn't mention >> freopen anymore but I'd be happy to change the example there to >> show an API that does include a reallocator (e.g., reallocarray). >> >> Having said all this, after double-checking the latest Glibc patch >> I see it still has the attribute on freopen by mistake (as well as >> the ordinary attribute malloc, which would make it even worse). >> I've removed both in the attached revision. Sorry if this confused >> you -- freopen obviously confused me. >> >> Martin >
* Martin Sebor via Libc-alpha: > diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h > index 9cf8b05a87..4c1c7f1119 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,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict __dest, > /* Wide character I/O functions. */ > > #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2) > +# ifdef __REDIRECT > +/* Declare the __fclose alias and associate it as a deallocator > + with open_wmemstream below. */ > +extern int __REDIRECT (__fclose, (FILE *), fclose); > +# define __attr_dealloc_fclose __attr_dealloc (__fclose, 1) > +# else > +# 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 Why is an alias for fclose needed here, but not for free? Thanks, Florian
On 1/4/21 9:07 AM, Florian Weimer wrote: > * Martin Sebor via Libc-alpha: > >> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h >> index 9cf8b05a87..4c1c7f1119 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,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict __dest, >> /* Wide character I/O functions. */ >> >> #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2) >> +# ifdef __REDIRECT >> +/* Declare the __fclose alias and associate it as a deallocator >> + with open_wmemstream below. */ >> +extern int __REDIRECT (__fclose, (FILE *), fclose); >> +# define __attr_dealloc_fclose __attr_dealloc (__fclose, 1) >> +# else >> +# 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 > > Why is an alias for fclose needed here, but not for free? Because fclose is not a built-in so there's no __builtin_fclose to associate open_wmemstream with. free is a built-in and so __attr_dealloc_free just references __builtin_free and doesn't need an explicit declaration. Martin > > Thanks, > Florian >
* Martin Sebor: > On 1/4/21 9:07 AM, Florian Weimer wrote: >> * Martin Sebor via Libc-alpha: >> >>> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h >>> index 9cf8b05a87..4c1c7f1119 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,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict __dest, >>> /* Wide character I/O functions. */ >>> #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2) >>> +# ifdef __REDIRECT >>> +/* Declare the __fclose alias and associate it as a deallocator >>> + with open_wmemstream below. */ >>> +extern int __REDIRECT (__fclose, (FILE *), fclose); >>> +# define __attr_dealloc_fclose __attr_dealloc (__fclose, 1) >>> +# else >>> +# 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 >> Why is an alias for fclose needed here, but not for free? > > Because fclose is not a built-in so there's no __builtin_fclose > to associate open_wmemstream with. free is a built-in and so > __attr_dealloc_free just references __builtin_free and doesn't > need an explicit declaration. Ahh, that explains the discrepancy. I'm a bit worried that the __fclose alias causes problems. Would it be possible to add __builtin_fclose to GCC instead? Based on how this patch appears to make both __fclose and fclose acceptable as a deallocator, GCC resolves redirects as part of the matching check. I wonder if this constrains the usefulness of the attribute in some way. I can imagine situations where at the source level, different deallocators should be used (say to support debugging builds), but release builds redirect different deallocators to the same implementation. Thanks, Florian
On 1/4/21 9:57 AM, Florian Weimer wrote: > * Martin Sebor: > >> On 1/4/21 9:07 AM, Florian Weimer wrote: >>> * Martin Sebor via Libc-alpha: >>> >>>> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h >>>> index 9cf8b05a87..4c1c7f1119 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,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict __dest, >>>> /* Wide character I/O functions. */ >>>> #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2) >>>> +# ifdef __REDIRECT >>>> +/* Declare the __fclose alias and associate it as a deallocator >>>> + with open_wmemstream below. */ >>>> +extern int __REDIRECT (__fclose, (FILE *), fclose); >>>> +# define __attr_dealloc_fclose __attr_dealloc (__fclose, 1) >>>> +# else >>>> +# 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 >>> Why is an alias for fclose needed here, but not for free? >> >> Because fclose is not a built-in so there's no __builtin_fclose >> to associate open_wmemstream with. free is a built-in and so >> __attr_dealloc_free just references __builtin_free and doesn't >> need an explicit declaration. > > Ahh, that explains the discrepancy. > > I'm a bit worried that the __fclose alias causes problems. Would it be > possible to add __builtin_fclose to GCC instead? I don't like the alias hack either. Adding a built-in is possible but it's late in GCC stage 3 and I'm doubtful the change would be accepted before the Glibc deadline (that's this week, right?) The alias isn't necessary in <stdio.h> where fclose is declared so I've removed it from there. It would have been only marginally useful in <wchar.h>, and only when fclose isn't declared, but it's probably best avoided. So one possibility is to prepare the header for __builtin_fclose if/when it's added, fall back on fclose when it's declared (i.e., when <stdio.h> is included), and do nothing otherwise (and accept that calling, say free, on a pointer returned from open_wmemstteam, will not be diagnosed in those translation units). Attached is a patch that does that. If you want to change it to something else (e.g, forget about open_wmemstream altogether for now or conditionally declare it in <stdio.h> when <wchar.h> is included, or any other viable alternative) please let me know. > Based on how this patch appears to make both __fclose and fclose > acceptable as a deallocator, GCC resolves redirects as part of the > matching check. I wonder if this constrains the usefulness of the > attribute in some way. I can imagine situations where at the source > level, different deallocators should be used (say to support debugging > builds), but release builds redirect different deallocators to the same > implementation. The attribute doesn't do anything special with aliases (it was just a way to get around the problem with functions not being declared in some headers). As for different configurations, the attribute is designed for standard C/POSIX APIs and others like those. A user-defined API with different deallocators in one configuration than in another has to create the associations conditionally, based on those configurations. But there's no way to change these associations for GCC built-ins, just like there's no way to change their semantics. They're hardwired into GCC and the only way to affect them is to disable the built-ins. Martin > > Thanks, > Florian >
Florian, do you have any outstanding concerns or is the most recent patch good to go? Thanks Martin On 1/4/21 4:18 PM, Martin Sebor wrote: > On 1/4/21 9:57 AM, Florian Weimer wrote: >> * Martin Sebor: >> >>> On 1/4/21 9:07 AM, Florian Weimer wrote: >>>> * Martin Sebor via Libc-alpha: >>>> >>>>> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h >>>>> index 9cf8b05a87..4c1c7f1119 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,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict >>>>> __dest, >>>>> /* Wide character I/O functions. */ >>>>> #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2) >>>>> +# ifdef __REDIRECT >>>>> +/* Declare the __fclose alias and associate it as a deallocator >>>>> + with open_wmemstream below. */ >>>>> +extern int __REDIRECT (__fclose, (FILE *), fclose); >>>>> +# define __attr_dealloc_fclose __attr_dealloc (__fclose, 1) >>>>> +# else >>>>> +# 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 >>>> Why is an alias for fclose needed here, but not for free? >>> >>> Because fclose is not a built-in so there's no __builtin_fclose >>> to associate open_wmemstream with. free is a built-in and so >>> __attr_dealloc_free just references __builtin_free and doesn't >>> need an explicit declaration. >> >> Ahh, that explains the discrepancy. >> >> I'm a bit worried that the __fclose alias causes problems. Would it be >> possible to add __builtin_fclose to GCC instead? > > I don't like the alias hack either. Adding a built-in is possible > but it's late in GCC stage 3 and I'm doubtful the change would be > accepted before the Glibc deadline (that's this week, right?) > > The alias isn't necessary in <stdio.h> where fclose is declared > so I've removed it from there. It would have been only marginally > useful in <wchar.h>, and only when fclose isn't declared, but it's > probably best avoided. So one possibility is to prepare the header > for __builtin_fclose if/when it's added, fall back on fclose when > it's declared (i.e., when <stdio.h> is included), and do nothing > otherwise (and accept that calling, say free, on a pointer returned > from open_wmemstteam, will not be diagnosed in those translation > units). > > Attached is a patch that does that. If you want to change it > to something else (e.g, forget about open_wmemstream altogether > for now or conditionally declare it in <stdio.h> when <wchar.h> > is included, or any other viable alternative) please let me know. > >> Based on how this patch appears to make both __fclose and fclose >> acceptable as a deallocator, GCC resolves redirects as part of the >> matching check. I wonder if this constrains the usefulness of the >> attribute in some way. I can imagine situations where at the source >> level, different deallocators should be used (say to support debugging >> builds), but release builds redirect different deallocators to the same >> implementation. > > The attribute doesn't do anything special with aliases (it was > just a way to get around the problem with functions not being > declared in some headers). > > As for different configurations, the attribute is designed for > standard C/POSIX APIs and others like those. A user-defined API > with different deallocators in one configuration than in another > has to create the associations conditionally, based on those > configurations. But there's no way to change these associations > for GCC built-ins, just like there's no way to change their > semantics. They're hardwired into GCC and the only way to > affect them is to disable the built-ins. > > Martin > >> >> Thanks, >> Florian >> >
* Martin Sebor: > I don't like the alias hack either. Adding a built-in is possible > but it's late in GCC stage 3 and I'm doubtful the change would be > accepted before the Glibc deadline (that's this week, right?) I think we still have some time. One problem is that the annotations do not permit diagnosing memory leaks: realloc is a deallocator for malloc, xfclose is one for fopen, and so on. But the annotations do not capture this. So if they are used for diagnosing leaks, false positives will be a result. Listing all potential deallocators in the allocator does not scale. It also leads to the problem shown by open_wmemstream. I think allocators need to assign some sort of pool identifier to allocated pointers, and deallocators should declare on which pools they operate. This would allow adding additional deallocators such as xfclose. >> Based on how this patch appears to make both __fclose and fclose >> acceptable as a deallocator, GCC resolves redirects as part of the >> matching check. I wonder if this constrains the usefulness of the >> attribute in some way. I can imagine situations where at the source >> level, different deallocators should be used (say to support debugging >> builds), but release builds redirect different deallocators to the same >> implementation. > > The attribute doesn't do anything special with aliases (it was > just a way to get around the problem with functions not being > declared in some headers). But it has to do something with them, otherwise __fclose and fclose are not the same deallocator and thus different functions. > diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h > index 5fb6e309be..21e6177fac 100644 > --- a/misc/sys/cdefs.h > +++ b/misc/sys/cdefs.h > @@ -569,6 +569,17 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf > # define __attr_access(x) > #endif > > +#if __GNUC_PREREQ (11, 0) > +/* Designates dealloc as a function to call to deallocate objects > + allocated by the declared function. */ > +# define __attr_dealloc(dealloc, argno) \ > + __attribute__ ((__malloc__ (dealloc, argno))) > +# define __attr_dealloc_free __attr_dealloc (__builtin_free, 1) > +#else > +# define __attr_dealloc(x) > +# define __attr_dealloc_free > +#endif If the GCC attribute is called “malloc”, the macro name should reflect that. I think it would make sense to make __attribute_malloc__ redundant, so that the headers contain only one of the two attributes per declaration. The new macro could expand to the argument-less version for older GCC versions. > 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. > + /* This shouldn't trigger -Wmismatched-dealloc. */ > + fclose (f1); > +#if defined __GNUC__ && __GNUC__ >= 11 > +# pragma GCC diagnostic pop > +#endif Likewise. > 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 <wchar.h> > > +/* Verify that calling fclose on the result of open_wmemstream doesn't > + trigger GCC -Wmismatched-dealloc with fclose forward-declared and > + without <stdio.h> 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. > +#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. > +#include <wchar.h> > + > +/* Verify that calling fclose on the result of open_wmemstream doesn't > + trigger GCC -Wmismatched-dealloc with fclose forward-declared and > + without <stdio.h> 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. > 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. > 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 <stdio.h>) 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 <wchar.h> first and then <stdio.h> to get the declaration of fclose, fclose is not registered as a deallocator for open_wmemstream? Maybe it is possible to redeclare open_wmemstream in <stdio.h> if <wchar.h> has already been included? Thanks, Florian
On 1/11/21 2:13 AM, Florian Weimer wrote: > * Martin Sebor: > >> I don't like the alias hack either. Adding a built-in is possible >> but it's late in GCC stage 3 and I'm doubtful the change would be >> accepted before the Glibc deadline (that's this week, right?) > > I think we still have some time. > > One problem is that the annotations do not permit diagnosing memory > leaks: realloc is a deallocator for malloc, xfclose is one for fopen, > and so on. But the annotations do not capture this. So if they are > used for diagnosing leaks, false positives will be a result. Listing > all potential deallocators in the allocator does not scale. It also > leads to the problem shown by open_wmemstream. Florian and I discussed this in IRC and cleared most things up so this is largely just for the benefit of others. I'll post a new patch with the suggested changes separately from this. The attribute is used to detect other problems besides allocator and deallocator mismatches (e.g., -Wfree-nonheap-object), and should also already be in principle used by analyzer warnings like -Wanalyzer-double-fclose and -Wanalyzer-double-free. David Malcolm prototyped a similar attribute in the analyzer first and now is in the process of adjusting the analyzer to work with this one. The open_wmemstream problem is due to the <wchar.h> header not having access to the fclose declaration (because it's only in <stdio.h>). Declaring open_wmemstream with the attribute also in <stdio.h> (when <wchar.h> is included) solves it. Finally [and this qualifies what I said to Florian this morning], GCC makes it possible to avoid having to associate every allocator with every matching deallocator for reallocators only (like realloc). This was done to simplify its use in Glibc headers. But no such assumption is made about straight (non-deallocating) allocators and they do have to be explicitly associated with all their deallocators. > I think allocators need to assign some sort of pool identifier to > allocated pointers, and deallocators should declare on which pools they > operate. This would allow adding additional deallocators such as > xfclose. Other than the simplification for realloc there's no notion of a "pool." Supporting it wasn't a goal (the subject of the request in GCC PR 94527). The design could be relatively easily changed to avoid having to associate every allocator with every deallocator but almost certainly not for GCC 11. >>> Based on how this patch appears to make both __fclose and fclose >>> acceptable as a deallocator, GCC resolves redirects as part of the >>> matching check. I wonder if this constrains the usefulness of the >>> attribute in some way. I can imagine situations where at the source >>> level, different deallocators should be used (say to support debugging >>> builds), but release builds redirect different deallocators to the same >>> implementation. >> >> The attribute doesn't do anything special with aliases (it was >> just a way to get around the problem with functions not being >> declared in some headers). > > But it has to do something with them, otherwise __fclose and fclose are > not the same deallocator and thus different functions. Yes, they are different. That's also why I agree it's not a good solution. I've snipped the rest of your comments and replied to them separately to make it easier to focus on just the code changes. Martin
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 <wchar.h> >> >> +/* Verify that calling fclose on the result of open_wmemstream doesn't >> + trigger GCC -Wmismatched-dealloc with fclose forward-declared and >> + without <stdio.h> 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 <wchar.h> >> + >> +/* Verify that calling fclose on the result of open_wmemstream doesn't >> + trigger GCC -Wmismatched-dealloc with fclose forward-declared and >> + without <stdio.h> 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 <stdio.h>) 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 <wchar.h> first and then <stdio.h> > 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 <stdio.h> if > <wchar.h> 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
* Martin Sebor via Libc-alpha: >> 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). Maybe David can comment on how this interacts with his static analyzer work. In all other cases, the attribute means that the pointer needs to be freed to avoid a resource leak. If we suddenly apply it pointers which can only conditionally be freed, that reduces the value of those annotations, I think. Thanks, Florian
David, now that you've committed the analyzer support for the extended attribute malloc, can you please comment on Florian's concern about using it to annotate realpath() as in the patch below? https://sourceware.org/pipermail/libc-alpha/2021-January/121527.html (I tested how it interacts with -Wmismatched-dealloc but I haven't yet had a chance to see how the annotations might interact with the analyzer warnings.) Thanks Martin On 1/12/21 1:59 AM, Florian Weimer wrote: > * Martin Sebor via Libc-alpha: > >>> 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). > > Maybe David can comment on how this interacts with his static analyzer > work. In all other cases, the attribute means that the pointer needs to > be freed to avoid a resource leak. If we suddenly apply it pointers > which can only conditionally be freed, that reduces the value of those > annotations, I think. > > Thanks, > Florian >
On Tue, 2021-01-12 at 09:59 +0100, Florian Weimer wrote: > * Martin Sebor via Libc-alpha: > > > > 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). > > Maybe David can comment on how this interacts with his static > analyzer > work. BTW, the -fanalyzer part of support for __attribute__((malloc(deallocator))) is in gcc 11 as of yesterday: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=c7e276b869bdeb4a95735c1f037ee1a5f629de3d > In all other cases, the attribute means that the pointer needs to > be freed to avoid a resource leak. Indeed, the analyzer doesn't have any special knowledge of realpath and the conditional behavior. Given that annotation it will assume that the returned value is either a pointer that needs to be freed, or NULL on a failure, with no knowledge that the 2nd argument could be returned. I haven't tested Martin's patch, but I tried this example: $ cat t.c #include <stdio.h> #define PATH_MAX 4096 void free(void *ptr); char *realpath(const char *path, char *resolved_path) __attribute__ ((malloc (free))) __attribute__ ((__warn_unused_result__)); void test_1 (const char *path) { char buf[PATH_MAX]; char *result = realpath (path, buf); printf ("result: %s\n", result); } void test_2 (const char *path) { char buf[PATH_MAX]; char *result = realpath (path, buf); printf ("result: %s\n", result); free (result); } I believe test_1 is correct (although redundant in its use of "result" rather than buf, and can output a truncated path). I believe test_2 is a crasher bug: a "free" of on-stack "buf". Compiling with GCC 11 (with the __attribute__((malloc (DEALLOCATOR))) support: $ ./xgcc -B. -c -fanalyzer t.c t.c: In function ‘test_1’: t.c:14:1: warning: leak of ‘result’ [CWE-401] [-Wanalyzer-malloc-leak] 14 | } | ^ ‘test_1’: events 1-2 | | 12 | char *result = realpath (path, buf); | | ^~~~~~~~~~~~~~~~~~~~ | | | | | (1) allocated here | 13 | printf ("result: %s\n", result); | 14 | } | | ~ | | | | | (2) ‘result’ leaks here; was allocated at (1) | Here it falsely complains about test_1; it doesn't "know" that buf is returned by realpath and assumes that result needs to be freed. In test_2, there's a free of result == buf i.e. a free of an on-stack buffer, which it doesn't complain about, treating "result" as a malloc- ed pointer (as specified by the attribute). So I don't think this attribute should be applied to realpath. If we suddenly apply it pointers > which can only conditionally be freed, that reduces the value of > those > annotations, I think. FWIW, the analyzer already special-cases some functions; see the various region_model::impl_call_* functions in: https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/analyzer/region-model-impl-calls.cc There's a case for doing this for stuff in POSIX, which would apply here, I think. As noted above, I haven't tested Martin's glibc patch (I don't think I'm subscribed to this list). > Thanks, > Florian Hope this is constructive Dave
* Martin Sebor via Libc-alpha: > diff --git a/libio/stdio.h b/libio/stdio.h > index 144137cf67..62814c93db 100644 > --- a/libio/stdio.h > +++ b/libio/stdio.h > @@ -202,15 +214,9 @@ extern char *tmpnam_r (char *__s) __THROW __wur; > P_tmpdir is tried and finally "/tmp". The storage for the filename > is allocated by `malloc'. */ > extern char *tempnam (const char *__dir, const char *__pfx) > - __THROW __attribute_malloc__ __wur; > + __attribute_malloc__ __wur __attr_dealloc_free __THROW; > #endif This breaks boostrap because in some C++ modes, __THROW (aka noexcept) needs to come before all attributes. Further tests are ongoing; this is difficult to do due to general toolchain breakage (although things have improved lately). Thanks, Florian
* Martin Sebor via Libc-alpha: > diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h > index 6360845d98..a555fb9c0f 100644 > --- a/stdlib/stdlib.h > +++ b/stdlib/stdlib.h > @@ -550,6 +550,9 @@ extern void *calloc (size_t __nmemb, size_t __size) > extern void *realloc (void *__ptr, size_t __size) > __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2)); > > +/* Free a block allocated by `malloc', `realloc' or `calloc'. */ > +extern void free (void *__ptr) __THROW; > + > #ifdef __USE_MISC > /* Re-allocate the previously allocated block in PTR, making the new > block large enough for NMEMB elements of SIZE bytes each. */ > @@ -558,11 +561,13 @@ extern void *realloc (void *__ptr, size_t __size) > between objects pointed by the old and new pointers. */ > extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size) > __THROW __attribute_warn_unused_result__ > - __attribute_alloc_size__ ((2, 3)); > -#endif > + __attribute_alloc_size__ ((2, 3)) > + __attr_dealloc_free; > > -/* Free a block allocated by `malloc', `realloc' or `calloc'. */ > -extern void free (void *__ptr) __THROW; > +/* Add reallocarray as its own deallocator. */ > +extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size) > + __attr_dealloc (reallocarray, 1); > +#endif This causes: In file included from ../include/stdlib.h:15, from ../test-skeleton.c:36, from test-math-iszero.cc:164: ../stdlib/stdlib.h:568:14: error: declaration of ‘void* reallocarray(void*, size_t, size_t)’ has a different exception specifier 568 | extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size) | ^~~~~~~~~~~~ ../stdlib/stdlib.h:562:14: note: from previous declaration ‘void* reallocarray(void*, size_t, size_t) noexcept’ 562 | extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size) | ^~~~~~~~~~~~ Thanks, Florian
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 <wchar.h> >>> +/* Verify that calling fclose on the result of open_wmemstream doesn't >>> + trigger GCC -Wmismatched-dealloc with fclose forward-declared and >>> + without <stdio.h> 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 <wchar.h> >>> + >>> +/* Verify that calling fclose on the result of open_wmemstream doesn't >>> + trigger GCC -Wmismatched-dealloc with fclose forward-declared and >>> + without <stdio.h> 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 <stdio.h>) 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 <wchar.h> first and then <stdio.h> >> 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 <stdio.h> if >> <wchar.h> 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
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 <wchar.h> >>>> +/* Verify that calling fclose on the result of open_wmemstream doesn't >>>> + trigger GCC -Wmismatched-dealloc with fclose forward-declared and >>>> + without <stdio.h> 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 <wchar.h> >>>> + >>>> +/* Verify that calling fclose on the result of open_wmemstream doesn't >>>> + trigger GCC -Wmismatched-dealloc with fclose forward-declared and >>>> + without <stdio.h> 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 <stdio.h>) 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 <wchar.h> first and then <stdio.h> >>> 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 <stdio.h> if >>> <wchar.h> 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 >
Attached is the final patch tested with GCC 10 and 11 (the latter both 32- and 64-bit this time). If there are no objections I'll commit it tomorrow. Florian and/or anyone else who provided comments on it: please let me know if you would like me to mention you in the Reviewed-by tag. Thanks Martin On 5/6/21 5:54 PM, Martin Sebor wrote: > 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 <wchar.h> >>>>> +/* Verify that calling fclose on the result of open_wmemstream >>>>> doesn't >>>>> + trigger GCC -Wmismatched-dealloc with fclose forward-declared and >>>>> + without <stdio.h> 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 <wchar.h> >>>>> + >>>>> +/* Verify that calling fclose on the result of open_wmemstream >>>>> doesn't >>>>> + trigger GCC -Wmismatched-dealloc with fclose forward-declared and >>>>> + without <stdio.h> 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 <stdio.h>) 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 <wchar.h> first and then <stdio.h> >>>> 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 <stdio.h> if >>>> <wchar.h> 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 >> >
I have committed the patch in g:c1760eaf3b. On 5/13/21 3:49 PM, Martin Sebor wrote: > Attached is the final patch tested with GCC 10 and 11 (the latter > both 32- and 64-bit this time). If there are no objections I'll > commit it tomorrow. > > Florian and/or anyone else who provided comments on it: please let > me know if you would like me to mention you in the Reviewed-by tag. > > Thanks > Martin > > On 5/6/21 5:54 PM, Martin Sebor wrote: >> 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 <wchar.h> >>>>>> +/* Verify that calling fclose on the result of open_wmemstream >>>>>> doesn't >>>>>> + trigger GCC -Wmismatched-dealloc with fclose forward-declared and >>>>>> + without <stdio.h> 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 <wchar.h> >>>>>> + >>>>>> +/* Verify that calling fclose on the result of open_wmemstream >>>>>> doesn't >>>>>> + trigger GCC -Wmismatched-dealloc with fclose forward-declared and >>>>>> + without <stdio.h> 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 <stdio.h>) 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 <wchar.h> first and then <stdio.h> >>>>> 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 <stdio.h> if >>>>> <wchar.h> 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 >>> >> >
diff --git a/libio/stdio.h b/libio/stdio.h index 59f2ee01ab..f7ee14a9fd 100644 --- a/libio/stdio.h +++ b/libio/stdio.h @@ -165,22 +165,62 @@ extern int renameat2 (int __oldfd, const char *__old, int __newfd, const char *__new, unsigned int __flags) __THROW; #endif +/* Close STREAM. + + This function is a possible cancellation point and therefore not + marked with __THROW. */ +extern int fclose (FILE *__stream); + +/* This function is a possible cancellation point and therefore not + marked with __THROW. It's paired with fclose as a deallocator, + and (in a subsequent redeclarations) also with itself. */ +extern FILE *freopen (const char *__restrict __filename, + const char *__restrict __modes, + FILE *__restrict __stream) __wur + __attribute_malloc__ __attr_dealloc (fclose, 1); + +#ifdef __USE_LARGEFILE64 +extern FILE *freopen64 (const char *__restrict __filename, + const char *__restrict __modes, + FILE *__restrict __stream) __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3); +extern FILE *freopen64 (const char *__restrict __filename, + const char *__restrict __modes, + FILE *__restrict __stream) __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc (freopen64, 3); +extern FILE *fopen64 (const char *__restrict __filename, + const char *__restrict __modes) __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc (freopen64, 3); + +# define __attr_dealloc_freopen64 __attr_dealloc (freopen64, 3) +#else +# define __attr_dealloc_freopen64 /* empty */ +#endif + /* Create a temporary file and open it read/write. This function is a possible cancellation point and therefore not marked with __THROW. */ #ifndef __USE_FILE_OFFSET64 -extern FILE *tmpfile (void) __wur; +extern FILE *tmpfile (void) __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64; #else # ifdef __REDIRECT -extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) __wur; +extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64; # else # define tmpfile tmpfile64 # endif #endif #ifdef __USE_LARGEFILE64 -extern FILE *tmpfile64 (void) __wur; +extern FILE *tmpfile64 (void) __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64; #endif /* Generate a temporary filename. */ @@ -201,16 +241,21 @@ extern char *tmpnam_r (char __s[L_tmpnam]) __THROW __wur; If not and if DIR is not NULL, that value is checked. If that fails, P_tmpdir is tried and finally "/tmp". The storage for the filename is allocated by `malloc'. */ -extern char *tempnam (const char *__dir, const char *__pfx) - __THROW __attribute_malloc__ __wur; -#endif - - -/* Close STREAM. +extern char *tempnam (const char *__dir, const char *__pfx) __wur + __attribute_malloc__ +# ifdef __has_builtin +/* Reference the builtins since the declaration of neither function need be + in scope at this point. */ +# if __has_builtin (__builtin_free) + __attr_dealloc (__builtin_free, 1) +# endif +# if __has_builtin (__builtin_realloc) + __attr_dealloc (__builtin_realloc, 1) +# endif +# endif +# endif + __THROW; - This function is a possible cancellation point and therefore not - marked with __THROW. */ -extern int fclose (FILE *__stream); /* Flush STREAM, or all streams if STREAM is NULL. This function is a possible cancellation point and therefore not @@ -244,39 +289,40 @@ extern int fcloseall (void); This function is a possible cancellation point and therefore not marked with __THROW. */ extern FILE *fopen (const char *__restrict __filename, - const char *__restrict __modes) __wur; + const char *__restrict __modes) __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64; /* Open a file, replacing an existing stream with it. This function is a possible cancellation point and therefore not marked with __THROW. */ extern FILE *freopen (const char *__restrict __filename, const char *__restrict __modes, - FILE *__restrict __stream) __wur; + FILE *__restrict __stream) __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64; #else # ifdef __REDIRECT extern FILE *__REDIRECT (fopen, (const char *__restrict __filename, const char *__restrict __modes), fopen64) - __wur; + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64 __wur; extern FILE *__REDIRECT (freopen, (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream), freopen64) - __wur; + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64 __wur; # else # define fopen fopen64 # define freopen freopen64 # endif #endif -#ifdef __USE_LARGEFILE64 -extern FILE *fopen64 (const char *__restrict __filename, - const char *__restrict __modes) __wur; -extern FILE *freopen64 (const char *__restrict __filename, - const char *__restrict __modes, - FILE *__restrict __stream) __wur; -#endif #ifdef __USE_POSIX /* Create a new stream that refers to an existing system file descriptor. */ -extern FILE *fdopen (int __fd, const char *__modes) __THROW __wur; +extern FILE *fdopen (int __fd, const char *__modes) __THROW __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64; #endif #ifdef __USE_GNU @@ -284,18 +330,24 @@ extern FILE *fdopen (int __fd, const char *__modes) __THROW __wur; and uses the given functions for input and output. */ extern FILE *fopencookie (void *__restrict __magic_cookie, const char *__restrict __modes, - cookie_io_functions_t __io_funcs) __THROW __wur; + cookie_io_functions_t __io_funcs) __THROW __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64; #endif #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2) /* Create a new stream that refers to a memory buffer. */ extern FILE *fmemopen (void *__s, size_t __len, const char *__modes) - __THROW __wur; + __THROW __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64; /* Open a stream that writes into a malloc'd buffer that is expanded as necessary. *BUFLOC and *SIZELOC are updated with the buffer's location and the number of characters written on fflush or fclose. */ -extern FILE *open_memstream (char **__bufloc, size_t *__sizeloc) __THROW __wur; +extern FILE *open_memstream (char **__bufloc, size_t *__sizeloc) __THROW __wur + __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 3) + __attr_dealloc_freopen64; #endif @@ -792,17 +844,17 @@ extern int fileno_unlocked (FILE *__stream) __THROW __wur; #ifdef __USE_POSIX2 -/* Create a new stream connected to a pipe running the given command. +/* Close a stream opened by popen and return the status of its child. This function is a possible cancellation point and therefore not marked with __THROW. */ -extern FILE *popen (const char *__command, const char *__modes) __wur; - -/* Close a stream opened by popen and return the status of its child. +extern int pclose (FILE *__stream); +/* Create a new stream connected to a pipe running the given command. This function is a possible cancellation point and therefore not marked with __THROW. */ -extern int pclose (FILE *__stream); +extern FILE *popen (const char *__command, const char *__modes) __wur + __attribute_malloc__ __attr_dealloc (pclose, 1); #endif diff --git a/libio/tst-popen1.c b/libio/tst-popen1.c index bae6615b9b..9f36b20090 100644 --- a/libio/tst-popen1.c +++ b/libio/tst-popen1.c @@ -21,7 +21,7 @@ do_test (void) res = 1; } - fclose (fp); + pclose (fp); } fp = popen ("echo hello", "re"); @@ -39,7 +39,7 @@ do_test (void) res = 1; } - fclose (fp); + pclose (fp); } return res;