add support for -Wmismatched-dealloc

  To help detect common kinds of memory (and other resource) management
bugs, GCC 11 adds support for the detection of mismatched calls to
allocation and deallocation functions.  At each call site to a known
deallocation function GCC checks the set of allocation functions
the former can be paired with and, if the two don't match, issues
a -Wmismatched-dealloc warning (something similar happens in C++
for mismatched calls to new and delete).  GCC also uses the same
mechanism to detect attempts to deallocate objects not allocated
by any allocation function (or pointers past the first byte into
allocated objects) by -Wfree-nonheap-object.

This support is enabled for built-in functions like malloc and free.
To extend it beyond those, GCC extends attribute malloc to designate
a deallocation function to which pointers returned from the allocation
function may be passed to deallocate the allocated objects.  Another,
optional argument designates the positional argument to which
the pointer must be passed.

The attached change is the first step in enabling this extended support
for Glibc.  It enables GCC to diagnose mismatched calls such as in
tst-popen.c1:

   FILE *f = popen ("foo", "r");
   ...
   fclose (f);

   warning: ‘fclose’ called on pointer returned from a mismatched 
allocation function [-Wmismatched-dealloc]
      83 |     fclose (f);
         |     ^~~~~~~~~~
   note: returned from ‘popen’
      81 |     FILE *f = popen ("foo", "r");
         |               ^~~~~~~~~~~~~~~~~~

Since <stdio.h> doesn't declare free() and realloc() the patch uses
__builtin_free and __builtin_realloc as the deallocators.  This isn't
pretty but I couldn't think of a more elegant way to do it.  (I also
missed this bit in the initial implementation of the attribute so
current trunk doesn't handle the __builtin_xxx forms.  I submitted
a patch to handle it just today, but to avoid delays, I post this
solution for comments ahead of its acceptance).

Since the deallocation function referenced by the malloc attribute
must have been declared, the changes require rearranging the order
of the declarations a bit.

I tested the changes by rebuilding Glibc on x86_64 and by verifying
that warnings are issued where appropriate for my little test program
but not much else.  Neither set of changes (either in GCC or in Glibc)
impacts code generation and should make no difference in testing
(I didn't see any.)

If there is support for this approach I'll work on extending it to
other headers, hopefully before the upcoming Glibc release.

Thanks
Martin
  

I don't see any definition of __attr_dealloc (presumably should be a macro 
in misc/sys/cdefs.h) in this patch (or in the glibc source tree).

Given all the functions in stdio.h with the same list of deallocation 
functions, there should probably be another macro there to expand to 
__attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen, 
3) __attr_dealloc_freopen64.  (That would also apply to open_wmemstream in 
wchar.h, but I suppose you have the issue there with functions not being 
declared in wchar.h, only in stdio.h?)
  

On 12/8/20 5:07 PM, Joseph Myers wrote:
> I don't see any definition of __attr_dealloc (presumably should be a macro
> in misc/sys/cdefs.h) in this patch (or in the glibc source tree).

Whoops!

> 
> Given all the functions in stdio.h with the same list of deallocation
> functions, there should probably be another macro there to expand to
> __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen,
> 3) __attr_dealloc_freopen64.  (That would also apply to open_wmemstream in
> wchar.h, but I suppose you have the issue there with functions not being
> declared in wchar.h, only in stdio.h?)

You're right that adding the attribute to open_wmemstream runs into
the same problem as declaring the ctermid argument with L_ctermid
elements in <unistd.h>: fclose isn't declared in <wchar.h>.  I did
some more work on adding the attribute to other functions and found
out that the same problem also happens between tempnam() in <stdio.h>
and reallocarray() in <stdlib.h>.

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 attached patch implements this for <stdio.h>, <stdlib.h>, and
<wchar.h>.  To get around the <wchar.h> dependency on <stdio.h> it
uses __REDIRECT to introduce a reserved alias for fclose.

