[Toybox] setenv memory leak.
I'm inching towards reopening the toysh can of worms, and last time I opened it
there was a problem with environment variable memory leaking in libc:
https://landley.net/notes-2006.html#24-10-2006
Over the years I griped about this to Rich Felker, who fixed it in musl:
http://git.musl-libc.org/cgit/musl/tree/src/env/setenv.c
But bionic still leaks each entry when you replace it:
https://android.googlesource.com/platform/bionic/+/refs/heads/master/libc/upstream-openbsd/lib/libc/stdlib/setenv.c
Meaning if you do:
for (i = 0; i<9; i++) setenv("this", "that", 1);
It'll leak 9 gigabytes.
This seems suboptimal, and is bad for long-running shell scripts.
I'm also trying to redo the tar "command mode" (does anyone _use_ this? It was
submitted so I _assume_ so but I've never encountered it in the wild) to use
nommu-friendly xpopen_both() and the _easy_ way is to just set the environment
variables in the host before the vfork() but that's resetting the same variables
an aribtrary number of times and if libc can't free the variable memory...
Rob
P.S. I can do it myself with putenv() or my own direct environ() manipulation
but that's not my first choice? Nor is rewriting xpopen() to have a callback
doing arbitrary stuff in the vfork context before it execs, kinda defeats the
point of that function...
P.P.S. I honestly don't care if glibc gets this right or I'd have poked them
about it sometime over the past 15 years. If they can't gnu/do the basic "wait,
how does this work and what happens if" stuff a civilian like me does in passing
that's _their_ gnu/problem.
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] setenv memory leak.
On 4/8/19 1:42 PM, Rob Landley wrote: > I'm inching towards reopening the toysh can of worms, and last time I opened > it > there was a problem with environment variable memory leaking in libc: > > https://landley.net/notes-2006.html#24-10-2006 Nevermind, creating a lib/env.c to do this stuff properly, with the usual xsetenv() and friends. This isn't hard, and I'm not gonna guess what libc du jour gets right... (The design I had way back when, with an initial envc count that decrements every time we drop something out of our initial environment, and everything else is our allocation we can free again, seems quite reasonable. I should leave /proc/self/environment alone the same way I leave /proc/self/cmdline alone, it's what we inherited at exec() time not what we're using now.) Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] setenv memory leak.
why does toysh care? shells i've seen don't call setenv at all ---
they maintain an "environ" of their own for their children.
On Mon, Apr 8, 2019 at 11:42 AM Rob Landley wrote:
>
> I'm inching towards reopening the toysh can of worms, and last time I opened
> it
> there was a problem with environment variable memory leaking in libc:
>
> https://landley.net/notes-2006.html#24-10-2006
>
> Over the years I griped about this to Rich Felker, who fixed it in musl:
>
> http://git.musl-libc.org/cgit/musl/tree/src/env/setenv.c
>
> But bionic still leaks each entry when you replace it:
>
>
> https://android.googlesource.com/platform/bionic/+/refs/heads/master/libc/upstream-openbsd/lib/libc/stdlib/setenv.c
>
> Meaning if you do:
>
> for (i = 0; i<9; i++) setenv("this", "that", 1);
>
> It'll leak 9 gigabytes.
>
> This seems suboptimal, and is bad for long-running shell scripts.
>
> I'm also trying to redo the tar "command mode" (does anyone _use_ this? It was
> submitted so I _assume_ so but I've never encountered it in the wild) to use
> nommu-friendly xpopen_both() and the _easy_ way is to just set the environment
> variables in the host before the vfork() but that's resetting the same
> variables
> an aribtrary number of times and if libc can't free the variable memory...
>
> Rob
>
> P.S. I can do it myself with putenv() or my own direct environ() manipulation
> but that's not my first choice? Nor is rewriting xpopen() to have a callback
> doing arbitrary stuff in the vfork context before it execs, kinda defeats the
> point of that function...
>
> P.P.S. I honestly don't care if glibc gets this right or I'd have poked them
> about it sometime over the past 15 years. If they can't gnu/do the basic
> "wait,
> how does this work and what happens if" stuff a civilian like me does in
> passing
> that's _their_ gnu/problem.
> ___
> Toybox mailing list
> Toybox@lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] setenv memory leak.
On 4/8/19 2:31 PM, enh wrote: > why does toysh care? shells i've seen don't call setenv at all --- > they maintain an "environ" of their own for their children. Yes, they have to do it themselves because libc is doing it unusably wrong for anything that persists and changes environment variables repeatedly. *shrug* Reinventing this wheel isn't hard, I'd just hoped I wouldn't have to. But I didn't complain widely enough long ago, and only musl fixed the leak. Rob P.S. I tried to make ftw() work before writing dirtree, I tried to make realpath() work before writing xrealpath(), I tried pretty hard to avoid writing next_printf() at all... ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] setenv memory leak.
On Mon, Apr 8, 2019 at 1:12 PM Rob Landley wrote: > Yes, they have to do it themselves because libc is doing it unusably wrong for > anything that persists and changes environment variables repeatedly. > > *shrug* Reinventing this wheel isn't hard, I'd just hoped I wouldn't have to. > But I didn't complain widely enough long ago, and only musl fixed the leak. Shells need to keep track of variables separately from the environment to handle non-exported variables anyway. I suppose you could keep everything in the environment and keep a list of non-exported variables as well, but that just seems like more work for no reason. Making setenv not leak seems impossible in the presence of threaded code, anyway: you have no way of knowing whether the return value of getenv just got freed because some other thread came in and called setenv, so the only solution is basically to just never call setenv (but that fixes your leak, too!). ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
