[pulseaudio-discuss] [PATCH 09/10] Implement some functions for win32

[Colin Guthrie]

'Twas brillig, and Maarten Bosmans at 11/01/11 06:16 did gyre and gimble:
> Thanks for the review
> 
> 2011/1/11 Colin Guthrie <gmane at colin.guthr.ie>:
>> 'Twas brillig, and Maarten Bosmans at 06/01/11 01:39 did gyre and gimble:
>>> @@ -2565,7 +2569,11 @@ void pa_unset_env_recorded(void) {
>>>          if (!s)
>>>              break;
>>>
>>> +#ifdef OS_IS_WIN32
>>> +        putenv(pa_sprintf_malloc("%s=", s));
>>> +#else
>>>          unsetenv(s);
>>> +#endif
>>>          pa_xfree(s);
>>>      }
>>>  }
>>
>>
>> This looks leaky....
> 
> Yes, I thought so to, but I'm not really that proficient in C (getting
> there though). The pa_sprintf_malloc I just copied from the pa_set_env
> definition, a couple of lines earlier in core-util.c:
> 
> 
> void pa_set_env(const char *key, const char *value) {
>     pa_assert(key);
>     pa_assert(value);
> 
>     /* This is not thread-safe */
> 
>     putenv(pa_sprintf_malloc("%s=%s", key, value));
> }

Hmm, that looks wrong too on first glance, but with a little digging
(well, man 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
 auto‐
       matic  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.


So I guess this was correct back in the olden days but perhaps now we
should be free'ing it. I'd rather have someone who knows such things
confirm this first tho' as I'm not really sure!!

> Is that wrong to? What fix would you suggest?

Well, if it turns out correct to do so:

char *t;
putenv(t = pa_sprintf_malloc("%s=", s));
pa_xfree(t);


But I will ask some folk about this as I'm not certain.

FWIW, further up in that file, there is some WIN32 code that uses
_putenv() with a stack variable. I'm not really sure of the differences
between putenv() and _putenv() tho' :s


Col

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]