Besides running the test suite I tested it with my own test and also
by adding the same declarations to the GCC test suite and verifying
it triggers warnings as expected.

The GCC patches needed to make this simpler scheme work haven't been
reviewed yet so this work has a dependency on them getting approved.

I grepped for __attribute_malloc__ in Glibc headers to see if there
are other APIs that would benefit from the same annotation but found
none.  At the same time, I don't have the impression that malloc is
used on all the APIs it could be.  Are there any that you or anyone
else can think of that might be worth looking at?

Martin
  

On 12/11/20 7:25 PM, Martin Sebor wrote:
> On 12/8/20 5:07 PM, Joseph Myers wrote:
>> I don't see any definition of __attr_dealloc (presumably should be a 
>> macro
>> in misc/sys/cdefs.h) in this patch (or in the glibc source tree).
> 
> Whoops!
> 
>>
>> Given all the functions in stdio.h with the same list of deallocation
>> functions, there should probably be another macro there to expand to
>> __attribute_malloc__ __attr_dealloc (fclose, 1) __attr_dealloc (freopen,
>> 3) __attr_dealloc_freopen64.  (That would also apply to 
>> open_wmemstream in
>> wchar.h, but I suppose you have the issue there with functions not being
>> declared in wchar.h, only in stdio.h?)
> 
> You're right that adding the attribute to open_wmemstream runs into
> the same problem as declaring the ctermid argument with L_ctermid
> elements in <unistd.h>: fclose isn't declared in <wchar.h>.  I did
> some more work on adding the attribute to other functions and found
> out that the same problem also happens between tempnam() in <stdio.h>
> and reallocarray() in <stdlib.h>.
> 
> 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 attached patch implements this for <stdio.h>, <stdlib.h>, and
> <wchar.h>.  To get around the <wchar.h> dependency on <stdio.h> it
> uses __REDIRECT to introduce a reserved alias for fclose.
> 
> Besides running the test suite I tested it with my own test and also
> by adding the same declarations to the GCC test suite and verifying
> it triggers warnings as expected.
> 
> The GCC patches needed to make this simpler scheme work haven't been
> reviewed yet so this work has a dependency on them getting approved.

The GCC patches have now been committed and the dependency resolved.

Florian asked this morning if getaddrinfo() and freeaddrinfo() are
covered by this change.  They're not because getaddrinfo() returns
the allocated memory indirectly, via an argument.  To handle those
kinds of APIs that return a pointer to the allocated object
indirectly, through an argument, the attribute will need to be
enhanced somehow.

> 
> I grepped for __attribute_malloc__ in Glibc headers to see if there
> are other APIs that would benefit from the same annotation but found
> none.  At the same time, I don't have the impression that malloc is
> used on all the APIs it could be.  Are there any that you or anyone
> else can think of that might be worth looking at?
> 
> Martin
  

* Martin Sebor:

> Florian asked this morning if getaddrinfo() and freeaddrinfo() are
> covered by this change.  They're not because getaddrinfo() returns
> the allocated memory indirectly, via an argument.  To handle those
> kinds of APIs that return a pointer to the allocated object
> indirectly, through an argument, the attribute will need to be
> enhanced somehow.

asprintf is another such function, perhaps slightly more commonly used.
It would be nice if this interface pattern could be covered as well.

Thanks,
Florian
  

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.
  

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
  

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
  

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
>
  

* Martin Sebor via Libc-alpha:

> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h
> index 9cf8b05a87..4c1c7f1119 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,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict __dest,
>  /* Wide character I/O functions.  */
>  
>  #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
> +#  ifdef __REDIRECT
> +/* Declare the __fclose alias and associate it as a deallocator
> +   with open_wmemstream below.  */
> +extern int __REDIRECT (__fclose, (FILE *), fclose);
> +#    define __attr_dealloc_fclose __attr_dealloc (__fclose, 1)
> +#  else
> +#    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

Why is an alias for fclose needed here, but not for free?

Thanks,
Florian
  

