[PATCH] add support for -Wmismatched-dealloc

[Martin Sebor]

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: 10913 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20201215/a337becd/attachment-0001.bin>