[PATCH] add support for -Wmismatched-dealloc

[Martin Sebor]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: glibc-attr-malloc.diff
Type: text/x-patch
Size: 17060 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20201227/df73ad63/attachment-0001.bin>