On 1/4/21 9:07 AM, Florian Weimer wrote:
> * Martin Sebor via Libc-alpha:
> 
>> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h
>> index 9cf8b05a87..4c1c7f1119 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,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict __dest,
>>   /* Wide character I/O functions.  */
>>   
>>   #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
>> +#  ifdef __REDIRECT
>> +/* Declare the __fclose alias and associate it as a deallocator
>> +   with open_wmemstream below.  */
>> +extern int __REDIRECT (__fclose, (FILE *), fclose);
>> +#    define __attr_dealloc_fclose __attr_dealloc (__fclose, 1)
>> +#  else
>> +#    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
> 
> Why is an alias for fclose needed here, but not for free?

Because fclose is not a built-in so there's no __builtin_fclose
to associate open_wmemstream with.  free is a built-in and so
__attr_dealloc_free just references __builtin_free and doesn't
need an explicit declaration.

Martin

> 
> Thanks,
> Florian
>
  

* Martin Sebor:

> On 1/4/21 9:07 AM, Florian Weimer wrote:
>> * Martin Sebor via Libc-alpha:
>> 
>>> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h
>>> index 9cf8b05a87..4c1c7f1119 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,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict __dest,
>>>   /* Wide character I/O functions.  */
>>>     #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
>>> +#  ifdef __REDIRECT
>>> +/* Declare the __fclose alias and associate it as a deallocator
>>> +   with open_wmemstream below.  */
>>> +extern int __REDIRECT (__fclose, (FILE *), fclose);
>>> +#    define __attr_dealloc_fclose __attr_dealloc (__fclose, 1)
>>> +#  else
>>> +#    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
>> Why is an alias for fclose needed here, but not for free?
>
> Because fclose is not a built-in so there's no __builtin_fclose
> to associate open_wmemstream with.  free is a built-in and so
> __attr_dealloc_free just references __builtin_free and doesn't
> need an explicit declaration.

Ahh, that explains the discrepancy.

I'm a bit worried that the __fclose alias causes problems.  Would it be
possible to add __builtin_fclose to GCC instead?

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.

Thanks,
Florian
  

On 1/4/21 9:57 AM, Florian Weimer wrote:
> * Martin Sebor:
> 
>> On 1/4/21 9:07 AM, Florian Weimer wrote:
>>> * Martin Sebor via Libc-alpha:
>>>
>>>> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h
>>>> index 9cf8b05a87..4c1c7f1119 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,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict __dest,
>>>>    /* Wide character I/O functions.  */
>>>>      #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
>>>> +#  ifdef __REDIRECT
>>>> +/* Declare the __fclose alias and associate it as a deallocator
>>>> +   with open_wmemstream below.  */
>>>> +extern int __REDIRECT (__fclose, (FILE *), fclose);
>>>> +#    define __attr_dealloc_fclose __attr_dealloc (__fclose, 1)
>>>> +#  else
>>>> +#    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
>>> Why is an alias for fclose needed here, but not for free?
>>
>> Because fclose is not a built-in so there's no __builtin_fclose
>> to associate open_wmemstream with.  free is a built-in and so
>> __attr_dealloc_free just references __builtin_free and doesn't
>> need an explicit declaration.
> 
> Ahh, that explains the discrepancy.
> 
> I'm a bit worried that the __fclose alias causes problems.  Would it be
> possible to add __builtin_fclose to GCC instead?

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

The alias isn't necessary in <stdio.h> where fclose is declared
so I've removed it from there.  It would have been only marginally
useful in <wchar.h>, and only when fclose isn't declared, but it's
probably best avoided.  So one possibility is to prepare the header
for __builtin_fclose if/when it's added, fall back on fclose when
it's declared (i.e., when <stdio.h> is included), and do nothing
otherwise (and accept that calling, say free, on a pointer returned
from open_wmemstteam, will not be diagnosed in those translation
units).

