[PATCH] add support for -Wmismatched-dealloc
Martin Sebor
msebor@gmail.com
Thu May 13 21:49:41 GMT 2021
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
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: glibc-attr-malloc.diff
Type: text/x-patch
Size: 21658 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20210513/709e589e/attachment-0001.bin>
More information about the Libc-alpha
mailing list