From patchwork Tue Dec 8 22:52:59 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 41341 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5F79A3851C17; Tue, 8 Dec 2020 22:53:04 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5F79A3851C17 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1607467984; bh=fXngpkYqK3nMNneG0yAKh4Oy0TkyD44E7r8DNsLKsVE=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=ys59wjpSiUNe3A8aM7w+74TSxoXTVxFNRRRfu2H8DpXv/gnx9eOCtlPvClJXZGhLa rtIJkDlujpM7Lp3JR+xAiC7Yxkl7o/I2ELT1NpKHv3ppI2m7F/bhR3VzrHdMaPwuDn NnOgMmxxAb6KQttXaTMCbPm1ossm3gdqefuYEhX0= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-qt1-x82c.google.com (mail-qt1-x82c.google.com [IPv6:2607:f8b0:4864:20::82c]) by sourceware.org (Postfix) with ESMTPS id 8F7923857003 for ; Tue, 8 Dec 2020 22:53:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8F7923857003 Received: by mail-qt1-x82c.google.com with SMTP id k4so12047qtj.10 for ; Tue, 08 Dec 2020 14:53:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:from:subject:message-id:date:user-agent :mime-version:content-language; bh=fXngpkYqK3nMNneG0yAKh4Oy0TkyD44E7r8DNsLKsVE=; b=nT44pNKwYtCWbP6kIESkhHZWq5koZC0zJ93/PTfC6NHRuQYWdUX7hZwCXbGd0cVWp8 b0ABmNpcBQWFvMTcA1UiTsDMIYsS28O6kXRF+Uqy3fd34887EbcNAUoGLd5aFCff20V9 4x5hEp1Ru9zUuvtLjaei4dKtd6sg/JYS4sgpBsvbVI0WJuRlg6XDuQ7JeXBKXC/jXXtX WX/7WkGiPkuHToOTFsMnc3o0iEWugdN7okvx3aLnFRma1JSrNJz0OJaAmQ3HaDm210P/ cj7N4tOdYFSkwV88kk0sADWWlXFzCJ9fTeWdsdA+3UUoYHBFEYizHZf6YV/Pqt1tQFu1 gc9A== X-Gm-Message-State: AOAM532Rgu6nFYlVX1HCjkfBsmOKkMhM7XPfgzUdDzPg8nabi8xiyQwI MpOm/B1F8+7NTt21QKRpPUCN5wZa0ME= X-Google-Smtp-Source: ABdhPJwHFvo4Guor3V1SrRpE/1LwvRAVAIPaDMYlnvMH+dlIdTNrl6RS0XwcHtmrjzQZh2r61XHCzw== X-Received: by 2002:aed:2084:: with SMTP id 4mr393551qtb.81.1607467980961; Tue, 08 Dec 2020 14:53:00 -0800 (PST) Received: from [192.168.0.41] (174-16-97-231.hlrn.qwest.net. [174.16.97.231]) by smtp.gmail.com with ESMTPSA id 6sm124213qko.3.2020.12.08.14.53.00 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 08 Dec 2020 14:53:00 -0800 (PST) To: GNU C Library Subject: [PATCH] add support for -Wmismatched-dealloc Message-ID: Date: Tue, 8 Dec 2020 15:52:59 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 Content-Language: en-US X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Martin Sebor via Libc-alpha From: Martin Sebor Reply-To: Martin Sebor Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" 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 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 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;