Attached is a patch that does that.  If you want to change it
to something else (e.g, forget about open_wmemstream altogether
for now or conditionally declare it in <stdio.h> when <wchar.h>
is included, or any other viable alternative) please let me know.

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

As for different configurations, the attribute is designed for
standard C/POSIX APIs and others like those.  A user-defined API
with different deallocators in one configuration than in another
has to create the associations conditionally, based on those
configurations.  But there's no way to change these associations
for GCC built-ins, just like there's no way to change their
semantics.  They're hardwired into GCC and the only way to
affect them is to disable the built-ins.

Martin

> 
> Thanks,
> Florian
>
  

Florian, do you have any outstanding concerns or is the most recent
patch good to go?

Thanks
Martin

On 1/4/21 4:18 PM, Martin Sebor wrote:
> On 1/4/21 9:57 AM, Florian Weimer wrote:
>> * Martin Sebor:
>>
>>> On 1/4/21 9:07 AM, Florian Weimer wrote:
>>>> * Martin Sebor via Libc-alpha:
>>>>
>>>>> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h
>>>>> index 9cf8b05a87..4c1c7f1119 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,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict 
>>>>> __dest,
>>>>>    /* Wide character I/O functions.  */
>>>>>      #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
>>>>> +#  ifdef __REDIRECT
>>>>> +/* Declare the __fclose alias and associate it as a deallocator
>>>>> +   with open_wmemstream below.  */
>>>>> +extern int __REDIRECT (__fclose, (FILE *), fclose);
>>>>> +#    define __attr_dealloc_fclose __attr_dealloc (__fclose, 1)
>>>>> +#  else
>>>>> +#    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
>>>> Why is an alias for fclose needed here, but not for free?
>>>
>>> Because fclose is not a built-in so there's no __builtin_fclose
>>> to associate open_wmemstream with.  free is a built-in and so
>>> __attr_dealloc_free just references __builtin_free and doesn't
>>> need an explicit declaration.
>>
>> Ahh, that explains the discrepancy.
>>
>> I'm a bit worried that the __fclose alias causes problems.  Would it be
>> possible to add __builtin_fclose to GCC instead?
> 
> 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?)
> 
> The alias isn't necessary in <stdio.h> where fclose is declared
> so I've removed it from there.  It would have been only marginally
> useful in <wchar.h>, and only when fclose isn't declared, but it's
> probably best avoided.  So one possibility is to prepare the header
> for __builtin_fclose if/when it's added, fall back on fclose when
> it's declared (i.e., when <stdio.h> is included), and do nothing
> otherwise (and accept that calling, say free, on a pointer returned
> from open_wmemstteam, will not be diagnosed in those translation
> units).
> 
> Attached is a patch that does that.  If you want to change it
> to something else (e.g, forget about open_wmemstream altogether
> for now or conditionally declare it in <stdio.h> when <wchar.h>
> is included, or any other viable alternative) please let me know.
> 
>> 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).
> 
> As for different configurations, the attribute is designed for
> standard C/POSIX APIs and others like those.  A user-defined API
> with different deallocators in one configuration than in another
> has to create the associations conditionally, based on those
> configurations.  But there's no way to change these associations
> for GCC built-ins, just like there's no way to change their
> semantics.  They're hardwired into GCC and the only way to
> affect them is to disable the built-ins.
> 
> Martin
> 
>>
>> Thanks,
>> Florian
>>
>
  

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

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.

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

> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index 5fb6e309be..21e6177fac 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h
> @@ -569,6 +569,17 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf
>  #  define __attr_access(x)
>  #endif
>  
> +#if __GNUC_PREREQ (11, 0)
> +/* Designates dealloc as a function to call to deallocate objects
> +   allocated by the declared function.  */
> +# define __attr_dealloc(dealloc, argno) \
> +    __attribute__ ((__malloc__ (dealloc, argno)))
> +# define __attr_dealloc_free __attr_dealloc (__builtin_free, 1)
> +#else
> +# define __attr_dealloc(x)
> +# define __attr_dealloc_free
> +#endif

