'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/]
