Re: Fix for memory leak in setenv/unsetenv

On Tue, 10 Oct 2006, John Baldwin wrote:

> On Friday 06 October 2006 21:13, Sean C. Farley wrote:
>> Many a moon ago[1], I put together a patch to fix the leak in
>> setenv() and unsetenv().  A few months ago, I submitted a PR
>> (kern/99826[2]) for the final fix.  I was wondering if anyone would
>> take a look at it to see if any changes are still warranted.  The PR
>> contains information about the patch and sample programs to test it
>> out.
>>
>> Thank you.
>>
>> Sean
>>    1. http://lists.freebsd.org/pipermail/freebsd-hackers/2005-February/010463.html
>>    2. http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/99826
>
> 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.

Although it would not crash, the following would fail anyway:

  	setenv("FOO", "bar", 1);
  	a = getenv("FOO");
  	setenv("FOO", "0", 1);
  	printf("FOO was %s\n", a);

In this scenario, the printf() would print "0" since the second value
had a string length less than or equal to the previous value.  The
current implementation of setenv() would reuse the string instead of
malloc'ing a new one.

Also, this snippet from IEEE Std 1003.1, 2004 Edition regarding
getenv()[3]:

     The return value from getenv() may point to static data which may be
     overwritten by subsequent calls to getenv(), setenv(), or
     unsetenv().

After the call to the second setenv(), a portable application should not
assume that a still points to the same value.  Also, it says "may point
to static data" suggesting (at least to me) that the pointer may point
to dynamic memory and be freed following the call to setenv().

> I know for one app at my last job we had a problem with this with TZ,
> and so we explicitly space padded the timezone name out to a
> fixed-size each time to avoid the leak.

This is what I am trying to fix in setenv().  :)

Sean
   3. http://www.opengroup.org/onlinepubs/009695399/functions/getenv.html
-- 
sean-freebsd_at_farley.org