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

[Martin Sebor <msebor@gmail.com>]

On 1/11/21 2:13 AM, Florian Weimer wrote:
> * Martin Sebor:
> 
>> I don't like the alias hack either.  Adding a built-in is possible
>> but it's late in GCC stage 3 and I'm doubtful the change would be
>> accepted before the Glibc deadline (that's this week, right?)
> 
> I think we still have some time.
> 
> One problem is that the annotations do not permit diagnosing memory
> leaks: realloc is a deallocator for malloc, xfclose is one for fopen,
> and so on.  But the annotations do not capture this.  So if they are
> used for diagnosing leaks, false positives will be a result.  Listing
> all potential deallocators in the allocator does not scale.  It also
> leads to the problem shown by open_wmemstream.

Florian and I discussed this in IRC and cleared most things up
so this is largely just for the benefit of others.  I'll post
a new patch with the suggested changes separately from this.

The attribute is used to detect other problems besides allocator
and deallocator mismatches (e.g., -Wfree-nonheap-object), and
should also already be in principle used by analyzer warnings
like -Wanalyzer-double-fclose and -Wanalyzer-double-free.  David
Malcolm prototyped a similar attribute in the analyzer first and
now is in the process of adjusting the analyzer to work with this
one.

The open_wmemstream problem is due to the <wchar.h> header not
having access to the fclose declaration (because it's only in
<stdio.h>).  Declaring open_wmemstream with the attribute also
in <stdio.h> (when <wchar.h> is included) solves it.

Finally [and this qualifies what I said to Florian this morning],
GCC makes it possible to avoid having to associate every allocator
with every matching deallocator for reallocators only (like realloc).
This was done to simplify its use in Glibc headers.  But no such
assumption is made about straight (non-deallocating) allocators
and they do have to be explicitly associated with all their
deallocators.

> I think allocators need to assign some sort of pool identifier to
> allocated pointers, and deallocators should declare on which pools they
> operate.  This would allow adding additional deallocators such as
> xfclose.

Other than the simplification for realloc there's no notion of
a "pool."  Supporting it wasn't a goal (the subject of the request
in GCC PR 94527).  The design could be relatively easily changed
to avoid having to associate every allocator with every deallocator
but almost certainly not for GCC 11.

>>> Based on how this patch appears to make both __fclose and fclose
>>> acceptable as a deallocator, GCC resolves redirects as part of the
>>> matching check.  I wonder if this constrains the usefulness of the
>>> attribute in some way.  I can imagine situations where at the source
>>> level, different deallocators should be used (say to support debugging
>>> builds), but release builds redirect different deallocators to the same
>>> implementation.
>>
>> The attribute doesn't do anything special with aliases (it was
>> just a way to get around the problem with functions not being
>> declared in some headers).
> 
> But it has to do something with them, otherwise __fclose and fclose are
> not the same deallocator and thus different functions.

Yes, they are different.  That's also why I agree it's not a good
solution.

I've snipped the rest of your comments and replied to them separately
to make it easier to focus on just the code changes.

Martin