Add comments to explain when a stream is freed by, __libc_freeres().

  In bug 26022 a user reports that valgrind shows two unfreed
allocations when using stderr.  This is normal, we don't free
unbuffered streams in __libc_freeres() because users expect
that such streams will be available all the way through the
process shutdown.

To suppress this I've filed the following valgrind issue:
https://bugs.kde.org/show_bug.cgi?id=421931
---
 libio/genops.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)
  

* Carlos O'Donell via Libc-alpha:

> In bug 26022 a user reports that valgrind shows two unfreed
> allocations when using stderr.  This is normal, we don't free
> unbuffered streams in __libc_freeres() because users expect
> that such streams will be available all the way through the
> process shutdown.
>
> To suppress this I've filed the following valgrind issue:
> https://bugs.kde.org/show_bug.cgi?id=421931

I don't think these changes go in the right direction at all, sorry.

Consider the definition of _IO_doallocbuf in in libio/genops.c (which is
on the stack for the leaked allocation, as reported by valgrind):

| void
| _IO_doallocbuf (FILE *fp)
| {
|   if (fp->_IO_buf_base)
|     return;
|   if (!(fp->_flags & _IO_UNBUFFERED) || fp->_mode > 0)
|     if (_IO_DOALLOCATE (fp) != EOF)
|       return;
|   _IO_setb (fp, fp->_shortbuf, fp->_shortbuf+1, 0);
| }
| libc_hidden_def (_IO_doallocbuf)

The key part is the “fp->_mode > 0”.  It does not matter if the stream
is unbuffered or not if it is a wide stream.

The code in __libc_freeres is simply not aligned with this behavior of
_IO_doallocbuf.

I believe _IO_doallocbuf behaves this way because the libio character
conversion always needs a buffer, even for nominally unbuffered streams.

Thanks,
Florian
  

On 5/25/20 11:30 AM, Florian Weimer wrote:
> * Carlos O'Donell via Libc-alpha:
> 
>> In bug 26022 a user reports that valgrind shows two unfreed
>> allocations when using stderr.  This is normal, we don't free
>> unbuffered streams in __libc_freeres() because users expect
>> that such streams will be available all the way through the
>> process shutdown.
>>
>> To suppress this I've filed the following valgrind issue:
>> https://bugs.kde.org/show_bug.cgi?id=421931
> 
> I don't think these changes go in the right direction at all, sorry.

No need to apologize.

I assume, and please clarify if I get this wrong, that your suggestion
is for valgrind to close bug 421931 as CLOSED/WONTFIX?

From your analysis below I surmise your position is as follows, but
again please clarify if I've misunderstood.

* glibc should shutdown and free all streams in __libc_freeres()
  regardless of narrow vs. wide or buffered vs. unbuffered.

  - Fixing this involves fixing buffer_free() to correctly shutdown
    all stream.

  - Today only mtrace calls __libc_freeres() via release_libc_mem()
    via __cxa_atexit() and so even if valgrind calls it after the last
    thread exits, we still have one use in glibc.  We have
    never prevented allocation tracers from calling __libc_freeres from
    destructors and we should not require that now.

Until such a fix is in place though, and for all existing already
deployed glibc, should valgrind still implement suppression for the
unfreed unbuffered stderr missed by __libc_freeres?

