From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf43.google.com (mail-qv1-xf43.google.com [IPv6:2607:f8b0:4864:20::f43]) by sourceware.org (Postfix) with ESMTPS id 5CAEA3844046 for ; Tue, 15 Dec 2020 16:52:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5CAEA3844046 Received: by mail-qv1-xf43.google.com with SMTP id s6so9864091qvn.6 for ; Tue, 15 Dec 2020 08:52:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=NIpUl6EqAx3W+uRg5CCgFpngJO1TJRw9amqFmrnmA0I=; b=HQPbeqGjPvndPKTetv2PJ4YXEbSFKa3TOQ0MZGLB43ZP26OZfWyziw3yr5GDzD9KgC 7juh2DCD55N1b4h2krj/8VbUWzZEfutGffwAB9rqYJRE9Rs1Qy07SnyVF68p60uxjd/F ylTMIDjFn+5eby2XIYZUuOfYuFa83gjb+LiUlQZIgXGKOZaL+IbHsmTcMq4WPN3i4YMf l9iaL2Isf7o1UyacZNF76BUADxgEKEFSmp6sl3cVRgu5S98/jFwrfh3jiJwjPKRlille ch2H2S+mxbw+EAaNW+ZsCwRS7Wizfl9PTb8mOLfhFu3vZoBqDJ9r6ESJXQvR6H5TouuM zgOw== X-Gm-Message-State: AOAM531oAJjMU77gBFqtJpQtKs9+1KujUm9IVrCzN9LZTIdHohAyQqpF c54E2M/y9MaW/2QiLUnY1YtkcgiJkWI= X-Google-Smtp-Source: ABdhPJynKEmL5wMl5/te6K3uIaPbekIeXx9+wXlzMcfQVyif6kODrFuApbnBGoA7RshKx43fIwozBg== X-Received: by 2002:a05:6214:c66:: with SMTP id t6mr38010294qvj.43.1608051170593; Tue, 15 Dec 2020 08:52:50 -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 i11sm17868302qkg.45.2020.12.15.08.52.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 15 Dec 2020 08:52:50 -0800 (PST) Subject: Re: [PATCH] add support for -Wmismatched-dealloc To: Joseph Myers Cc: Florian Weimer , GNU C Library References: <74efece7-9a4b-83ee-7fdd-475c0d514378@gmail.com> <758e723b-67cf-a211-7bc2-2ccd3fc744e5@gmail.com> From: Martin Sebor Message-ID: <2555516b-4583-21fc-e844-fd44619488cd@gmail.com> Date: Tue, 15 Dec 2020 09:52:48 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------991A10AA2085A2E66D2F6ED8" Content-Language: en-US X-Spam-Status: No, score=-9.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Dec 2020 16:52:53 -0000 This is a multi-part message in MIME format. --------------991A10AA2085A2E66D2F6ED8 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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 --------------991A10AA2085A2E66D2F6ED8 Content-Type: text/x-patch; charset=UTF-8; name="glibc-attr-malloc.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="glibc-attr-malloc.diff" diff --git a/libio/stdio.h b/libio/stdio.h index 998470943e..a52324cc23 100644 --- a/libio/stdio.h +++ b/libio/stdio.h @@ -165,22 +165,31 @@ 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); + /* 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) + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; #else # ifdef __REDIRECT -extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) __wur; +extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; # else # define tmpfile tmpfile64 # endif #endif #ifdef __USE_LARGEFILE64 -extern FILE *tmpfile64 (void) __wur; +extern FILE *tmpfile64 (void) + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; #endif /* Generate a temporary filename. */ @@ -202,15 +211,17 @@ 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 +# ifdef __has_builtin +/* Reference the builtin since the ordinary declaration need not be + in scope at this point. */ +# if __has_builtin (__builtin_free) + __attr_dealloc (__builtin_free, 1) +# endif +# endif + __THROW; #endif - -/* Close STREAM. - - 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,7 +255,8 @@ 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) + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; /* Open a file, replacing an existing stream with it. This function is a possible cancellation point and therefore not @@ -256,7 +268,7 @@ extern FILE *freopen (const char *__restrict __filename, # ifdef __REDIRECT extern FILE *__REDIRECT (fopen, (const char *__restrict __filename, const char *__restrict __modes), fopen64) - __wur; + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; extern FILE *__REDIRECT (freopen, (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream), freopen64) @@ -268,7 +280,8 @@ extern FILE *__REDIRECT (freopen, (const char *__restrict __filename, #endif #ifdef __USE_LARGEFILE64 extern FILE *fopen64 (const char *__restrict __filename, - const char *__restrict __modes) __wur; + const char *__restrict __modes) + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; extern FILE *freopen64 (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream) __wur; @@ -276,7 +289,8 @@ extern FILE *freopen64 (const char *__restrict __filename, #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 + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; #endif #ifdef __USE_GNU @@ -284,18 +298,20 @@ 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 + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; #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 __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; /* 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 + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; #endif @@ -792,17 +808,19 @@ 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; +extern int pclose (FILE *__stream); -/* Close a stream opened by popen and return the status of its child. +/* 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) + __attribute_malloc__ __attr_dealloc (pclose, 1) __wur; + #endif diff --git a/libio/tst-freopen.c b/libio/tst-freopen.c index baa13166fe..a69e4430a7 100644 --- a/libio/tst-freopen.c +++ b/libio/tst-freopen.c @@ -81,6 +81,28 @@ do_test_basic (void) fclose (f); } +/* Verify that freopen returns stream. */ +static void +do_test_return_stream (void) +{ + FILE *f1 = fopen (name, "r"); + if (f1 == NULL) + FAIL_EXIT1 ("fopen: %m"); + + FILE *f2 = freopen (name, "r+", f1); + if (f2 == NULL) + FAIL_EXIT1 ("freopen: %m"); + + /* Verify that freopen isn't declared with attribute malloc + (which would let GCC fold the inequality to false). */ + if (f1 != f2) + FAIL_EXIT1 ("freopen returned a different stream"); + + /* This shouldn't trigger -Wmismatched-dealloc. */ + fclose (f1); +} + + /* Test for BZ#21398, where it tries to freopen stdio after the close of its file descriptor. */ static void @@ -105,6 +127,7 @@ do_test (void) { do_test_basic (); do_test_bz21398 (); + do_test_return_stream (); return 0; } 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; diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index a06f1cfd91..092e078ef8 100644 --- a/misc/sys/cdefs.h +++ b/misc/sys/cdefs.h @@ -560,6 +560,15 @@ _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))) +#else +# define __attr_dealloc(x) +#endif + /* Specify that a function such as setjmp or vfork may return twice. */ #if __GNUC_PREREQ (4, 1) diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h index 3aa27a9d25..a039f17d97 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, 1); -/* 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 #ifdef __USE_MISC # include @@ -788,7 +793,8 @@ extern int system (const char *__command) __wur; /* Return a malloc'd string containing the canonical absolute name of the existing named file. */ extern char *canonicalize_file_name (const char *__name) - __THROW __nonnull ((1)) __wur; + __THROW __nonnull ((1)) __attribute_malloc__ + __attr_dealloc (free, 1) __wur; #endif #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, 1) __wur; #endif diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h index 9cf8b05a87..7442c80753 100644 --- a/wcsmbs/wchar.h +++ b/wcsmbs/wchar.h @@ -562,9 +562,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_memstream below. */ +extern char *__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 --------------991A10AA2085A2E66D2F6ED8--