Re: [PATCH] add support for -Wmismatched-dealloc

[Martin Sebor <msebor@gmail.com>]

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
>>>
>>
>