* [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). @ 2020-05-22 20:37 Carlos O'Donell 2020-05-25 15:30 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2020-05-22 20:37 UTC (permalink / raw) To: 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 --- libio/genops.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/libio/genops.c b/libio/genops.c index 28419cc963..d533d47fd5 100644 --- a/libio/genops.c +++ b/libio/genops.c @@ -768,7 +768,7 @@ weak_alias (_IO_flush_all_linebuffered, _flushlbf) function sin the libc_freeres section. Those are called as part of the atexit routine, just like _IO_cleanup. The problem is we do not know whether the freeres code is called first or _IO_cleanup. - if the former is the case, we set the DEALLOC_BUFFER variable to + if the former is the case, we set the dealloc_buffers variable to true and _IO_unbuffer_all will take care of the rest. If _IO_unbuffer_all is called first we add the streams to a list which the freeres function later can walk through. */ @@ -796,8 +796,23 @@ _IO_unbuffer_all (void) legacy = 1; #endif + /* We free a stream if: + + (1) The stream is buffered and was used (_mode != 0), and + so the user can't expect any output to be printed during + shutdown since absolute order of destructors is not + explicitly guaranteed. + + (2) The stream is buffered and is a legacy stream used for + backwards compatibility with legacy libstdc++ + implementations. + + By default stderr starts unbuffered and so by default we + never process it here, and as such it is always available for + late destructors to use, and will not be freed by the + buffer_free function below (unless it is changed by the + user). */ if (! (fp->_flags & _IO_UNBUFFERED) - /* Iff stream is un-orientated, it wasn't used. */ && (legacy || fp->_mode != 0)) { #ifdef _IO_MTSAFE_IO @@ -844,11 +859,13 @@ _IO_unbuffer_all (void) #endif } - +/* Called by __libc_freeres() to free streams. */ libc_freeres_fn (buffer_free) { dealloc_buffers = true; + /* See the note in _IO_unbuffer_all about when a stream is + considered for freeing. */ while (freeres_list != NULL) { free (freeres_list->_freeres_buf); -- 2.26.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-22 20:37 [PATCH] Add comments to explain when a stream is freed by, __libc_freeres() Carlos O'Donell @ 2020-05-25 15:30 ` Florian Weimer 2020-05-25 21:17 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2020-05-25 15:30 UTC (permalink / raw) To: Carlos O'Donell via Libc-alpha * 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-25 15:30 ` Florian Weimer @ 2020-05-25 21:17 ` Carlos O'Donell 2020-05-26 7:53 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2020-05-25 21:17 UTC (permalink / raw) To: Florian Weimer, Carlos O'Donell via Libc-alpha 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? -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-25 21:17 ` Carlos O'Donell @ 2020-05-26 7:53 ` Florian Weimer 2020-05-26 8:16 ` Andreas Schwab 2020-06-04 18:35 ` Carlos O'Donell 0 siblings, 2 replies; 15+ messages in thread From: Florian Weimer @ 2020-05-26 7:53 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Carlos O'Donell via Libc-alpha * 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 7:53 ` Florian Weimer @ 2020-05-26 8:16 ` Andreas Schwab 2020-05-26 8:39 ` Florian Weimer 2020-06-04 18:35 ` Carlos O'Donell 1 sibling, 1 reply; 15+ messages in thread From: Andreas Schwab @ 2020-05-26 8:16 UTC (permalink / raw) To: Florian Weimer via Libc-alpha; +Cc: Carlos O'Donell, Florian Weimer 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, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 8:16 ` Andreas Schwab @ 2020-05-26 8:39 ` Florian Weimer 2020-05-26 8:53 ` Andreas Schwab 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2020-05-26 8:39 UTC (permalink / raw) To: Andreas Schwab; +Cc: Florian Weimer via Libc-alpha, Carlos O'Donell * 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 8:39 ` Florian Weimer @ 2020-05-26 8:53 ` Andreas Schwab 2020-05-26 9:31 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Andreas Schwab @ 2020-05-26 8:53 UTC (permalink / raw) To: Florian Weimer; +Cc: Florian Weimer via Libc-alpha, Carlos O'Donell 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, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 8:53 ` Andreas Schwab @ 2020-05-26 9:31 ` Florian Weimer 2020-05-26 9:37 ` Andreas Schwab 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2020-05-26 9:31 UTC (permalink / raw) To: Andreas Schwab; +Cc: Florian Weimer via Libc-alpha, Carlos O'Donell * 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 9:31 ` Florian Weimer @ 2020-05-26 9:37 ` Andreas Schwab 2020-05-26 12:31 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Andreas Schwab @ 2020-05-26 9:37 UTC (permalink / raw) To: Florian Weimer; +Cc: Florian Weimer via Libc-alpha, Carlos O'Donell 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, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 9:37 ` Andreas Schwab @ 2020-05-26 12:31 ` Florian Weimer 2020-05-26 12:57 ` Andreas Schwab 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2020-05-26 12:31 UTC (permalink / raw) To: Andreas Schwab; +Cc: Florian Weimer via Libc-alpha, Carlos O'Donell * 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 12:31 ` Florian Weimer @ 2020-05-26 12:57 ` Andreas Schwab 2020-05-26 13:15 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Andreas Schwab @ 2020-05-26 12:57 UTC (permalink / raw) To: Florian Weimer; +Cc: Florian Weimer via Libc-alpha, Carlos O'Donell 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, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 12:57 ` Andreas Schwab @ 2020-05-26 13:15 ` Florian Weimer 2020-05-26 13:18 ` Andreas Schwab 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2020-05-26 13:15 UTC (permalink / raw) To: Andreas Schwab; +Cc: Florian Weimer via Libc-alpha, Carlos O'Donell * 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 13:15 ` Florian Weimer @ 2020-05-26 13:18 ` Andreas Schwab 2020-05-26 13:23 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Andreas Schwab @ 2020-05-26 13:18 UTC (permalink / raw) To: Florian Weimer; +Cc: Florian Weimer via Libc-alpha, Carlos O'Donell 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, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 13:18 ` Andreas Schwab @ 2020-05-26 13:23 ` Florian Weimer 0 siblings, 0 replies; 15+ messages in thread From: Florian Weimer @ 2020-05-26 13:23 UTC (permalink / raw) To: Andreas Schwab; +Cc: Florian Weimer via Libc-alpha, Carlos O'Donell * 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add comments to explain when a stream is freed by, __libc_freeres(). 2020-05-26 7:53 ` Florian Weimer 2020-05-26 8:16 ` Andreas Schwab @ 2020-06-04 18:35 ` Carlos O'Donell 1 sibling, 0 replies; 15+ messages in thread From: Carlos O'Donell @ 2020-06-04 18:35 UTC (permalink / raw) To: Florian Weimer; +Cc: Carlos O'Donell via Libc-alpha 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. -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-06-04 18:36 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-22 20:37 [PATCH] Add comments to explain when a stream is freed by, __libc_freeres() Carlos O'Donell 2020-05-25 15:30 ` Florian Weimer 2020-05-25 21:17 ` Carlos O'Donell 2020-05-26 7:53 ` Florian Weimer 2020-05-26 8:16 ` Andreas Schwab 2020-05-26 8:39 ` Florian Weimer 2020-05-26 8:53 ` Andreas Schwab 2020-05-26 9:31 ` Florian Weimer 2020-05-26 9:37 ` Andreas Schwab 2020-05-26 12:31 ` Florian Weimer 2020-05-26 12:57 ` Andreas Schwab 2020-05-26 13:15 ` Florian Weimer 2020-05-26 13:18 ` Andreas Schwab 2020-05-26 13:23 ` Florian Weimer 2020-06-04 18:35 ` Carlos O'Donell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).