Date: Thu, 19 Oct 2006 11:54:37 -0500 (CDT) From: "Sean C. Farley" <sean-freebsd@farley.org> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-current@freebsd.org Subject: Re: Fix for memory leak in setenv/unsetenv Message-ID: <20061019112601.J91957@thor.farley.org> In-Reply-To: <200610191032.29232.jhb@freebsd.org> References: <20061006200320.T1063@baba.farley.org> <200610111427.42195.jhb@freebsd.org> <20061018211005.L1466@baba.farley.org> <200610191032.29232.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Sean
--
sean-freebsd@farley.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061019112601.J91957>