[PATCH] Fix stdio memory leaks with _REENT_SMALL + _LITE_EXIT

[PATCH] Fix stdio memory leaks with _REENT_SMALL + _LITE_EXIT

[Volodymyr Medvid]

When a thread calls stdio functions (say, printf) and then dies,
_reclaim_reent() runs cleanup_stdio() to free the file buffers
and descriptors created for this thread. This is causing multiple
memory leaks when newlib is configured with _REENT_SMALL
and _LITE_EXIT - this is the standard configuration for
newlib-nano provided with GNU Arm Embedded Toolchain.

1. While __sfp() allocates the FILE objects in GLOBAL_REENT glue chain,
   stdio_cleanup walks through the thread-specific glue chain
   to run the cleanup_func. Therefore, the FILE objects are never
   freed. This leaks ~428 bytes per thread (glue_with_file + 3 x FILE).

   To fix this, update __sfp() to use the per-thread glue chain for
   stdio descriptors.

2. With _LITE_EXIT enabled, _fflush_r is used as cleanup_func
   instead of _fclose_r - as a result, the I/O buffer memory allocated
   by __smakebuf_r is never freed - this leaks another 1024 bytes.
   To fix this, update cleanup_stdio to always use _fclose_r.

This is a follow-up patch for https://ecos.sourceware.org/ml/newlib/current/017697.html
---
 newlib/libc/stdio/findfp.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c
index 1370b63b8..2799980f3 100644
--- a/newlib/libc/stdio/findfp.c
+++ b/newlib/libc/stdio/findfp.c
@@ -153,7 +153,7 @@ __sfp (struct _reent *d)
 
   if (_GLOBAL_REENT->__cleanup == NULL)
     __sinit (_GLOBAL_REENT);
-  for (g = &_GLOBAL_REENT->__sglue;; g = g->_next)
+  for (g = &d->__sglue;; g = g->_next)
     {
       for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++)
 	if (fp->_flags == 0)
@@ -209,14 +209,9 @@ cleanup_stdio (struct _reent *ptr)
      the aforementioned systems. */
   cleanup_func = __sflushw_r;
 #else
-  /* Otherwise close files and flush read streams, too.
-     Note we call flush directly if "--enable-lite-exit" is in effect.  */
-#ifdef _LITE_EXIT
-  cleanup_func = _fflush_r;
-#else
+  /* Otherwise close files and flush read streams, too. */
   cleanup_func = _fclose_r;
 #endif
-#endif
 #ifdef _REENT_GLOBAL_STDIO_STREAMS
   if (ptr->_stdin != &__sf[0])
     (*cleanup_func) (ptr, ptr->_stdin);
-- 
2.25.1

Re: [PATCH] Fix stdio memory leaks with _REENT_SMALL + _LITE_EXIT

[Sebastian Huber]

On 12/04/2022 15:23, Volodymyr Medvid wrote:
> When a thread calls stdio functions (say, printf) and then dies,
> _reclaim_reent() runs cleanup_stdio() to free the file buffers
> and descriptors created for this thread. This is causing multiple
> memory leaks when newlib is configured with _REENT_SMALL
> and _LITE_EXIT - this is the standard configuration for
> newlib-nano provided with GNU Arm Embedded Toolchain.
> 
> 1. While __sfp() allocates the FILE objects in GLOBAL_REENT glue chain,
>     stdio_cleanup walks through the thread-specific glue chain
>     to run the cleanup_func. Therefore, the FILE objects are never
>     freed. This leaks ~428 bytes per thread (glue_with_file + 3 x FILE).

Yes, this is the Newlib design. FILE objects are never freed. If you 
close a FILE object, then it is basically placed on a free list and 
recycled once another FILE object is opened.

> 
>     To fix this, update __sfp() to use the per-thread glue chain for
>     stdio descriptors.

This is definitely not the right way to address this issue.

Why don't you use the _REENT_GLOBAL_STDIO_STREAMS Newlib configuration 
option?

> 
> 2. With _LITE_EXIT enabled, _fflush_r is used as cleanup_func
>     instead of _fclose_r - as a result, the I/O buffer memory allocated
>     by __smakebuf_r is never freed - this leaks another 1024 bytes.
>     To fix this, update cleanup_stdio to always use _fclose_r.
> 
> This is a follow-up patch forhttps://ecos.sourceware.org/ml/newlib/current/017697.html
> ---
>   newlib/libc/stdio/findfp.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c
> index 1370b63b8..2799980f3 100644
> --- a/newlib/libc/stdio/findfp.c
> +++ b/newlib/libc/stdio/findfp.c
> @@ -153,7 +153,7 @@ __sfp (struct _reent *d)
>   
>     if (_GLOBAL_REENT->__cleanup == NULL)
>       __sinit (_GLOBAL_REENT);
> -  for (g = &_GLOBAL_REENT->__sglue;; g = g->_next)
> +  for (g = &d->__sglue;; g = g->_next)
>       {
>         for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++)
>   	if (fp->_flags == 0)

This patch would break at least RTEMS.

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH] Fix stdio memory leaks with _REENT_SMALL + _LITE_EXIT

[Volodymyr Medvid]

On Tue, 12 Apr 2022 18:10:38 +0200
Sebastian Huber <sebastian.huber@embedded-brains.de> wrote:

> This is definitely not the right way to address this issue.
>
> Why don't you use the _REENT_GLOBAL_STDIO_STREAMS Newlib
> configuration option?

Thanks for the suggestion - I was able to verify the memory leak goes away once
newlib-nano is reconfigured with "--enable-newlib-global-stdio-streams".
I also see you originally submitted commit
668a4c8722090fffd10869dbb15b879651c1370d
that was later extended to also apply to _REENT_SMALL
(b7520b14d5fe175d9bc60266700fb7b988600a84).
I filed a ticket https://bugs.linaro.org/show_bug.cgi?id=5841 to
enable global stdio
streams for standard newlib-nano provided with GNU Arm Embedded
toolchain binary package.

> This patch would break at least RTEMS.
>

Sorry, I have no way to validate this on any platform except Arm
Cortex-M + FreeRTOS.
Also, the current test suite is not capable of catching issues with
stdio reentrancy.
I understand there are lot of platform/OS-specific stdio
implementations, some of them
may rely on configure flags like "--enable-newlib-reent-small" or
"--enable-lite-exit"
to meet the memory and performance constraints. Unfortunately, it is
not clear from
the commit e7565f10886bac86410db6eb6fda47da1d04ac9b why _fclose_r was changed to
_fflush_r when _LITE_EXIT is enabled - is this to reduce memory footprint?

Thanks,
Volodymyr