* [PATCH] add support for -Wmismatched-dealloc @ 2020-12-08 22:52 Martin Sebor 2020-12-09 0:07 ` Joseph Myers 0 siblings, 1 reply; 27+ messages in thread From: Martin Sebor @ 2020-12-08 22:52 UTC (permalink / raw) To: GNU C Library [-- Attachment #1: Type: text/plain, Size: 2569 bytes --] 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 [-- Attachment #2: glibc-attr-malloc-stdio.diff --] [-- Type: text/x-patch, Size: 8382 bytes --] 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; ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2020-12-08 22:52 [PATCH] add support for -Wmismatched-dealloc Martin Sebor @ 2020-12-09 0:07 ` Joseph Myers 2020-12-12 2:25 ` Martin Sebor 0 siblings, 1 reply; 27+ messages in thread From: Joseph Myers @ 2020-12-09 0:07 UTC (permalink / raw) To: Martin Sebor; +Cc: GNU C Library 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?) -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2020-12-09 0:07 ` Joseph Myers @ 2020-12-12 2:25 ` Martin Sebor 2020-12-14 21:39 ` Martin Sebor 0 siblings, 1 reply; 27+ messages in thread From: Martin Sebor @ 2020-12-12 2:25 UTC (permalink / raw) To: Joseph Myers; +Cc: GNU C Library [-- Attachment #1: Type: text/plain, Size: 2301 bytes --] 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 [-- Attachment #2: glibc-attr-malloc.diff --] [-- Type: text/x-patch, Size: 10910 bytes --] diff --git a/libio/stdio.h b/libio/stdio.h index 998470943e..7b89090ea7 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,23 +255,35 @@ 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 marked with __THROW. */ extern FILE *freopen (const char *__restrict __filename, const char *__restrict __modes, - FILE *__restrict __stream) __wur; + FILE *__restrict __stream) + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; +/* Make freopen its own deallocator. */ +extern FILE *freopen (const char *__restrict __filename, + const char *__restrict __modes, + FILE *__restrict __stream) + __attr_dealloc (freopen, 3); #else # 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) + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; +/* Make freopen its own deallocator. */ extern FILE *__REDIRECT (freopen, (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream), freopen64) - __wur; + __attr_dealloc (freopen, 3); # else # define fopen fopen64 # define freopen freopen64 @@ -268,15 +291,23 @@ 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; + FILE *__restrict __stream) + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; +/* Make freopen64 its own deallocator. */ +extern FILE *freopen64 (const char *__restrict __filename, + const char *__restrict __modes, + FILE *__restrict __stream) + __attr_dealloc (freopen64, 3); #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 + __attribute_malloc__ __attr_dealloc (fclose, 1) __wur; #endif #ifdef __USE_GNU @@ -284,18 +315,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 +825,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-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 e94d09d7dd..7bc99ec2f5 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 <alloca.h> @@ -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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2020-12-12 2:25 ` Martin Sebor @ 2020-12-14 21:39 ` Martin Sebor 2020-12-14 22:16 ` Florian Weimer 2020-12-15 1:01 ` Joseph Myers 0 siblings, 2 replies; 27+ messages in thread From: Martin Sebor @ 2020-12-14 21:39 UTC (permalink / raw) To: Joseph Myers; +Cc: GNU C Library, Florian Weimer 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2020-12-14 21:39 ` Martin Sebor @ 2020-12-14 22:16 ` Florian Weimer 2020-12-15 1:01 ` Joseph Myers 1 sibling, 0 replies; 27+ messages in thread From: Florian Weimer @ 2020-12-14 22:16 UTC (permalink / raw) To: Martin Sebor; +Cc: GNU C Library * 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 -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2020-12-14 21:39 ` Martin Sebor 2020-12-14 22:16 ` Florian Weimer @ 2020-12-15 1:01 ` Joseph Myers 2020-12-15 16:52 ` Martin Sebor 1 sibling, 1 reply; 27+ messages in thread From: Joseph Myers @ 2020-12-15 1:01 UTC (permalink / raw) To: Martin Sebor; +Cc: Florian Weimer, GNU C Library 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. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2020-12-15 1:01 ` Joseph Myers @ 2020-12-15 16:52 ` Martin Sebor 2020-12-27 23:13 ` Martin Sebor 0 siblings, 1 reply; 27+ messages in thread From: Martin Sebor @ 2020-12-15 16:52 UTC (permalink / raw) To: Joseph Myers; +Cc: Florian Weimer, GNU C Library [-- Attachment #1: Type: text/plain, Size: 3017 bytes --] 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 [-- Attachment #2: glibc-attr-malloc.diff --] [-- Type: text/x-patch, Size: 10913 bytes --] 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 <alloca.h> @@ -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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2020-12-15 16:52 ` Martin Sebor @ 2020-12-27 23:13 ` Martin Sebor 2021-01-04 15:56 ` Ping: " Martin Sebor 2021-01-04 16:07 ` Florian Weimer 0 siblings, 2 replies; 27+ messages in thread From: Martin Sebor @ 2020-12-27 23:13 UTC (permalink / raw) To: Joseph Myers; +Cc: Florian Weimer, GNU C Library [-- Attachment #1: Type: text/plain, Size: 3748 bytes --] 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 [-- Attachment #2: glibc-attr-malloc.diff --] [-- Type: text/x-patch, Size: 17060 bytes --] diff --git a/include/programs/xmalloc.h b/include/programs/xmalloc.h index fd6cf61730..2a8d6ff23c 100644 --- a/include/programs/xmalloc.h +++ b/include/programs/xmalloc.h @@ -23,11 +23,11 @@ /* Prototypes for a few program-wide used functions. */ extern void *xmalloc (size_t n) - __attribute_malloc__ __attribute_alloc_size__ ((1)); + __attribute_malloc__ __attribute_alloc_size__ ((1)) __attr_dealloc_free; extern void *xcalloc (size_t n, size_t s) - __attribute_malloc__ __attribute_alloc_size__ ((1, 2)); + __attribute_malloc__ __attribute_alloc_size__ ((1, 2)) __attr_dealloc_free; extern void *xrealloc (void *o, size_t n) - __attribute_malloc__ __attribute_alloc_size__ ((2)); -extern char *xstrdup (const char *) __attribute_malloc__; + __attribute_malloc__ __attribute_alloc_size__ ((2)) __attr_dealloc_free; +extern char *xstrdup (const char *) __attribute_malloc__ __attr_dealloc_free; #endif /* xmalloc.h */ diff --git a/libio/Makefile b/libio/Makefile index 276baf58ac..0f1f536f7a 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -61,7 +61,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \ bug-ungetc2 bug-ftell bug-ungetc3 bug-ungetc4 tst-fopenloc2 \ tst-memstream1 tst-memstream2 tst-memstream3 tst-memstream4 \ tst-wmemstream1 tst-wmemstream2 tst-wmemstream3 tst-wmemstream4 \ - bug-memstream1 bug-wmemstream1 \ + tst-wmemstream5 bug-memstream1 bug-wmemstream1 \ tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \ tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \ diff --git a/libio/stdio.h b/libio/stdio.h index 998470943e..49d8607bf3 100644 --- a/libio/stdio.h +++ b/libio/stdio.h @@ -165,22 +165,41 @@ 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); + +#ifdef __REDIRECT +/* Declare the __fclose alias and associate it as a deallocator with + fopen below. Use an alias for the sake of open_wmemstream in + <wchar.h>. */ +extern int __REDIRECT (__fclose, (FILE *), fclose); +# define __attr_dealloc_fclose __attr_dealloc (__fclose, 1) +#else +# define __attr_dealloc_fclose /* 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) + __attribute_malloc__ __attr_dealloc_fclose __wur; #else # ifdef __REDIRECT -extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) __wur; +extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) + __attribute_malloc__ __attr_dealloc_fclose __wur; # else # define tmpfile tmpfile64 # endif #endif #ifdef __USE_LARGEFILE64 -extern FILE *tmpfile64 (void) __wur; +extern FILE *tmpfile64 (void) + __attribute_malloc__ __attr_dealloc_fclose __wur; #endif /* Generate a temporary filename. */ @@ -202,15 +221,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 - -/* 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 +257,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 __wur; /* Open a file, replacing an existing stream with it. This function is a possible cancellation point and therefore not @@ -256,7 +270,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 __wur; extern FILE *__REDIRECT (freopen, (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream), freopen64) @@ -268,7 +282,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 __wur; extern FILE *freopen64 (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream) __wur; @@ -276,7 +291,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 __wur; #endif #ifdef __USE_GNU @@ -284,18 +300,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 __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 __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 __wur; #endif @@ -792,17 +810,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..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 + +/* 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); +} + +#if defined __GNUC__ && __GNUC__ >= 11 +# pragma GCC diagnostic pop +#endif + /* Test for BZ#21398, where it tries to freopen stdio after the close of its file descriptor. */ static void @@ -105,6 +135,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/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 + +int test_open_wmemstream_no_stdio (void) +{ + { + wchar_t *buf; + size_t size; + FILE *f = open_wmemstream (&buf, &size); + fclose (f); + } + + { + FILE* (*pf)(wchar_t**, size_t*) = open_wmemstream; + wchar_t *buf; + size_t size; + FILE *f = pf (&buf, &size); + fclose (f); + } + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +# pragma GCC diagnostic pop +#endif + #define CHAR_T wchar_t #define W(o) L##o #define OPEN_MEMSTREAM open_wmemstream 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 @@ +#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 + +static int +do_test (void) +{ + { + wchar_t *buf; + size_t size; + FILE *f = open_wmemstream (&buf, &size); + fclose (f); + } + + { + FILE* (*pf)(wchar_t**, size_t*) = open_wmemstream; + wchar_t *buf; + size_t size; + FILE *f = pf (&buf, &size); + fclose (f); + } + + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +# pragma GCC diagnostic pop +#endif + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/malloc/malloc.h b/malloc/malloc.h index b2371f7704..d9dc359559 100644 --- a/malloc/malloc.h +++ b/malloc/malloc.h @@ -56,22 +56,25 @@ __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2)); the same pointer that was passed to it, aliasing needs to be allowed 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)); + __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2, 3)) + __attr_dealloc_free; /* Free a block allocated by `malloc', `realloc' or `calloc'. */ extern void free (void *__ptr) __THROW; /* Allocate SIZE bytes allocated to ALIGNMENT bytes. */ extern void *memalign (size_t __alignment, size_t __size) -__THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur; + __THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur + __attr_dealloc_free; /* Allocate SIZE bytes on a page boundary. */ extern void *valloc (size_t __size) __THROW __attribute_malloc__ - __attribute_alloc_size__ ((1)) __wur; + __attribute_alloc_size__ ((1)) __wur __attr_dealloc_free; /* Equivalent to valloc(minimum-page-that-holds(n)), that is, round up __size to nearest pagesize. */ -extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ __wur; +extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ + __wur __attr_dealloc_free; /* Underlying allocation function; successive calls should return contiguous pieces of memory. */ diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index a06f1cfd91..fe5cb56511 100644 --- a/misc/sys/cdefs.h +++ b/misc/sys/cdefs.h @@ -560,6 +560,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 + /* 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..f88f8e55b3 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 #ifdef __USE_MISC # include <alloca.h> @@ -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 __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 __wur; #endif 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Ping: [PATCH] add support for -Wmismatched-dealloc 2020-12-27 23:13 ` Martin Sebor @ 2021-01-04 15:56 ` Martin Sebor 2021-01-04 16:07 ` Florian Weimer 1 sibling, 0 replies; 27+ messages in thread From: Martin Sebor @ 2021-01-04 15:56 UTC (permalink / raw) To: Joseph Myers; +Cc: Florian Weimer, GNU C Library 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2020-12-27 23:13 ` Martin Sebor 2021-01-04 15:56 ` Ping: " Martin Sebor @ 2021-01-04 16:07 ` Florian Weimer 2021-01-04 16:18 ` Martin Sebor 1 sibling, 1 reply; 27+ messages in thread From: Florian Weimer @ 2021-01-04 16:07 UTC (permalink / raw) To: Martin Sebor via Libc-alpha; +Cc: Joseph Myers, Martin Sebor * 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 -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-04 16:07 ` Florian Weimer @ 2021-01-04 16:18 ` Martin Sebor 2021-01-04 16:57 ` Florian Weimer 0 siblings, 1 reply; 27+ messages in thread From: Martin Sebor @ 2021-01-04 16:18 UTC (permalink / raw) To: Florian Weimer, Martin Sebor via Libc-alpha; +Cc: Joseph Myers 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-04 16:18 ` Martin Sebor @ 2021-01-04 16:57 ` Florian Weimer 2021-01-04 23:18 ` Martin Sebor 0 siblings, 1 reply; 27+ messages in thread From: Florian Weimer @ 2021-01-04 16:57 UTC (permalink / raw) To: Martin Sebor; +Cc: Martin Sebor via Libc-alpha, Joseph Myers * 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 -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-04 16:57 ` Florian Weimer @ 2021-01-04 23:18 ` Martin Sebor 2021-01-10 20:42 ` Ping: " Martin Sebor 2021-01-11 9:13 ` Florian Weimer 0 siblings, 2 replies; 27+ messages in thread From: Martin Sebor @ 2021-01-04 23:18 UTC (permalink / raw) To: Florian Weimer; +Cc: Martin Sebor via Libc-alpha, Joseph Myers [-- Attachment #1: Type: text/plain, Size: 4267 bytes --] 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 > [-- Attachment #2: glibc-attr-malloc.diff --] [-- Type: text/x-patch, Size: 16904 bytes --] diff --git a/include/programs/xmalloc.h b/include/programs/xmalloc.h index fd6cf61730..2a8d6ff23c 100644 --- a/include/programs/xmalloc.h +++ b/include/programs/xmalloc.h @@ -23,11 +23,11 @@ /* Prototypes for a few program-wide used functions. */ extern void *xmalloc (size_t n) - __attribute_malloc__ __attribute_alloc_size__ ((1)); + __attribute_malloc__ __attribute_alloc_size__ ((1)) __attr_dealloc_free; extern void *xcalloc (size_t n, size_t s) - __attribute_malloc__ __attribute_alloc_size__ ((1, 2)); + __attribute_malloc__ __attribute_alloc_size__ ((1, 2)) __attr_dealloc_free; extern void *xrealloc (void *o, size_t n) - __attribute_malloc__ __attribute_alloc_size__ ((2)); -extern char *xstrdup (const char *) __attribute_malloc__; + __attribute_malloc__ __attribute_alloc_size__ ((2)) __attr_dealloc_free; +extern char *xstrdup (const char *) __attribute_malloc__ __attr_dealloc_free; #endif /* xmalloc.h */ diff --git a/libio/Makefile b/libio/Makefile index 276baf58ac..0f1f536f7a 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -61,7 +61,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \ bug-ungetc2 bug-ftell bug-ungetc3 bug-ungetc4 tst-fopenloc2 \ tst-memstream1 tst-memstream2 tst-memstream3 tst-memstream4 \ tst-wmemstream1 tst-wmemstream2 tst-wmemstream3 tst-wmemstream4 \ - bug-memstream1 bug-wmemstream1 \ + tst-wmemstream5 bug-memstream1 bug-wmemstream1 \ tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \ tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \ diff --git a/libio/stdio.h b/libio/stdio.h index 998470943e..f27d526f44 100644 --- a/libio/stdio.h +++ b/libio/stdio.h @@ -165,22 +165,34 @@ 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); + +#undef __attr_dealloc_fclose +#define __attr_dealloc_fclose __attr_dealloc (fclose, 1) + /* 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 __wur; #else # ifdef __REDIRECT -extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) __wur; +extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) + __attribute_malloc__ __attr_dealloc_fclose __wur; # else # define tmpfile tmpfile64 # endif #endif #ifdef __USE_LARGEFILE64 -extern FILE *tmpfile64 (void) __wur; +extern FILE *tmpfile64 (void) + __attribute_malloc__ __attr_dealloc_fclose __wur; #endif /* Generate a temporary filename. */ @@ -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 - -/* 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 +250,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 __wur; /* Open a file, replacing an existing stream with it. This function is a possible cancellation point and therefore not @@ -256,7 +263,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 __wur; extern FILE *__REDIRECT (freopen, (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream), freopen64) @@ -268,7 +275,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 __wur; extern FILE *freopen64 (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream) __wur; @@ -276,7 +284,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 __wur; #endif #ifdef __USE_GNU @@ -284,18 +293,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 __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 __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 __wur; #endif @@ -792,17 +803,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..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 + +/* 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); +} + +#if defined __GNUC__ && __GNUC__ >= 11 +# pragma GCC diagnostic pop +#endif + /* Test for BZ#21398, where it tries to freopen stdio after the close of its file descriptor. */ static void @@ -105,6 +135,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/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 + +int test_open_wmemstream_no_stdio (void) +{ + { + wchar_t *buf; + size_t size; + FILE *f = open_wmemstream (&buf, &size); + fclose (f); + } + + { + FILE* (*pf)(wchar_t**, size_t*) = open_wmemstream; + wchar_t *buf; + size_t size; + FILE *f = pf (&buf, &size); + fclose (f); + } + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +# pragma GCC diagnostic pop +#endif + #define CHAR_T wchar_t #define W(o) L##o #define OPEN_MEMSTREAM open_wmemstream 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 @@ +#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 + +static int +do_test (void) +{ + { + wchar_t *buf; + size_t size; + FILE *f = open_wmemstream (&buf, &size); + fclose (f); + } + + { + FILE* (*pf)(wchar_t**, size_t*) = open_wmemstream; + wchar_t *buf; + size_t size; + FILE *f = pf (&buf, &size); + fclose (f); + } + + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +# pragma GCC diagnostic pop +#endif + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/malloc/malloc.h b/malloc/malloc.h index b2371f7704..d9dc359559 100644 --- a/malloc/malloc.h +++ b/malloc/malloc.h @@ -56,22 +56,25 @@ __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2)); the same pointer that was passed to it, aliasing needs to be allowed 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)); + __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2, 3)) + __attr_dealloc_free; /* Free a block allocated by `malloc', `realloc' or `calloc'. */ extern void free (void *__ptr) __THROW; /* Allocate SIZE bytes allocated to ALIGNMENT bytes. */ extern void *memalign (size_t __alignment, size_t __size) -__THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur; + __THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur + __attr_dealloc_free; /* Allocate SIZE bytes on a page boundary. */ extern void *valloc (size_t __size) __THROW __attribute_malloc__ - __attribute_alloc_size__ ((1)) __wur; + __attribute_alloc_size__ ((1)) __wur __attr_dealloc_free; /* Equivalent to valloc(minimum-page-that-holds(n)), that is, round up __size to nearest pagesize. */ -extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ __wur; +extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ + __wur __attr_dealloc_free; /* Underlying allocation function; successive calls should return contiguous pieces of memory. */ 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 + /* 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..f88f8e55b3 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 #ifdef __USE_MISC # include <alloca.h> @@ -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 __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 __wur; #endif 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Ping: [PATCH] add support for -Wmismatched-dealloc 2021-01-04 23:18 ` Martin Sebor @ 2021-01-10 20:42 ` Martin Sebor 2021-01-11 9:13 ` Florian Weimer 1 sibling, 0 replies; 27+ messages in thread From: Martin Sebor @ 2021-01-10 20:42 UTC (permalink / raw) To: Florian Weimer; +Cc: Martin Sebor via Libc-alpha, Joseph Myers 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 >> > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-04 23:18 ` Martin Sebor 2021-01-10 20:42 ` Ping: " Martin Sebor @ 2021-01-11 9:13 ` Florian Weimer 2021-01-12 0:00 ` Martin Sebor 2021-01-12 0:01 ` Martin Sebor 1 sibling, 2 replies; 27+ messages in thread From: Florian Weimer @ 2021-01-11 9:13 UTC (permalink / raw) To: Martin Sebor; +Cc: Martin Sebor via Libc-alpha, Joseph Myers * 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 -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-11 9:13 ` Florian Weimer @ 2021-01-12 0:00 ` Martin Sebor 2021-01-12 0:01 ` Martin Sebor 1 sibling, 0 replies; 27+ messages in thread From: Martin Sebor @ 2021-01-12 0:00 UTC (permalink / raw) To: Florian Weimer; +Cc: Martin Sebor via Libc-alpha, Joseph Myers 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-11 9:13 ` Florian Weimer 2021-01-12 0:00 ` Martin Sebor @ 2021-01-12 0:01 ` Martin Sebor 2021-01-12 8:59 ` Florian Weimer ` (4 more replies) 1 sibling, 5 replies; 27+ messages in thread From: Martin Sebor @ 2021-01-12 0:01 UTC (permalink / raw) To: Florian Weimer; +Cc: Martin Sebor via Libc-alpha, Joseph Myers [-- Attachment #1: Type: text/plain, Size: 5957 bytes --] 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 [-- Attachment #2: glibc-attr-malloc.diff --] [-- Type: text/x-patch, Size: 18695 bytes --] diff --git a/include/programs/xmalloc.h b/include/programs/xmalloc.h index 76e345f4ee..33871e22ef 100644 --- a/include/programs/xmalloc.h +++ b/include/programs/xmalloc.h @@ -23,11 +23,11 @@ /* Prototypes for a few program-wide used functions. */ extern void *xmalloc (size_t n) - __attribute_malloc__ __attribute_alloc_size__ ((1)); + __attribute_malloc__ __attribute_alloc_size__ ((1)) __attr_dealloc_free; extern void *xcalloc (size_t n, size_t s) - __attribute_malloc__ __attribute_alloc_size__ ((1, 2)); + __attribute_malloc__ __attribute_alloc_size__ ((1, 2)) __attr_dealloc_free; extern void *xrealloc (void *o, size_t n) - __attribute_malloc__ __attribute_alloc_size__ ((2)); -extern char *xstrdup (const char *) __attribute_malloc__; + __attribute_malloc__ __attribute_alloc_size__ ((2)) __attr_dealloc_free; +extern char *xstrdup (const char *) __attribute_malloc__ __attr_dealloc_free; #endif /* xmalloc.h */ diff --git a/libio/Makefile b/libio/Makefile index 12ce41038f..99ec4bee20 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -61,7 +61,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \ bug-ungetc2 bug-ftell bug-ungetc3 bug-ungetc4 tst-fopenloc2 \ tst-memstream1 tst-memstream2 tst-memstream3 tst-memstream4 \ tst-wmemstream1 tst-wmemstream2 tst-wmemstream3 tst-wmemstream4 \ - bug-memstream1 bug-wmemstream1 \ + tst-wmemstream5 bug-memstream1 bug-wmemstream1 \ tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \ tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \ diff --git a/libio/stdio.h b/libio/stdio.h index 144137cf67..62814c93db 100644 --- a/libio/stdio.h +++ b/libio/stdio.h @@ -165,22 +165,34 @@ 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); + +#undef __attr_dealloc_fclose +#define __attr_dealloc_fclose __attr_dealloc (fclose, 1) + /* 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 __wur; #else # ifdef __REDIRECT -extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) __wur; +extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) + __attribute_malloc__ __attr_dealloc_fclose __wur; # else # define tmpfile tmpfile64 # endif #endif #ifdef __USE_LARGEFILE64 -extern FILE *tmpfile64 (void) __wur; +extern FILE *tmpfile64 (void) + __attribute_malloc__ __attr_dealloc_fclose __wur; #endif /* Generate a temporary filename. */ @@ -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 - -/* 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 +250,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 __wur; /* Open a file, replacing an existing stream with it. This function is a possible cancellation point and therefore not @@ -256,7 +263,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 __wur; extern FILE *__REDIRECT (freopen, (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream), freopen64) @@ -268,7 +275,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 __wur; extern FILE *freopen64 (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream) __wur; @@ -276,7 +284,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 __wur; #endif #ifdef __USE_GNU @@ -284,21 +293,30 @@ 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 __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 __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 __wur; + +#ifdef _WCHAR_H +/* Like OPEN_MEMSTREAM, but the stream is wide oriented and produces + a wide character string. Declared here only to add attribute malloc + and only if <wchar.h> has been previously #included. */ +extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t *__sizeloc) __THROW + __attribute_malloc__ __attr_dealloc_fclose; +# endif #endif - /* If BUF is NULL, make STREAM unbuffered. Else make it use buffer BUF, of size BUFSIZ. */ extern void setbuf (FILE *__restrict __stream, char *__restrict __buf) __THROW; @@ -792,17 +810,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 660882a28a..c8bc0a3d07 100644 --- a/libio/tst-freopen.c +++ b/libio/tst-freopen.c @@ -81,6 +81,42 @@ do_test_basic (void) fclose (f); } +#if defined __GNUC__ && __GNUC__ >= 11 +/* Force an error to detect incorrectly making freopen a deallocator + for its last argument via attribute malloc. The function closes + the stream without deallocating it so either the argument or + the pointer returned from the function (but not both) can be passed + to fclose. */ +#pragma GCC diagnostic push +#pragma GCC diagnostic error "-Wmismatched-dealloc" +#endif + +/* 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 the no-argument attribute + malloc (which could 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); +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Pop -Wmismatched-dealloc set to error above. */ +# pragma GCC diagnostic pop +#endif + /* Test for BZ#21398, where it tries to freopen stdio after the close of its file descriptor. */ static void @@ -105,6 +141,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/libio/tst-wmemstream1.c b/libio/tst-wmemstream1.c index f8b308bc6c..f80655b2a4 100644 --- a/libio/tst-wmemstream1.c +++ b/libio/tst-wmemstream1.c @@ -1,5 +1,40 @@ #include <wchar.h> +extern int fclose (FILE*); + +#if defined __GNUC__ && __GNUC__ >= 11 +/* 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"). */ +#pragma GCC diagnostic push +#pragma GCC diagnostic error "-Wmismatched-dealloc" +#endif + +int test_open_wmemstream_no_stdio (void) +{ + { + wchar_t *buf; + size_t size; + FILE *f = open_wmemstream (&buf, &size); + fclose (f); + } + + { + FILE* (*pf)(wchar_t**, size_t*) = open_wmemstream; + wchar_t *buf; + size_t size; + FILE *f = pf (&buf, &size); + fclose (f); + } + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Restore -Wmismatched-dealloc setting. */ +# pragma GCC diagnostic pop +#endif + #define CHAR_T wchar_t #define W(o) L##o #define OPEN_MEMSTREAM open_wmemstream diff --git a/libio/tst-wmemstream5.c b/libio/tst-wmemstream5.c new file mode 100644 index 0000000000..47f5e63603 --- /dev/null +++ b/libio/tst-wmemstream5.c @@ -0,0 +1,57 @@ +/* Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <wchar.h> + +extern int fclose (FILE*); + +#if defined __GNUC__ && __GNUC__ >= 11 +/* 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. */ +#pragma GCC diagnostic push +#pragma GCC diagnostic error "-Wmismatched-dealloc" +#endif + +static int +do_test (void) +{ + { + wchar_t *buf; + size_t size; + FILE *f = open_wmemstream (&buf, &size); + fclose (f); + } + + { + FILE* (*pf)(wchar_t**, size_t*) = open_wmemstream; + wchar_t *buf; + size_t size; + FILE *f = pf (&buf, &size); + fclose (f); + } + + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Restore -Wmismatched-dealloc setting. */ +# pragma GCC diagnostic pop +#endif + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/malloc/malloc.h b/malloc/malloc.h index e9157ddfc3..c1c0896d29 100644 --- a/malloc/malloc.h +++ b/malloc/malloc.h @@ -56,22 +56,25 @@ __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2)); the same pointer that was passed to it, aliasing needs to be allowed 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)); + __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2, 3)) + __attr_dealloc_free; /* Free a block allocated by `malloc', `realloc' or `calloc'. */ extern void free (void *__ptr) __THROW; /* Allocate SIZE bytes allocated to ALIGNMENT bytes. */ extern void *memalign (size_t __alignment, size_t __size) -__THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur; + __THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur + __attr_dealloc_free; /* Allocate SIZE bytes on a page boundary. */ extern void *valloc (size_t __size) __THROW __attribute_malloc__ - __attribute_alloc_size__ ((1)) __wur; + __attribute_alloc_size__ ((1)) __wur __attr_dealloc_free; /* Equivalent to valloc(minimum-page-that-holds(n)), that is, round up __size to nearest pagesize. */ -extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ __wur; +extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ + __wur __attr_dealloc_free; /* Underlying allocation function; successive calls should return contiguous pieces of memory. */ diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index 57ca262bdf..14e25e2ac1 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 + /* 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 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 #ifdef __USE_MISC # include <alloca.h> @@ -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 __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 __wur; #endif diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h index ce0acb1c28..075776890f 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-12 0:01 ` Martin Sebor @ 2021-01-12 8:59 ` Florian Weimer 2021-01-19 1:08 ` Martin Sebor 2021-01-19 16:54 ` David Malcolm 2021-01-22 21:26 ` DJ Delorie ` (3 subsequent siblings) 4 siblings, 2 replies; 27+ messages in thread From: Florian Weimer @ 2021-01-12 8:59 UTC (permalink / raw) To: Martin Sebor via Libc-alpha; +Cc: Martin Sebor, Joseph Myers, David Malcolm * 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 -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-12 8:59 ` Florian Weimer @ 2021-01-19 1:08 ` Martin Sebor 2021-01-19 16:54 ` David Malcolm 1 sibling, 0 replies; 27+ messages in thread From: Martin Sebor @ 2021-01-19 1:08 UTC (permalink / raw) To: Florian Weimer, Martin Sebor via Libc-alpha, David Malcolm; +Cc: Joseph Myers 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-12 8:59 ` Florian Weimer 2021-01-19 1:08 ` Martin Sebor @ 2021-01-19 16:54 ` David Malcolm 1 sibling, 0 replies; 27+ messages in thread From: David Malcolm @ 2021-01-19 16:54 UTC (permalink / raw) To: Florian Weimer, Martin Sebor via Libc-alpha; +Cc: Martin Sebor, Joseph Myers 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-12 0:01 ` Martin Sebor 2021-01-12 8:59 ` Florian Weimer @ 2021-01-22 21:26 ` DJ Delorie 2021-01-25 10:56 ` Florian Weimer ` (2 subsequent siblings) 4 siblings, 0 replies; 27+ messages in thread From: DJ Delorie @ 2021-01-22 21:26 UTC (permalink / raw) To: Martin Sebor; +Cc: fweimer, libc-alpha, joseph Martin Sebor via Libc-alpha <libc-alpha@sourceware.org> writes: >> 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). Do we have a resolution on this? I think deferring the realpath part of this patch until it can be handled correctly is a reasonable solution. I think an unwarned leak is better than false warnings. The rest of the patch looks fine and has been reviewed... ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-12 0:01 ` Martin Sebor 2021-01-12 8:59 ` Florian Weimer 2021-01-22 21:26 ` DJ Delorie @ 2021-01-25 10:56 ` Florian Weimer 2021-01-25 11:31 ` Florian Weimer 2021-04-23 0:00 ` Martin Sebor 4 siblings, 0 replies; 27+ messages in thread From: Florian Weimer @ 2021-01-25 10:56 UTC (permalink / raw) To: Martin Sebor via Libc-alpha; +Cc: Martin Sebor, Joseph Myers * 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 -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-12 0:01 ` Martin Sebor ` (2 preceding siblings ...) 2021-01-25 10:56 ` Florian Weimer @ 2021-01-25 11:31 ` Florian Weimer 2021-04-23 0:00 ` Martin Sebor 4 siblings, 0 replies; 27+ messages in thread From: Florian Weimer @ 2021-01-25 11:31 UTC (permalink / raw) To: Martin Sebor via Libc-alpha; +Cc: Martin Sebor, Joseph Myers * 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 -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-01-12 0:01 ` Martin Sebor ` (3 preceding siblings ...) 2021-01-25 11:31 ` Florian Weimer @ 2021-04-23 0:00 ` Martin Sebor 2021-05-06 23:54 ` Martin Sebor 4 siblings, 1 reply; 27+ messages in thread From: Martin Sebor @ 2021-04-23 0:00 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Joseph Myers [-- Attachment #1: Type: text/plain, Size: 7143 bytes --] 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 [-- Attachment #2: glibc-attr-malloc.diff --] [-- Type: text/x-patch, Size: 21868 bytes --] diff --git a/include/programs/xmalloc.h b/include/programs/xmalloc.h index 76e345f4ee..33871e22ef 100644 --- a/include/programs/xmalloc.h +++ b/include/programs/xmalloc.h @@ -23,11 +23,11 @@ /* Prototypes for a few program-wide used functions. */ extern void *xmalloc (size_t n) - __attribute_malloc__ __attribute_alloc_size__ ((1)); + __attribute_malloc__ __attribute_alloc_size__ ((1)) __attr_dealloc_free; extern void *xcalloc (size_t n, size_t s) - __attribute_malloc__ __attribute_alloc_size__ ((1, 2)); + __attribute_malloc__ __attribute_alloc_size__ ((1, 2)) __attr_dealloc_free; extern void *xrealloc (void *o, size_t n) - __attribute_malloc__ __attribute_alloc_size__ ((2)); -extern char *xstrdup (const char *) __attribute_malloc__; + __attribute_malloc__ __attribute_alloc_size__ ((2)) __attr_dealloc_free; +extern char *xstrdup (const char *) __attribute_malloc__ __attr_dealloc_free; #endif /* xmalloc.h */ diff --git a/libio/Makefile b/libio/Makefile index ed0ce4ee81..73f731e064 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -61,7 +61,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \ bug-ungetc2 bug-ftell bug-ungetc3 bug-ungetc4 tst-fopenloc2 \ tst-memstream1 tst-memstream2 tst-memstream3 tst-memstream4 \ tst-wmemstream1 tst-wmemstream2 tst-wmemstream3 tst-wmemstream4 \ - bug-memstream1 bug-wmemstream1 \ + tst-wmemstream5 bug-memstream1 bug-wmemstream1 \ tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \ tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \ diff --git a/libio/stdio.h b/libio/stdio.h index 144137cf67..4ac82ae5ff 100644 --- a/libio/stdio.h +++ b/libio/stdio.h @@ -165,22 +165,34 @@ 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); + +#undef __attr_dealloc_fclose +#define __attr_dealloc_fclose __attr_dealloc (fclose, 1) + /* 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 __wur; #else # ifdef __REDIRECT -extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) __wur; +extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) + __attribute_malloc__ __attr_dealloc_fclose __wur; # else # define tmpfile tmpfile64 # endif #endif #ifdef __USE_LARGEFILE64 -extern FILE *tmpfile64 (void) __wur; +extern FILE *tmpfile64 (void) + __attribute_malloc__ __attr_dealloc_fclose __wur; #endif /* Generate a temporary filename. */ @@ -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; + __THROW __attribute_malloc__ __wur __attr_dealloc_free; #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 +250,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 __wur; /* Open a file, replacing an existing stream with it. This function is a possible cancellation point and therefore not @@ -256,7 +263,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 __wur; extern FILE *__REDIRECT (freopen, (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream), freopen64) @@ -268,7 +275,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 __wur; extern FILE *freopen64 (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream) __wur; @@ -276,7 +284,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 __wur; #endif #ifdef __USE_GNU @@ -284,21 +293,30 @@ 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 __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 __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 __wur; + +#ifdef _WCHAR_H +/* Like OPEN_MEMSTREAM, but the stream is wide oriented and produces + a wide character string. Declared here only to add attribute malloc + and only if <wchar.h> has been previously #included. */ +extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t *__sizeloc) __THROW + __attribute_malloc__ __attr_dealloc_fclose; +# endif #endif - /* If BUF is NULL, make STREAM unbuffered. Else make it use buffer BUF, of size BUFSIZ. */ extern void setbuf (FILE *__restrict __stream, char *__restrict __buf) __THROW; @@ -792,17 +810,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 660882a28a..c8bc0a3d07 100644 --- a/libio/tst-freopen.c +++ b/libio/tst-freopen.c @@ -81,6 +81,42 @@ do_test_basic (void) fclose (f); } +#if defined __GNUC__ && __GNUC__ >= 11 +/* Force an error to detect incorrectly making freopen a deallocator + for its last argument via attribute malloc. The function closes + the stream without deallocating it so either the argument or + the pointer returned from the function (but not both) can be passed + to fclose. */ +#pragma GCC diagnostic push +#pragma GCC diagnostic error "-Wmismatched-dealloc" +#endif + +/* 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 the no-argument attribute + malloc (which could 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); +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Pop -Wmismatched-dealloc set to error above. */ +# pragma GCC diagnostic pop +#endif + /* Test for BZ#21398, where it tries to freopen stdio after the close of its file descriptor. */ static void @@ -105,6 +141,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/libio/tst-wmemstream1.c b/libio/tst-wmemstream1.c index f8b308bc6c..f80655b2a4 100644 --- a/libio/tst-wmemstream1.c +++ b/libio/tst-wmemstream1.c @@ -1,5 +1,40 @@ #include <wchar.h> +extern int fclose (FILE*); + +#if defined __GNUC__ && __GNUC__ >= 11 +/* 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"). */ +#pragma GCC diagnostic push +#pragma GCC diagnostic error "-Wmismatched-dealloc" +#endif + +int test_open_wmemstream_no_stdio (void) +{ + { + wchar_t *buf; + size_t size; + FILE *f = open_wmemstream (&buf, &size); + fclose (f); + } + + { + FILE* (*pf)(wchar_t**, size_t*) = open_wmemstream; + wchar_t *buf; + size_t size; + FILE *f = pf (&buf, &size); + fclose (f); + } + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Restore -Wmismatched-dealloc setting. */ +# pragma GCC diagnostic pop +#endif + #define CHAR_T wchar_t #define W(o) L##o #define OPEN_MEMSTREAM open_wmemstream diff --git a/libio/tst-wmemstream5.c b/libio/tst-wmemstream5.c new file mode 100644 index 0000000000..47f5e63603 --- /dev/null +++ b/libio/tst-wmemstream5.c @@ -0,0 +1,57 @@ +/* Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <wchar.h> + +extern int fclose (FILE*); + +#if defined __GNUC__ && __GNUC__ >= 11 +/* 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. */ +#pragma GCC diagnostic push +#pragma GCC diagnostic error "-Wmismatched-dealloc" +#endif + +static int +do_test (void) +{ + { + wchar_t *buf; + size_t size; + FILE *f = open_wmemstream (&buf, &size); + fclose (f); + } + + { + FILE* (*pf)(wchar_t**, size_t*) = open_wmemstream; + wchar_t *buf; + size_t size; + FILE *f = pf (&buf, &size); + fclose (f); + } + + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Restore -Wmismatched-dealloc setting. */ +# pragma GCC diagnostic pop +#endif + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/malloc/malloc.h b/malloc/malloc.h index e9157ddfc3..c1c0896d29 100644 --- a/malloc/malloc.h +++ b/malloc/malloc.h @@ -56,22 +56,25 @@ __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2)); the same pointer that was passed to it, aliasing needs to be allowed 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)); + __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2, 3)) + __attr_dealloc_free; /* Free a block allocated by `malloc', `realloc' or `calloc'. */ extern void free (void *__ptr) __THROW; /* Allocate SIZE bytes allocated to ALIGNMENT bytes. */ extern void *memalign (size_t __alignment, size_t __size) -__THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur; + __THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur + __attr_dealloc_free; /* Allocate SIZE bytes on a page boundary. */ extern void *valloc (size_t __size) __THROW __attribute_malloc__ - __attribute_alloc_size__ ((1)) __wur; + __attribute_alloc_size__ ((1)) __wur __attr_dealloc_free; /* Equivalent to valloc(minimum-page-that-holds(n)), that is, round up __size to nearest pagesize. */ -extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ __wur; +extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ + __wur __attr_dealloc_free; /* Underlying allocation function; successive calls should return contiguous pieces of memory. */ diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index 8e244a77cf..3c4e6baf81 100644 --- a/misc/sys/cdefs.h +++ b/misc/sys/cdefs.h @@ -592,11 +592,22 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf array according to access mode, or at least one element when size-index is not provided: access (access-mode, <ref-index> [, <size-index>]) */ -#define __attr_access(x) __attribute__ ((__access__ x)) +# define __attr_access(x) __attribute__ ((__access__ x)) #else # 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 + /* Specify that a function such as setjmp or vfork may return twice. */ #if __GNUC_PREREQ (4, 1) diff --git a/stdlib/Makefile b/stdlib/Makefile index b3b30ab73e..ec18673e5a 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -86,7 +86,8 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \ tst-makecontext-align test-bz22786 tst-strtod-nan-sign \ tst-swapcontext1 tst-setcontext4 tst-setcontext5 \ tst-setcontext6 tst-setcontext7 tst-setcontext8 \ - tst-setcontext9 tst-bz20544 tst-canon-bz26341 + tst-setcontext9 tst-bz20544 tst-canon-bz26341 \ + tst-realpath tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \ tst-tls-atexit tst-tls-atexit-nodelete diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h index 6360845d98..0481c12355 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) + __THROW __attr_dealloc (reallocarray, 1); +#endif #ifdef __USE_MISC # include <alloca.h> @@ -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 __wur; #endif #if defined __USE_MISC || defined __USE_XOPEN_EXTENDED diff --git a/stdlib/tst-realpath.c b/stdlib/tst-realpath.c new file mode 100644 index 0000000000..48a42110be --- /dev/null +++ b/stdlib/tst-realpath.c @@ -0,0 +1,80 @@ +/* Test to verify that realpath() doesn't cause false positives due + to GCC attribute malloc. + + Test failure exposes the presence of the attribute in the following + declaration: + + __attribute__ ((__malloc__ (free, 1))) char* + realpath (const char *, char *); + + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <stdlib.h> + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Turn GCC -Wmismatched-dealloc warnings into errors to expose false + positives. */ +# pragma GCC diagnostic push +# pragma GCC diagnostic error "-Wmismatched-dealloc" + +/* Associate dealloc as the only deallocator suitable for pointers + returned from alloc. + GCC automatically disables inlining of allocator and deallocator + functions marked with the argument form of attribute malloc but + it doesn't hurt to disable it explicitly. */ +__attribute ((noipa)) void dealloc (void *); +__attribute ((malloc (dealloc, 1))) char* alloc (void); +#endif + +void dealloc (void *p) +{ + free (p); +} + +char* alloc (void) +{ + return (char *)malloc (8); +} + +static int +do_test (void) +{ + char *resolved_path = alloc (); + char *ret = realpath ("/", resolved_path); + dealloc (ret); + + resolved_path = alloc (); + ret = realpath ("/", resolved_path); + dealloc (resolved_path); + + /* The following should emit a warning (but doesn't with GCC 11): + resolved_path = alloc (); + ret = realpath ("/", resolved_path); + free (ret); // expect -Wmismatched-dealloc + */ + + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Restore -Wmismatched-dealloc setting. */ +# pragma GCC diagnostic pop +#endif + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h index ce0acb1c28..075776890f 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-04-23 0:00 ` Martin Sebor @ 2021-05-06 23:54 ` Martin Sebor 2021-05-13 21:49 ` Martin Sebor 0 siblings, 1 reply; 27+ messages in thread From: Martin Sebor @ 2021-05-06 23:54 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Joseph Myers 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-05-06 23:54 ` Martin Sebor @ 2021-05-13 21:49 ` Martin Sebor 2021-05-16 21:25 ` Martin Sebor 0 siblings, 1 reply; 27+ messages in thread From: Martin Sebor @ 2021-05-13 21:49 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Joseph Myers [-- Attachment #1: Type: text/plain, Size: 8331 bytes --] 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 >> > [-- Attachment #2: glibc-attr-malloc.diff --] [-- Type: text/x-patch, Size: 21658 bytes --] diff --git a/include/programs/xmalloc.h b/include/programs/xmalloc.h index 76e345f4ee..33871e22ef 100644 --- a/include/programs/xmalloc.h +++ b/include/programs/xmalloc.h @@ -23,11 +23,11 @@ /* Prototypes for a few program-wide used functions. */ extern void *xmalloc (size_t n) - __attribute_malloc__ __attribute_alloc_size__ ((1)); + __attribute_malloc__ __attribute_alloc_size__ ((1)) __attr_dealloc_free; extern void *xcalloc (size_t n, size_t s) - __attribute_malloc__ __attribute_alloc_size__ ((1, 2)); + __attribute_malloc__ __attribute_alloc_size__ ((1, 2)) __attr_dealloc_free; extern void *xrealloc (void *o, size_t n) - __attribute_malloc__ __attribute_alloc_size__ ((2)); -extern char *xstrdup (const char *) __attribute_malloc__; + __attribute_malloc__ __attribute_alloc_size__ ((2)) __attr_dealloc_free; +extern char *xstrdup (const char *) __attribute_malloc__ __attr_dealloc_free; #endif /* xmalloc.h */ diff --git a/libio/Makefile b/libio/Makefile index ed0ce4ee81..73f731e064 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -61,7 +61,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \ bug-ungetc2 bug-ftell bug-ungetc3 bug-ungetc4 tst-fopenloc2 \ tst-memstream1 tst-memstream2 tst-memstream3 tst-memstream4 \ tst-wmemstream1 tst-wmemstream2 tst-wmemstream3 tst-wmemstream4 \ - bug-memstream1 bug-wmemstream1 \ + tst-wmemstream5 bug-memstream1 bug-wmemstream1 \ tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \ tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \ diff --git a/libio/stdio.h b/libio/stdio.h index 76bda3728e..497da016ff 100644 --- a/libio/stdio.h +++ b/libio/stdio.h @@ -165,22 +165,34 @@ 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); + +#undef __attr_dealloc_fclose +#define __attr_dealloc_fclose __attr_dealloc (fclose, 1) + /* 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 __wur; #else # ifdef __REDIRECT -extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) __wur; +extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) + __attribute_malloc__ __attr_dealloc_fclose __wur; # else # define tmpfile tmpfile64 # endif #endif #ifdef __USE_LARGEFILE64 -extern FILE *tmpfile64 (void) __wur; +extern FILE *tmpfile64 (void) + __attribute_malloc__ __attr_dealloc_fclose __wur; #endif /* Generate a temporary filename. */ @@ -202,15 +214,9 @@ extern char *tmpnam_r (char __s[L_tmpnam]) __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; + __THROW __attribute_malloc__ __wur __attr_dealloc_free; #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 +250,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 __wur; /* Open a file, replacing an existing stream with it. This function is a possible cancellation point and therefore not @@ -256,7 +263,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 __wur; extern FILE *__REDIRECT (freopen, (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream), freopen64) @@ -268,7 +275,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 __wur; extern FILE *freopen64 (const char *__restrict __filename, const char *__restrict __modes, FILE *__restrict __stream) __wur; @@ -276,7 +284,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 __wur; #endif #ifdef __USE_GNU @@ -284,21 +293,30 @@ 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 __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 __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 __wur; + +#ifdef _WCHAR_H +/* Like OPEN_MEMSTREAM, but the stream is wide oriented and produces + a wide character string. Declared here only to add attribute malloc + and only if <wchar.h> has been previously #included. */ +extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t *__sizeloc) __THROW + __attribute_malloc__ __attr_dealloc_fclose; +# endif #endif - /* If BUF is NULL, make STREAM unbuffered. Else make it use buffer BUF, of size BUFSIZ. */ extern void setbuf (FILE *__restrict __stream, char *__restrict __buf) __THROW; @@ -792,17 +810,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 660882a28a..c8bc0a3d07 100644 --- a/libio/tst-freopen.c +++ b/libio/tst-freopen.c @@ -81,6 +81,42 @@ do_test_basic (void) fclose (f); } +#if defined __GNUC__ && __GNUC__ >= 11 +/* Force an error to detect incorrectly making freopen a deallocator + for its last argument via attribute malloc. The function closes + the stream without deallocating it so either the argument or + the pointer returned from the function (but not both) can be passed + to fclose. */ +#pragma GCC diagnostic push +#pragma GCC diagnostic error "-Wmismatched-dealloc" +#endif + +/* 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 the no-argument attribute + malloc (which could 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); +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Pop -Wmismatched-dealloc set to error above. */ +# pragma GCC diagnostic pop +#endif + /* Test for BZ#21398, where it tries to freopen stdio after the close of its file descriptor. */ static void @@ -105,6 +141,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/libio/tst-wmemstream1.c b/libio/tst-wmemstream1.c index f8b308bc6c..f80655b2a4 100644 --- a/libio/tst-wmemstream1.c +++ b/libio/tst-wmemstream1.c @@ -1,5 +1,40 @@ #include <wchar.h> +extern int fclose (FILE*); + +#if defined __GNUC__ && __GNUC__ >= 11 +/* 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"). */ +#pragma GCC diagnostic push +#pragma GCC diagnostic error "-Wmismatched-dealloc" +#endif + +int test_open_wmemstream_no_stdio (void) +{ + { + wchar_t *buf; + size_t size; + FILE *f = open_wmemstream (&buf, &size); + fclose (f); + } + + { + FILE* (*pf)(wchar_t**, size_t*) = open_wmemstream; + wchar_t *buf; + size_t size; + FILE *f = pf (&buf, &size); + fclose (f); + } + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Restore -Wmismatched-dealloc setting. */ +# pragma GCC diagnostic pop +#endif + #define CHAR_T wchar_t #define W(o) L##o #define OPEN_MEMSTREAM open_wmemstream diff --git a/libio/tst-wmemstream5.c b/libio/tst-wmemstream5.c new file mode 100644 index 0000000000..47f5e63603 --- /dev/null +++ b/libio/tst-wmemstream5.c @@ -0,0 +1,57 @@ +/* Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <wchar.h> + +extern int fclose (FILE*); + +#if defined __GNUC__ && __GNUC__ >= 11 +/* 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. */ +#pragma GCC diagnostic push +#pragma GCC diagnostic error "-Wmismatched-dealloc" +#endif + +static int +do_test (void) +{ + { + wchar_t *buf; + size_t size; + FILE *f = open_wmemstream (&buf, &size); + fclose (f); + } + + { + FILE* (*pf)(wchar_t**, size_t*) = open_wmemstream; + wchar_t *buf; + size_t size; + FILE *f = pf (&buf, &size); + fclose (f); + } + + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Restore -Wmismatched-dealloc setting. */ +# pragma GCC diagnostic pop +#endif + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/malloc/malloc.h b/malloc/malloc.h index e9157ddfc3..c1c0896d29 100644 --- a/malloc/malloc.h +++ b/malloc/malloc.h @@ -56,22 +56,25 @@ __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2)); the same pointer that was passed to it, aliasing needs to be allowed 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)); + __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2, 3)) + __attr_dealloc_free; /* Free a block allocated by `malloc', `realloc' or `calloc'. */ extern void free (void *__ptr) __THROW; /* Allocate SIZE bytes allocated to ALIGNMENT bytes. */ extern void *memalign (size_t __alignment, size_t __size) -__THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur; + __THROW __attribute_malloc__ __attribute_alloc_size__ ((2)) __wur + __attr_dealloc_free; /* Allocate SIZE bytes on a page boundary. */ extern void *valloc (size_t __size) __THROW __attribute_malloc__ - __attribute_alloc_size__ ((1)) __wur; + __attribute_alloc_size__ ((1)) __wur __attr_dealloc_free; /* Equivalent to valloc(minimum-page-that-holds(n)), that is, round up __size to nearest pagesize. */ -extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ __wur; +extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ + __wur __attr_dealloc_free; /* Underlying allocation function; successive calls should return contiguous pieces of memory. */ diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index 11f01f741b..30a621ab8f 100644 --- a/misc/sys/cdefs.h +++ b/misc/sys/cdefs.h @@ -603,6 +603,17 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf # define __attr_access_none(argno) #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(dealloc, argno) +# define __attr_dealloc_free +#endif + /* Specify that a function such as setjmp or vfork may return twice. */ #if __GNUC_PREREQ (4, 1) diff --git a/stdlib/Makefile b/stdlib/Makefile index b3b30ab73e..ec18673e5a 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -86,7 +86,8 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \ tst-makecontext-align test-bz22786 tst-strtod-nan-sign \ tst-swapcontext1 tst-setcontext4 tst-setcontext5 \ tst-setcontext6 tst-setcontext7 tst-setcontext8 \ - tst-setcontext9 tst-bz20544 tst-canon-bz26341 + tst-setcontext9 tst-bz20544 tst-canon-bz26341 \ + tst-realpath tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \ tst-tls-atexit tst-tls-atexit-nodelete diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h index 6360845d98..0481c12355 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) + __THROW __attr_dealloc (reallocarray, 1); +#endif #ifdef __USE_MISC # include <alloca.h> @@ -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 __wur; #endif #if defined __USE_MISC || defined __USE_XOPEN_EXTENDED diff --git a/stdlib/tst-realpath.c b/stdlib/tst-realpath.c new file mode 100644 index 0000000000..2ae5e4fb2b --- /dev/null +++ b/stdlib/tst-realpath.c @@ -0,0 +1,82 @@ +/* Test to verify that realpath() doesn't cause false positives due + to GCC attribute malloc. + + Test failure exposes the presence of the attribute in the following + declaration: + + __attribute__ ((__malloc__ (free, 1))) char* + realpath (const char *, char *); + + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <stdio.h> +#include <stdlib.h> +#include <malloc.h> + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Turn GCC -Wmismatched-dealloc warnings into errors to expose false + positives. */ +# pragma GCC diagnostic push +# pragma GCC diagnostic error "-Wmismatched-dealloc" + +/* Associate dealloc as the only deallocator suitable for pointers + returned from alloc. + GCC automatically disables inlining of allocator and deallocator + functions marked with the argument form of attribute malloc but + it doesn't hurt to disable it explicitly. */ +__attribute ((noipa)) void dealloc (void *); +__attribute ((malloc (dealloc, 1))) char* alloc (void); +#endif + +void dealloc (void *p) +{ + free (p); +} + +char* alloc (void) +{ + return (char *)malloc (8); +} + +static int +do_test (void) +{ + char *resolved_path = alloc (); + char *ret = realpath ("/", resolved_path); + dealloc (ret); + + resolved_path = alloc (); + ret = realpath ("/", resolved_path); + dealloc (resolved_path); + + /* The following should emit a warning (but doesn't with GCC 11): + resolved_path = alloc (); + ret = realpath ("/", resolved_path); + free (ret); // expect -Wmismatched-dealloc + */ + + return 0; +} + +#if defined __GNUC__ && __GNUC__ >= 11 +/* Restore -Wmismatched-dealloc setting. */ +# pragma GCC diagnostic pop +#endif + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h index ce0acb1c28..075776890f 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] add support for -Wmismatched-dealloc 2021-05-13 21:49 ` Martin Sebor @ 2021-05-16 21:25 ` Martin Sebor 0 siblings, 0 replies; 27+ messages in thread From: Martin Sebor @ 2021-05-16 21:25 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Joseph Myers 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 >>> >> > ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2021-05-16 21:25 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-08 22:52 [PATCH] add support for -Wmismatched-dealloc Martin Sebor 2020-12-09 0:07 ` Joseph Myers 2020-12-12 2:25 ` Martin Sebor 2020-12-14 21:39 ` Martin Sebor 2020-12-14 22:16 ` Florian Weimer 2020-12-15 1:01 ` Joseph Myers 2020-12-15 16:52 ` Martin Sebor 2020-12-27 23:13 ` Martin Sebor 2021-01-04 15:56 ` Ping: " Martin Sebor 2021-01-04 16:07 ` Florian Weimer 2021-01-04 16:18 ` Martin Sebor 2021-01-04 16:57 ` Florian Weimer 2021-01-04 23:18 ` Martin Sebor 2021-01-10 20:42 ` Ping: " Martin Sebor 2021-01-11 9:13 ` Florian Weimer 2021-01-12 0:00 ` Martin Sebor 2021-01-12 0:01 ` Martin Sebor 2021-01-12 8:59 ` Florian Weimer 2021-01-19 1:08 ` Martin Sebor 2021-01-19 16:54 ` David Malcolm 2021-01-22 21:26 ` DJ Delorie 2021-01-25 10:56 ` Florian Weimer 2021-01-25 11:31 ` Florian Weimer 2021-04-23 0:00 ` Martin Sebor 2021-05-06 23:54 ` Martin Sebor 2021-05-13 21:49 ` Martin Sebor 2021-05-16 21:25 ` Martin Sebor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).