From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id 05D2F3850413 for ; Mon, 11 Jan 2021 09:13:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 05D2F3850413 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-426-wygsEr5WOieJoH7QeqlVkw-1; Mon, 11 Jan 2021 04:13:25 -0500 X-MC-Unique: wygsEr5WOieJoH7QeqlVkw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8B6D01005E41; Mon, 11 Jan 2021 09:13:24 +0000 (UTC) Received: from oldenburg2.str.redhat.com (ovpn-113-100.ams2.redhat.com [10.36.113.100]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8327919746; Mon, 11 Jan 2021 09:13:23 +0000 (UTC) From: Florian Weimer To: Martin Sebor Cc: Martin Sebor via Libc-alpha , Joseph Myers Subject: Re: [PATCH] add support for -Wmismatched-dealloc References: <74efece7-9a4b-83ee-7fdd-475c0d514378@gmail.com> <758e723b-67cf-a211-7bc2-2ccd3fc744e5@gmail.com> <2555516b-4583-21fc-e844-fd44619488cd@gmail.com> <655918b2-16c6-74b1-6a49-505a7607007f@gmail.com> <87mtxok7ob.fsf@oldenburg2.str.redhat.com> <0aae9006-6001-8fc8-ad6d-c8e3ee60f82c@gmail.com> <87turwiqqw.fsf@oldenburg2.str.redhat.com> Date: Mon, 11 Jan 2021 10:13:21 +0100 In-Reply-To: (Martin Sebor's message of "Mon, 4 Jan 2021 16:18:07 -0700") Message-ID: <87czybsuoe.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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: Mon, 11 Jan 2021 09:13:30 -0000 * 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 requir= es redirection on this platf > # define __attr_access(x) > #endif > =20 > +#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 =E2=80=9Cmalloc=E2=80=9D, the macro name sho= uld 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); > } > =20 > +#if defined __GNUC__ && __GNUC__ >=3D 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__ >=3D 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 > =20 > +/* Verify that calling fclose on the result of open_wmemstream doesn't > + trigger GCC -Wmismatched-dealloc with fclose forward-declared and > + without included first (it is included later, in. > + "tst-memstream1.c"). */ > + > +extern int fclose (FILE*); > + > +#if defined __GNUC__ && __GNUC__ >=3D 11 > +#pragma GCC diagnostic push > +#pragma GCC diagnostic error "-Wmismatched-dealloc" > +#endif Likewise. > +#if defined __GNUC__ && __GNUC__ >=3D 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 > + > +/* Verify that calling fclose on the result of open_wmemstream doesn't > + trigger GCC -Wmismatched-dealloc with fclose forward-declared and > + without included in the same translation unit. */ > + > +extern int fclose (FILE*); > + > +#if defined __GNUC__ && __GNUC__ >=3D 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 *__na= me) > ENAMETOOLONG; if the name fits in fewer than PATH_MAX chars, > returns the name in RESOLVED. */ > extern char *realpath (const char *__restrict __name, > -=09=09 char *__restrict __resolved) __THROW __wur; > +=09=09 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, > =09=09=09 size_t __n, locale_t __loc) __THROW; > =20 > /* 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 > =20 > /* 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. */ > =20 > #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 ) 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 > =20 > #if defined __USE_ISOC95 || defined __USE_UNIX98 Does this mean that if one includes first and then 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 if has already been included? Thanks, Florian --=20 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'N= eill