If the GCC attribute is called “malloc”, the macro name should reflect
that.  I think it would make sense to make __attribute_malloc__
redundant, so that the headers contain only one of the two attributes
per declaration.  The new macro could expand to the argument-less
version for older GCC versions.

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

> +  /* This shouldn't trigger -Wmismatched-dealloc.  */
> +  fclose (f1);

> +#if defined __GNUC__ && __GNUC__ >= 11
> +# pragma GCC diagnostic pop
> +#endif

Likewise.

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

> +#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.

> +#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.

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

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

Maybe it is possible to redeclare open_wmemstream in <stdio.h> if
<wchar.h> has already been included?

Thanks,
Florian
  

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
  

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
  

* Martin Sebor via Libc-alpha:

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

Maybe David can comment on how this interacts with his static analyzer
work.  In all other cases, the attribute means that the pointer needs to
be freed to avoid a resource leak.  If we suddenly apply it pointers
which can only conditionally be freed, that reduces the value of those
annotations, I think.

Thanks,
Florian
  

David, now that you've committed the analyzer support for
the extended attribute malloc, can you please comment on Florian's
concern about using it to annotate realpath() as in the patch below?

https://sourceware.org/pipermail/libc-alpha/2021-January/121527.html

(I tested how it interacts with -Wmismatched-dealloc but I haven't
yet had a chance to see how the annotations might interact with
the analyzer warnings.)

Thanks
Martin

On 1/12/21 1:59 AM, Florian Weimer wrote:
> * Martin Sebor via Libc-alpha:
> 
>>> 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).
> 
> Maybe David can comment on how this interacts with his static analyzer
> work.  In all other cases, the attribute means that the pointer needs to
> be freed to avoid a resource leak.  If we suddenly apply it pointers
> which can only conditionally be freed, that reduces the value of those
> annotations, I think.
> 
> Thanks,
> Florian
>
  

On Tue, 2021-01-12 at 09:59 +0100, Florian Weimer wrote:
> * Martin Sebor via Libc-alpha:
> 
> > > 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).
> 
> Maybe David can comment on how this interacts with his static
> analyzer
> work.  

BTW, the -fanalyzer part of support for
__attribute__((malloc(deallocator))) is in gcc 11 as of yesterday:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=c7e276b869bdeb4a95735c1f037ee1a5f629de3d

> In all other cases, the attribute means that the pointer needs to
> be freed to avoid a resource leak.

Indeed, the analyzer doesn't have any special knowledge of realpath and
the conditional behavior.  Given that annotation it will assume that
the returned value is either a pointer that needs to be freed, or NULL
on a failure, with no knowledge that the 2nd argument could be
returned.

I haven't tested Martin's patch, but I tried this example:

$ cat t.c
#include <stdio.h>

#define PATH_MAX 4096
void free(void *ptr);
char *realpath(const char *path, char *resolved_path)
  __attribute__ ((malloc (free)))
  __attribute__ ((__warn_unused_result__));

void test_1 (const char *path)
{
  char buf[PATH_MAX];
  char *result = realpath (path, buf);
  printf ("result: %s\n", result);
}

void test_2 (const char *path)
{
  char buf[PATH_MAX];
  char *result = realpath (path, buf);
  printf ("result: %s\n", result);
  free (result);
}

I believe test_1 is correct (although redundant in its use of "result"
rather than buf, and can output a truncated path).

I believe test_2 is a crasher bug: a "free" of on-stack "buf".

Compiling with GCC 11 (with the __attribute__((malloc (DEALLOCATOR)))
support:

$ ./xgcc -B. -c -fanalyzer t.c
t.c: In function ‘test_1’:
t.c:14:1: warning: leak of ‘result’ [CWE-401] [-Wanalyzer-malloc-leak]
   14 | }
      | ^
  ‘test_1’: events 1-2
    |
    |   12 |   char *result = realpath (path, buf);
    |      |                  ^~~~~~~~~~~~~~~~~~~~
    |      |                  |
    |      |                  (1) allocated here
    |   13 |   printf ("result: %s\n", result);
    |   14 | }
    |      | ~                 
    |      | |
    |      | (2) ‘result’ leaks here; was allocated at (1)
    |

