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

[Martin Sebor via Libc-alpha <libc-alpha@sourceware.org>]

Florian/Joseph and/or others: is the latest patch okay to commit?

https://sourceware.org/pipermail/libc-alpha/2020-December/121121.html

On 12/27/20 4:13 PM, Martin Sebor wrote:
> More testing made me realize that further changes are needed:
> 1) correct the return value of the __fclose() alias to int,
> 2) declare and use the same alias for fclose in both <stdio.h>
>     and <wchar.h>.
> 
> In addition, I noticed a few more opportunities to use the new
> attribute:
>   *  in include/programs/xmalloc.h,
>   *  in malloc/malloc.h,
>   *  and in wcsdup in <wchar.h>.
> 
> I also simplified the new macro definitions a bit, and added
> a new test to verify that the warning doesn't cause false
> positives for open_wmemstream.
> 
> Attached is a patch with these updates.
> 
> On 12/15/20 9:52 AM, Martin Sebor wrote:
>> On 12/14/20 6:01 PM, Joseph Myers wrote:
>>> On Mon, 14 Dec 2020, Martin Sebor via Libc-alpha wrote:
>>>
>>>>> 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 GCC patches have now been committed and the dependency resolved.
>>>
>>> I've looked at the attribute documentation now in GCC, but I'm afraid 
>>> I'm
>>> unable to understand from that documentation why the proposed glibc 
>>> patch
>>> constitutes a valid way of specifying that, for example, it's valid 
>>> to use
>>> freopen as a deallocator for FILE pointers opened by functions whose
>>> attribute only mentions fclose.  Unless there's something I'm missing in
>>> the documentation or a separate documentation patch that's not yet
>>> committed, I think more work is needed on the GCC documentation to make
>>> clear the semantics the glibc patch is asserting for valid 
>>> combinations of
>>> allocators and deallocators, so that those semantics can be reviewed for
>>> correctness.
>>
>> I flip-flopped with freopen.  Initially I wanted to mark it up as
>> both an allocator and a deallocator, analogously to realloc (which
>> is implicitly both) or reallocarray (which is annotated as both in
>> the latest Glibc patch).  Both the initial Glibc and GCC patches
>> (the manual for the latter) reflected this and had freopen annotated
>> that way.
>>
>> But because freopen doesn't actually deallocate or allocate a stream
>> the markup wouldn't be correct.  It would cause false positives with
>> -Wmismatched-dealloc as well with other warnings like the future
>> -Wuse-after-free (or with -Wanalyzer-use-after-free when the GCC
>> analyzer adds support for the attribute that David Malcolm is
>> working on for GCC 11).  I've added a test case to the test suite:
>>
>>    void f (FILE *f1)
>>    {
>>      FILE *f2 = freopen ("", "", f1);
>>      fclose (f1);   // must not warn
>>    }
>>
>> To answer your question, without the attribute freopen is seen by
>> GCC as an ordinary function that happens to take a FILE* and return
>> another FILE*.  It neither allocates it nor deallocates it.  For
>> GCC 12, I'd like us to consider adding attribute returns_arg(position)
>> to improve the analysis here.  The GCC manual also doesn't mention
>> freopen anymore but I'd be happy to change the example there to
>> show an API that does include a reallocator (e.g., reallocarray).
>>
>> Having said all this, after double-checking the latest Glibc patch
>> I see it still has the attribute on freopen by mistake (as well as
>> the ordinary attribute malloc, which would make it even worse).
>> I've removed both in the attached revision.  Sorry if this confused
>> you -- freopen obviously confused me.
>>
>> Martin
>