os-win32.c : fix memory leak

string is allocated by g_malloc, will not be used after putenv, should be
free before return.

Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
---
 os-win32.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

 }

If you free the string, it will cause the environment variable unavailable.
 More details please see the following text extracted from manual of
"putenv":

       The  libc4  and  libc5  and  glibc 2.1.2 versions conform to SUSv2:
the pointer string given to putenv() is used.  In particular, this
       string becomes part of the environment; changing it later will
change the environment.  (Thus, it is an error is to call putenv() with
       an automatic variable as the argument, then return from the calling
function while string is still part of the environment.)  However,
       glibc 2.0-2.1.1 differs: a copy of the string is used.  On the one
hand this causes a memory leak, and on the other hand  it  violates
       SUSv2.  This has been fixed in glibc 2.1.2.


2011/11/24 Zhi Hui Li <zhihuili@linux.vnet.ibm.com>

> string is allocated by g_malloc, will not be used after putenv, should be
> free before return.
>
> Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
> ---
>  os-win32.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/os-win32.c b/os-win32.c
> index 8ad5fa1..e6e9143 100644
> --- a/os-win32.c
> +++ b/os-win32.c
> @@ -44,6 +44,7 @@ int setenv(const char *name, const char *value, int
> overwrite)
>          char *string = g_malloc(length);
>          snprintf(string, length, "%s=%s", name, value);
>          result = putenv(string);
> +        g_free(string);
>      }
>      return result;
>  }
> --
> 1.7.4.1
>
>
>

On Thu, Nov 24, 2011 at 05:15:30PM +0800, Mark wrote:
> If you free the string, it will cause the environment variable unavailable.
>  More details please see the following text extracted from manual of
> "putenv":
> 
>        The  libc4  and  libc5  and  glibc 2.1.2 versions conform to SUSv2:
> the pointer string given to putenv() is used.  In particular, this
>        string becomes part of the environment; changing it later will
> change the environment.  (Thus, it is an error is to call putenv() with
>        an automatic variable as the argument, then return from the calling
> function while string is still part of the environment.)  However,
>        glibc 2.0-2.1.1 differs: a copy of the string is used.  On the one
> hand this causes a memory leak, and on the other hand  it  violates
>        SUSv2.  This has been fixed in glibc 2.1.2.

I don't think this matters since os-win32.c is only built for mingw,
which uses the Microsoft C runtime and not glibc.

However, there is no documentation for putenv(3) on MSDN because the
function has been deprecated :(.  So I think the safest thing to do is
to assume this will leak memory but we are not allowed to free the
string.

Either you could investigate the new _putenv(3) and test the Windows
build to make sure it works.  Or you could send a patch that adds a
comment explaining why there is a memory leak here.

Stefan

Am 24.11.2011 11:27, schrieb Stefan Hajnoczi:
> On Thu, Nov 24, 2011 at 05:15:30PM +0800, Mark wrote:
>    
>> If you free the string, it will cause the environment variable unavailable.
>>   More details please see the following text extracted from manual of
>> "putenv":
>>
>>         The  libc4  and  libc5  and  glibc 2.1.2 versions conform to SUSv2:
>> the pointer string given to putenv() is used.  In particular, this
>>         string becomes part of the environment; changing it later will
>> change the environment.  (Thus, it is an error is to call putenv() with
>>         an automatic variable as the argument, then return from the calling
>> function while string is still part of the environment.)  However,
>>         glibc 2.0-2.1.1 differs: a copy of the string is used.  On the one
>> hand this causes a memory leak, and on the other hand  it  violates
>>         SUSv2.  This has been fixed in glibc 2.1.2.
>>      
> I don't think this matters since os-win32.c is only built for mingw,
> which uses the Microsoft C runtime and not glibc.
>
> However, there is no documentation for putenv(3) on MSDN because the
> function has been deprecated :(.  So I think the safest thing to do is
> to assume this will leak memory but we are not allowed to free the
> string.
>    

MS claims that putenv is a POSIX function, so I also expected
that free / f_free is not allowed.

I now wrote a short test which indicates that g_free would work:
getenv returns a pointer which is completely different from
the one passed to putenv.

Nevertheless, there is a better solution using _putenv_s.
I'll send a patch.

Regards,
Stefan Weil

Am 24.11.2011 20:28, schrieb Stefan Weil:
> Am 24.11.2011 11:27, schrieb Stefan Hajnoczi:
>> On Thu, Nov 24, 2011 at 05:15:30PM +0800, Mark wrote:
>>> If you free the string, it will cause the environment variable 
>>> unavailable.
>>>   More details please see the following text extracted from manual of
>>> "putenv":
>>>
>>>         The  libc4  and  libc5  and  glibc 2.1.2 versions conform to 
>>> SUSv2:
>>> the pointer string given to putenv() is used.  In particular, this
>>>         string becomes part of the environment; changing it later will
>>> change the environment.  (Thus, it is an error is to call putenv() with
>>>         an automatic variable as the argument, then return from the 
>>> calling
>>> function while string is still part of the environment.)  However,
>>>         glibc 2.0-2.1.1 differs: a copy of the string is used.  On 
>>> the one
>>> hand this causes a memory leak, and on the other hand  it  violates
>>>         SUSv2.  This has been fixed in glibc 2.1.2.
>> I don't think this matters since os-win32.c is only built for mingw,
>> which uses the Microsoft C runtime and not glibc.
>>
>> However, there is no documentation for putenv(3) on MSDN because the
>> function has been deprecated :(.  So I think the safest thing to do is
>> to assume this will leak memory but we are not allowed to free the
>> string.
>
> MS claims that putenv is a POSIX function, so I also expected
> that free / f_free is not allowed.
>
> I now wrote a short test which indicates that g_free would work:
> getenv returns a pointer which is completely different from
> the one passed to putenv.
>
> Nevertheless, there is a better solution using _putenv_s.
> I'll send a patch.
>
> Regards,
> Stefan Weil
>

Hi Stefan,

I'm afraid I was too fast when I promised a patch with _putenv_s.
Function _putenv_s is a good solution, but only if it is supported
by MinGW. Older versions of MinGW (Debian Squeeze!) don't support
it :-(

Therefore I suggest to apply this patch. I hope that my test which was
run on XP (32 bit) is sufficient.

Cheers,
Stefan W.

Tested-by: Stefan Weil <sw@weilnetz.de>

On 11/24/2011 08:28 PM, Stefan Weil wrote:
>
> MS claims that putenv is a POSIX function, so I also expected
> that free / f_free is not allowed.
>
> I now wrote a short test which indicates that g_free would work:
> getenv returns a pointer which is completely different from
> the one passed to putenv.

Confirmed by http://source.winehq.org/source/dlls/msvcrt/environ.c.  It 
makes a copy of the string, passes it to SetEnvironmentVariable, and 
frees the copy.  So Windows never even sees the string passed to putenv.

The reason for the dance is that: 1) the underlying Win32 APIs require 
separate arguments for the variable and value; 2) even though in the end 
Wine stores the environment as name=value 
(http://source.winehq.org/source/dlls/ntdll/env.c), it does so in a 
single consecutive block of memory, not as a char* array like POSIX 
does.  While (2) might apply only to Wine, (1) surely applies to Windows 
as well.

Stefan, can you add some of the info to the commit message?

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

On Thu, Nov 24, 2011 at 04:27:52PM +0800, Zhi Hui Li wrote:
> string is allocated by g_malloc, will not be used after putenv, should be
> free before return.
> 
> Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
> ---
>  os-win32.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)

Thanks, applied to the trivial patches -next tree:
http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches-next

Stefan