Re: Fix for memory leak in setenv/unsetenv

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