From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x242.google.com (mail-oi1-x242.google.com [IPv6:2607:f8b0:4864:20::242]) by sourceware.org (Postfix) with ESMTPS id D0A523844040 for ; Sat, 12 Dec 2020 02:25:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D0A523844040 Received: by mail-oi1-x242.google.com with SMTP id q25so12189707oij.10 for ; Fri, 11 Dec 2020 18:25:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=o1S6U7ogePyiP5kGQqapBRV+GF4TZKVqMcEpXua/5cY=; b=n/rdtGcnzXxwvKr2JhInqac9dN+0JXeVHegVrGCs5MrRP7tIsZZWq4YSHpA3qcDI5G doJ2QP3okpppaTawGSOCcn+A0PT/TO7KSHh6S+AbRUxFNz8SSi3yZ20RpASo4OuMEjN4 Ax5gv2Fbv88Yl/EpFf52FwOzAO2gFYrnwDEWfpGkkWASVqCIoLgJG7tGdekqMZun/Sz7 KJEGPeg7iIkBZokSoSIVBLK67eZRMIP09b3IP8xdmnV2KkI/SGfD+jB6qwXkvvzB63+K oK9ljhiOiAecLeFoYz6uHcVAUPKjabok51xVbeUg5r0zY6/YZb2W1dHi4u/ueDRnoyrk 9nQQ== X-Gm-Message-State: AOAM533PueTv1iomB7EyKmg+CI5xWVX37QAxeyUlqC5Fwj3J7rZJ3ORC 70mLaTb5PTU0i6DELDLJydlmiloINps= X-Google-Smtp-Source: ABdhPJzHWvewkDRjB+Q/eWS0Q9OC9XuQDAR6Njt3ZqbXafaOLyK+7lio8yqT4gTJ6Lcgx9zJMgtQrw== X-Received: by 2002:aca:5792:: with SMTP id l140mr11579742oib.175.1607739940010; Fri, 11 Dec 2020 18:25:40 -0800 (PST) Received: from [192.168.0.41] (174-16-97-231.hlrn.qwest.net. [174.16.97.231]) by smtp.gmail.com with ESMTPSA id m21sm2257524oos.28.2020.12.11.18.25.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 11 Dec 2020 18:25:39 -0800 (PST) From: Martin Sebor Subject: Re: [PATCH] add support for -Wmismatched-dealloc To: Joseph Myers Cc: GNU C Library References: Message-ID: <74efece7-9a4b-83ee-7fdd-475c0d514378@gmail.com> Date: Fri, 11 Dec 2020 19:25:37 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------FD4CD012C0ACABEA8BE29C1D" Content-Language: en-US X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 12 Dec 2020 02:25:43 -0000 This is a multi-part message in MIME format. --------------FD4CD012C0ACABEA8BE29C1D Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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 : fclose isn't declared in . I did some more work on adding the attribute to other functions and found out that the same problem also happens between tempnam() in and reallocarray() in . 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 , , and . To get around the dependency on 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 --------------FD4CD012C0ACABEA8BE29C1D Content-Type: text/x-patch; charset=UTF-8; name="glibc-attr-malloc.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="glibc-attr-malloc.diff" diff --git a/libio/stdio.h b/libio/stdio.h index 998470943e..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 @@ -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 --------------FD4CD012C0ACABEA8BE29C1D--