Date: Thu, 19 Oct 2006 13:10:04 -0400 From: John Baldwin <jhb@freebsd.org> To: "Sean C. Farley" <sean-freebsd@farley.org> Cc: freebsd-current@freebsd.org Subject: Re: Fix for memory leak in setenv/unsetenv Message-ID: <200610191310.05604.jhb@freebsd.org> In-Reply-To: <20061019112601.J91957@thor.farley.org> References: <20061006200320.T1063@baba.farley.org> <200610191032.29232.jhb@freebsd.org> <20061019112601.J91957@thor.farley.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday 19 October 2006 12:54, Sean C. Farley wrote:
> On Thu, 19 Oct 2006, John Baldwin wrote:
>
> > On Wednesday 18 October 2006 22:50, Sean C. Farley wrote:
> >> On Wed, 11 Oct 2006, John Baldwin wrote:
> >>> On Wednesday 11 October 2006 12:15, Sean C. Farley wrote:
> >>>> On Tue, 10 Oct 2006, John Baldwin wrote:
> >>>>> This still won't work. The reason for the intentional leak is
> >>>>> because of this code sequence:
> >>>>>
> >>>>> char *a;
> >>>>>
> >>>>> setenv("FOO", "0", 1);
> >>>>> a = getenv("FOO");
> >>>>> setenv("FOO", "bar", 1);
> >>>>> printf("FOO was %s\n", a);
> >>>>>
> >>>>> With the memory leak fixed this will use free'd memory. While
> >>>>> this code may seem weird in a program, it actually is quite
> >>>>> possible for a library to read and cache the value of an
> >>>>> environment variable. If you didn't leave the leak around, the
> >>>>> library could cause a crash if the main program (or another
> >>>>> library) changed the environment variable the first library had a
> >>>>> cached pointer to the value of.
> >>
> >> <snip>
> >>
> >>> Yeah, but it doesn't crash is the point actually. The pointer is
> >>> still valid, though it may be overwritten with a newer value, it's
> >>> still valid and a library can reliably doing getenv() and that
> >>> pointer will always point to some value of that variable, but it
> >>> won't ever point to anything else.
> >>
> >> <snip>
> >>
> >>> Part of the problem is that we have no way to notify consumers of an
> >>> environment variable when its value is changed. Alternatively, we
> >>> could add a different variant of getenv that required the user to
> >>> supply the buffer, but that's not the API we have.
> >>
> >> OK. I decided to fix the memory leak as well as keep backward
> >> compatibility. The result is on my site tar'd[1] and extracted[2].
> >> It still needs some touch-ups, but it works. It is even faster than
> >> the current implementation when I compared "hungry" and "lean"
> >> (main.c without the sleep() call).
> >
> > I don't see how you fixed the leak. You explicitly mention that you
> > don't free old values, so you are preserving the leak.
>
> I preserve the leak, but I also make use of old entries with matching
> names if their size is big enough. The maximum size of the value is
> recorded at allocation. The code in the source tree uses the strlen()
> of the current value which can shrink even if the space is available.
>
> Example:
> setenv("FOO", "BAR1", 1);
> w = getenv("FOO");
> setenv("FOO", "BARBAR1", 1);
> x = getenv("FOO");
> setenv("FOO", "BAR2", 1);
> y = getenv("FOO");
> setenv("FOO", "BARBAR2", 1);
> z = getenv("FOO");
>
> This will end up with w == y ("BAR2") and y == z ("BAR1"). w was
> allocated first in the array of variables. x is then allocated since w
> is too small. y finds w within the array and reuses it. z does the
> wame with x. If the larger value had been allocated first, then all
> setenv() calls would have used the same storage space. Yes, there is a
> leak, but flipping back and forth does not cause the leak to keep
> growing. Also, there would be no need to space-pad a value to prevent
> the leak.
Ah, very cool.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200610191310.05604.jhb>