> Consider the definition of _IO_doallocbuf in in libio/genops.c (which is
> on the stack for the leaked allocation, as reported by valgrind):
> 
> | void
> | _IO_doallocbuf (FILE *fp)
> | {
> |   if (fp->_IO_buf_base)
> |     return;
> |   if (!(fp->_flags & _IO_UNBUFFERED) || fp->_mode > 0)

Isn't this a layering violation?

Worse, it might be a typo...

commit 655de5fdf21929f7f11d2307b13aeb66a1b47181
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Mon Sep 25 05:12:05 2000 +0000
...
            * libio/genops.c (_IO_doallocbuf): Don't use single byte buffer if
            stream is in wide mode.
...

diff --git a/libio/genops.c b/libio/genops.c
index 42419bf508..c86adee4c0 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -368,7 +368,7 @@ _IO_doallocbuf (fp)
 {
   if (fp->_IO_buf_base)
     return;
-  if (!(fp->_flags & _IO_UNBUFFERED))
+  if (!(fp->_flags & _IO_UNBUFFERED) || fp->_mode > 0)
     if (_IO_DOALLOCATE (fp) != EOF)
       return;
   _IO_setb (fp, fp->_shortbuf, fp->_shortbuf+1, 0);
---
Should it have been:

if (!((fp->_flags & _IO_UNBUFFERED) || fp->_mode > 0))

with the extra parenthesis added?

None of the routines in genops.c or wgenops.c knows anything about
needing to allocate the buffers for wide mode converions with gconv,
that's all in fileops.c and wfileops.c.

> |     if (_IO_DOALLOCATE (fp) != EOF)
> |       return;
> |   _IO_setb (fp, fp->_shortbuf, fp->_shortbuf+1, 0);
> | }
> | libc_hidden_def (_IO_doallocbuf)
> 
> The key part is the “fp->_mode > 0”.  It does not matter if the stream
> is unbuffered or not if it is a wide stream.

In _IO_wdoallocbuf we actively try to avoid creating the buffer specifically
based on _IO_UNBUFFERED.

However, you are correct, we always allocate both buffers for wide streams
in buffered or unbuffered.

In libio/wgenops.c(_IO_wdoallocbuf):

365 void
366 _IO_wdoallocbuf (FILE *fp)
367 {
368   if (fp->_wide_data->_IO_buf_base)
369     return;
370   if (!(fp->_flags & _IO_UNBUFFERED))
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

371     if ((wint_t)_IO_WDOALLOCATE (fp) != WEOF)
372       return;
373   _IO_wsetb (fp, fp->_wide_data->_shortbuf,
374                      fp->_wide_data->_shortbuf + 1, 0);

We temporarily set the short buffer for unbuffered cases, assuming
that we only need 1 wchar_t output at a time.

375 }
376 libc_hidden_def (_IO_wdoallocbuf)

It is called from here in libio/wfileops.c (_IO_wfile_overflow):

 419       /* Allocate a buffer if needed. */
 420       if (f->_wide_data->_IO_write_base == 0)
 421         {
 422           _IO_wdoallocbuf (f);

Here we allocate the wide buffer conditional on buffering, and for
unbuffered we setup the short buffer (1 wchar_t), but for fully
buffered we call into _IO_wfile_doallocate to allocate both narrow
and wide buffers as required by the underlying implementation for
file-based streams.

 423           _IO_free_wbackup_area (f);
 424           _IO_wsetg (f, f->_wide_data->_IO_buf_base,
 425                      f->_wide_data->_IO_buf_base, f->_wide_data->_IO_buf_base);
 426 
 427           if (f->_IO_write_base == NULL)
 428             {
 429               _IO_doallocbuf (f);

If we *have* written something to the stream, then we won't be doing
this allocation? I don't even know if it is possible to get here without
first having allocated things.

If we haven't written anything yet we do the non-wide generic operation
which is to allocate the buffer. In the unbuffered wide mode case this
has that '|| fp->_mode > 0' that means we allocate both buffers. So no matter
what we set for buffering the wide mode streams both narrow and wide buffers
are always allocated. That seems odd.

 430               _IO_setg (f, f->_IO_buf_base, f->_IO_buf_base, f->_IO_buf_base);
 431             }
 432         }

So in the case of file-based wide mode streams:

* Conditionally allocate the wide mode buffer in wide mode (!_IO_UNBUFFERED)
  - Use shortbuf.
* Unconditionally allocate the narrow and wide mode buffers from the generic code.
  - For files allocate both buffers.
  - Is this a 20 year old typo?
* For file-backed wide mode streams the concept of _IO_UNBUFFERED makes no
  difference.

So buffered vs. unbuffered does make a difference.

> The code in __libc_freeres is simply not aligned with this behavior of
> _IO_doallocbuf.

What behaviour do we want?

libio/genops.c (_IO_unbuffer_all):

 815       if (! (fp->_flags & _IO_UNBUFFERED)
 816           && (legacy || fp->_mode != 0))

To me this looks like we violated layering here and added a "|| fp->_mode != 0"
which is a round-about way of saying "We know the file-based wide stream
implementation uses both buffers." This logic should probably be in a higher
level and not visible to genops.c.

This above says "Deallocate the narrow or wide activated streams that are buffered."

That is our current strategy.

As you point out "unbuffered" for file-based wide streams is to allocate both
buffers.

Can we specialize genops.c code to use file-based streams information?

I know that today only file-based streams are using these routines in genops.c
and wgenops.c, but it seems like a violation of layering if we actively put
in knowledge about the file-based implementation.

Should it instead become something else?

e.g.

(fp->_mode != 0)

This says "Deallocate all activated streams."

In this scenario we would be changing __libc_freeres()'s behaviour for
shutdown, particularly any user of this as an allocation tracking framework
would immediately stop being able to use unbuffered streams to write data
to disk.

Only valgrind with the more advanced thread control is able to terminate
all threads, and then from the non-pthread thread handle all the cleanup.
We don't do anything like that for mtrace, and we do call __libc_freeres
from there.

The alternative is that this is all a typo, and we should not allocate the
buffer for the unbuffered case, and so wouldn't need to free it?

> I believe _IO_doallocbuf behaves this way because the libio character
> conversion always needs a buffer, even for nominally unbuffered streams.

Agreed.

In summary:

- We need to choose what we do with the streams when __libc_freeres() is
  called.

  (a) Leave existing behaviour as-is, which frees only buffered streams
      during __libc_freeres(), but this always leaves visible leaks for
      unbuffered wide-mode streams like stderr (until we fix that typo?)

  (b) Change behaviour of __libc_freeres to always free all streams.

- Is there a typo in the code in libio/genops.c?
  

* Carlos O'Donell:

> From your analysis below I surmise your position is as follows, but
> again please clarify if I've misunderstood.

I have thought about this mostly from first principles, I have not
looked at the code too closely.

> * glibc should shutdown and free all streams in __libc_freeres()
>   regardless of narrow vs. wide or buffered vs. unbuffered.

Not shut down, but deallocate everything that can be reallocated if
needed.  This way, if something later uses the standard handles, it can
do so.

For buffers, I think it should be possible to perform the deallocation.

For the locale converters (not visible in the reproducer), it probably
is not.  Charset names have variable length, so even if we free the
decoder, something needs to remain to be able to recreate the previous
state.

>   - Fixing this involves fixing buffer_free() to correctly shutdown
>     all stream.

While maintaining a consistent state, yes.

> Until such a fix is in place though, and for all existing already
> deployed glibc, should valgrind still implement suppression for the
> unfreed unbuffered stderr missed by __libc_freeres?

I'm not sure if valgrind has this flexibility.

>> Consider the definition of _IO_doallocbuf in in libio/genops.c (which is
>> on the stack for the leaked allocation, as reported by valgrind):
>> 
>> | void
>> | _IO_doallocbuf (FILE *fp)
>> | {
>> |   if (fp->_IO_buf_base)
>> |     return;
>> |   if (!(fp->_flags & _IO_UNBUFFERED) || fp->_mode > 0)
>
> Isn't this a layering violation?

Well, it's libio, so isn't this expected?

> Worse, it might be a typo...
>
> commit 655de5fdf21929f7f11d2307b13aeb66a1b47181
> Author: Ulrich Drepper <drepper@redhat.com>
> Date:   Mon Sep 25 05:12:05 2000 +0000
> ...
>             * libio/genops.c (_IO_doallocbuf): Don't use single byte buffer if
>             stream is in wide mode.
> ...
>
> diff --git a/libio/genops.c b/libio/genops.c
> index 42419bf508..c86adee4c0 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -368,7 +368,7 @@ _IO_doallocbuf (fp)
>  {
>    if (fp->_IO_buf_base)
>      return;
> -  if (!(fp->_flags & _IO_UNBUFFERED))
> +  if (!(fp->_flags & _IO_UNBUFFERED) || fp->_mode > 0)
>      if (_IO_DOALLOCATE (fp) != EOF)
>        return;
>    _IO_setb (fp, fp->_shortbuf, fp->_shortbuf+1, 0);
> ---
> Should it have been:
>
> if (!((fp->_flags & _IO_UNBUFFERED) || fp->_mode > 0))
>
> with the extra parenthesis added?

I think the code matches the quoted ChangeLog entry.

> None of the routines in genops.c or wgenops.c knows anything about
> needing to allocate the buffers for wide mode converions with gconv,
> that's all in fileops.c and wfileops.c.

Maybe the wide stream code could set up _IO_buf_base earlier, and not
rely on lazy allocation via _IO_doallocbuf.

> In this scenario we would be changing __libc_freeres()'s behaviour for
> shutdown, particularly any user of this as an allocation tracking framework
> would immediately stop being able to use unbuffered streams to write data
> to disk.

We should only deallocate stdin/stdout/stderr in this way.  If there are
more streams around, the user neglected to call fclose on them, so there
is already a leak, and the reporting does not improve if we deallocate
their buffers.

I believe buffer deallocation can be handled in a conservative way, so
that the buffer gets reallocated again if needed later during shutdown.

> The alternative is that this is all a typo, and we should not allocate the
> buffer for the unbuffered case, and so wouldn't need to free it?

I'm not sure it is.  But I don't really understand the libio code.

>   (b) Change behaviour of __libc_freeres to always free all streams.

It should free an automatically allocated buffer for stdin/stdout/stderr
(all the internal handles, in various versions).  Full deallocation
seems impossible and undesirable.

Thanks,
Florian
  

On Mai 26 2020, Florian Weimer via Libc-alpha wrote:

> We should only deallocate stdin/stdout/stderr in this way.  If there are
> more streams around, the user neglected to call fclose on them, so there
> is already a leak, and the reporting does not improve if we deallocate
> their buffers.

FILE streams never leak, as they are implicitly closed by exit.

Andreas.
  

* Andreas Schwab:

> On Mai 26 2020, Florian Weimer via Libc-alpha wrote:
>
>> We should only deallocate stdin/stdout/stderr in this way.  If there are
>> more streams around, the user neglected to call fclose on them, so there
>> is already a leak, and the reporting does not improve if we deallocate
>> their buffers.
>
> FILE streams never leak, as they are implicitly closed by exit.

I'm not sure if that is useful.  If the programmer forgets to close them
in a loop, it is still a bug.  Freeing them automatically makes it
harder to track the bug down.

As a quality-of-implementation matter, file streams should be usable
after main has returned (and parts of the standard agrees; others do
not).  We need to flush them, of course.

C11 also mandates that the streams are closed three times if main
returns.  7.2.1.3/5 says:

| If the main function returns to its original caller, or if the exit
| function is called, all open files are closed (hence all output
| streams are flushed) before program termination.

And 5.1.2.2.3/1:

| If the return type of the main function is a type compatible with int,
| a return from the initial call to the main function is equivalent to
| calling the exit function with the value returned by the main function
| as its argument;

The return from main is the first closing operation.  It is specified to
be equivalent to a call to exit, which gives us the next closing
operation.

And the third closing operation is here, in 7.22.4.4/4:

| Next, all open streams with unwritten buffered data are flushed, all
| open streams are closed, and all files created by the tmpfile function
| are removed.

So I don't think “are closed” necessary means calling fclose because
that's undefined.

Thanks,
Florian
  

On Mai 26 2020, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Mai 26 2020, Florian Weimer via Libc-alpha wrote:
>>
>>> We should only deallocate stdin/stdout/stderr in this way.  If there are
>>> more streams around, the user neglected to call fclose on them, so there
>>> is already a leak, and the reporting does not improve if we deallocate
>>> their buffers.
>>
>> FILE streams never leak, as they are implicitly closed by exit.
>
> I'm not sure if that is useful.  If the programmer forgets to close them
> in a loop, it is still a bug.  Freeing them automatically makes it
> harder to track the bug down.

I don't understand what you are after.  Closing streams in exit is a
requirement.

Andreas.
  

* Andreas Schwab:

> On Mai 26 2020, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>> On Mai 26 2020, Florian Weimer via Libc-alpha wrote:
>>>
>>>> We should only deallocate stdin/stdout/stderr in this way.  If there are
>>>> more streams around, the user neglected to call fclose on them, so there
>>>> is already a leak, and the reporting does not improve if we deallocate
>>>> their buffers.
>>>
>>> FILE streams never leak, as they are implicitly closed by exit.
>>
>> I'm not sure if that is useful.  If the programmer forgets to close them
>> in a loop, it is still a bug.  Freeing them automatically makes it
>> harder to track the bug down.
>
> I don't understand what you are after.  Closing streams in exit is a
> requirement.

How can it be?  It's not an observable effect.  No user code runs after
this point, so what difference does it make?  Flushing is a different
matter, of course.

Thanks,
Florian
  

On Mai 26 2020, Florian Weimer wrote:

> How can it be?  It's not an observable effect.  No user code runs after
> this point, so what difference does it make?

I don't understand.  You postulate a leak due to "neglected to call
fclose", but streams are guaranteed to be closed anyway, thus there is
no leak.

Andreas.
  

* Andreas Schwab:

> On Mai 26 2020, Florian Weimer wrote:
>
>> How can it be?  It's not an observable effect.  No user code runs after
>> this point, so what difference does it make?
>
> I don't understand.  You postulate a leak due to "neglected to call
> fclose", but streams are guaranteed to be closed anyway, thus there is
> no leak.

All memory is freed on process termination, so not calling free on
memory allocations does not introduce a leak, either.  Yet we expect
memory debuggers such as valgrind to report them.

__libc_freeres needs to strike a balance between what is technically
possible (in the sense that we may not be able to free all memory due to
the way the termination sequence is ordered), and also what results in
useful reports to users.  I think never reporting streams that have not
been fclose'd is less useful than reporting them.  Not calling fclose is
typically an application bug because you lose error checking, for
instance.

__libc_freeres should mostly deal with things that have been allocated
behind the user's back by glibc, and not things that the user could free
if they wanted.  Streams created by fopen etc. are definitely in the
latter category.

All this is well out of C and POSIX territory, so it does not matter
what they say about closing (given that the effect of calling fclose is
not observable anyway).

Thanks,
Florian
  

On Mai 26 2020, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Mai 26 2020, Florian Weimer wrote:
>>
>>> How can it be?  It's not an observable effect.  No user code runs after
>>> this point, so what difference does it make?
>>
>> I don't understand.  You postulate a leak due to "neglected to call
>> fclose", but streams are guaranteed to be closed anyway, thus there is
>> no leak.
>
> All memory is freed on process termination, so not calling free on
> memory allocations does not introduce a leak, either.  Yet we expect
> memory debuggers such as valgrind to report them.

But unlike general allocations, open streams are guaranteed to be always
reachable (fflush (0) will find them all).

Andreas.
  

* Andreas Schwab:

> On Mai 26 2020, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>> On Mai 26 2020, Florian Weimer wrote:
>>>
>>>> How can it be?  It's not an observable effect.  No user code runs after
>>>> this point, so what difference does it make?
>>>
>>> I don't understand.  You postulate a leak due to "neglected to call
>>> fclose", but streams are guaranteed to be closed anyway, thus there is
>>> no leak.
>>
>> All memory is freed on process termination, so not calling free on
>> memory allocations does not introduce a leak, either.  Yet we expect
>> memory debuggers such as valgrind to report them.
>
> But unlike general allocations, open streams are guaranteed to be always
> reachable (fflush (0) will find them all).

Most general allocations are also reachable.  They are linked from
various free lists, after all.  I don't think that's a useful criterion.

Semaphores created by sem_open are very similar to file streams in that
they are also required to be closed by POSIX, and they have a global
data structure linking them together.  We don't free those via
__libc_freeres as far as I can tell.  I think this is the right thing to
do.

Thanks,
Florian
  

On Mai 26 2020, Florian Weimer wrote:

> Most general allocations are also reachable.  They are linked from
> various free lists, after all.  I don't think that's a useful criterion.

There is no official API to iterate over all allocations.

Andreas.
  

* Andreas Schwab:

> On Mai 26 2020, Florian Weimer wrote:
>
>> Most general allocations are also reachable.  They are linked from
>> various free lists, after all.  I don't think that's a useful criterion.
>
> There is no official API to iterate over all allocations.

There's malloc_info.  It iterates about as much as fflush (0).

I don't quite understand what you are after.  If we free more things in
__libc_freeres, things that the user can deallocate on their own, we
make memory debuggers less useful.  I'm not sure it makes sense to
exclude file streams just because there is an API like fflush (0).

Thanks,
Florian
  

On 5/26/20 3:53 AM, Florian Weimer wrote:
> We should only deallocate stdin/stdout/stderr in this way.  If there are
> more streams around, the user neglected to call fclose on them, so there
> is already a leak, and the reporting does not improve if we deallocate
> their buffers.

Agreed, and I think this is the right strategy here.
 
> I believe buffer deallocation can be handled in a conservative way, so
> that the buffer gets reallocated again if needed later during shutdown.

Agreed.

> It should free an automatically allocated buffer for stdin/stdout/stderr
> (all the internal handles, in various versions).  Full deallocation
> seems impossible and undesirable.

Agreed.

I think that sets consensus for this. I'm not sure I want to prioritize
fixing this right now, but I'll look at the code for a bit more and see
if it's fixable without too much problem.