Here it falsely complains about test_1; it doesn't "know" that buf is
returned by realpath and assumes that result needs to be freed.

In test_2, there's a free of result == buf i.e. a free of an on-stack
buffer, which it doesn't complain about, treating "result" as a malloc-
ed pointer (as specified by the attribute).

So I don't think this attribute should be applied to realpath.

  If we suddenly apply it pointers
> which can only conditionally be freed, that reduces the value of
> those
> annotations, I think.

FWIW, the analyzer already special-cases some functions; see the
various region_model::impl_call_* functions in:
 https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/analyzer/region-model-impl-calls.cc

There's a case for doing this for stuff in POSIX, which would apply
here, I think.

As noted above, I haven't tested Martin's glibc patch (I don't think
I'm subscribed to this list).

> Thanks,
> Florian

Hope this is constructive
Dave
  

* Martin Sebor via Libc-alpha:


> diff --git a/libio/stdio.h b/libio/stdio.h
> index 144137cf67..62814c93db 100644
> --- a/libio/stdio.h
> +++ b/libio/stdio.h

> @@ -202,15 +214,9 @@ extern char *tmpnam_r (char *__s) __THROW __wur;
>     P_tmpdir is tried and finally "/tmp".  The storage for the filename
>     is allocated by `malloc'.  */
>  extern char *tempnam (const char *__dir, const char *__pfx)
> -     __THROW __attribute_malloc__ __wur;
> +  __attribute_malloc__ __wur __attr_dealloc_free __THROW;
>  #endif

This breaks boostrap because in some C++ modes, __THROW (aka noexcept)
needs to come before all attributes.

Further tests are ongoing; this is difficult to do due to general
toolchain breakage (although things have improved lately).

Thanks,
Florian
  

* Martin Sebor via Libc-alpha:

> diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
> index 6360845d98..a555fb9c0f 100644
> --- a/stdlib/stdlib.h
> +++ b/stdlib/stdlib.h
> @@ -550,6 +550,9 @@ extern void *calloc (size_t __nmemb, size_t __size)
>  extern void *realloc (void *__ptr, size_t __size)
>       __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2));
>  
> +/* Free a block allocated by `malloc', `realloc' or `calloc'.  */
> +extern void free (void *__ptr) __THROW;
> +
>  #ifdef __USE_MISC
>  /* Re-allocate the previously allocated block in PTR, making the new
>     block large enough for NMEMB elements of SIZE bytes each.  */
> @@ -558,11 +561,13 @@ extern void *realloc (void *__ptr, size_t __size)
>     between objects pointed by the old and new pointers.  */
>  extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
>       __THROW __attribute_warn_unused_result__
> -     __attribute_alloc_size__ ((2, 3));
> -#endif
> +     __attribute_alloc_size__ ((2, 3))
> +    __attr_dealloc_free;
>  
> -/* Free a block allocated by `malloc', `realloc' or `calloc'.  */
> -extern void free (void *__ptr) __THROW;
> +/* Add reallocarray as its own deallocator.  */
> +extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
> +     __attr_dealloc (reallocarray, 1);
> +#endif

This causes:

In file included from ../include/stdlib.h:15,
                 from ../test-skeleton.c:36,
                 from test-math-iszero.cc:164:
../stdlib/stdlib.h:568:14: error: declaration of ‘void* reallocarray(void*, size_t, size_t)’ has a different exception specifier
  568 | extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
      |              ^~~~~~~~~~~~
../stdlib/stdlib.h:562:14: note: from previous declaration ‘void* reallocarray(void*, size_t, size_t) noexcept’
  562 | extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
      |              ^~~~~~~~~~~~

Thanks,
Florian
  

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
  

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
>
  

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

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