This is the mail archive of the
newlib@sourceware.org
mailing list for the newlib project.
Re: [patch 1/4] wordexp: dont leak file streams
On 2012-10-08 13:42, Corinna Vinschen wrote:
> Hi Peter,
>
> On Sep 14 01:04, Peter Rosin wrote:
>> 2012-09-13 Peter Rosin <peda@lysator.liu.se>
>>
>> * libc/posix/wordexp.c (wordexp): Don't leak file streams.
>>
>> Index: newlib/libc/posix/wordexp.c
>> ===================================================================
>> --- newlib.orig/libc/posix/wordexp.c
>> +++ newlib/libc/posix/wordexp.c
>> @@ -29,8 +29,8 @@
>> int
>> wordexp(const char *words, wordexp_t *pwordexp, int flags)
>> {
>> - FILE *f;
>> - FILE *f_err;
>> + FILE *f = NULL;
>> + FILE *f_err = NULL;
>> char tmp[MAXLINELEN];
>> int i = 0;
>> int offs = 0;
>> @@ -143,8 +143,14 @@ wordexp(const char *words, wordexp_t *pw
>> pwordexp->we_wordv[pwordexp->we_wordc + offs + i] = NULL;
>> pwordexp->we_wordc += num_words;
>>
>> - close(fd[0]);
>> - close(fd_err[0]);
>> + if (f)
>> + fclose(f);
>> + else
>> + close(fd[0]);
>> + if (f_err)
>> + fclose(f_err);
>> + else
>> + close(fd_err[0]);
>
> Thanks for the patch. Is the `if (f)' and `if (f_err)' really
> necessary, though? The code path which ultimately leads to these close
> calls always calls `f_err = fdopen(...)' and `f = fdopen(...)'. AFAICS
> you can always simply call fclose...
Yes, that is correct. Sorry for not doing a better split, but in 3/4 I
add a cleanup: label just above that "block", an then it makes sense.
It didn't seem that important at the time...
> Sorry for being a bit late due to sickness and vacation, but here's a
That's ok!
> more generic question: Shall we really stick to depending on an
> undocumented, more or less deprecated feature of bash?
Probably not, but I didn't fancy implementing it from scratch and...
> Wouldn't it make more sense to kill the current wordexp implementation
> and pull in code derived from one of the BSDs? I had a quick look into
> the FreeBSD implementation and it seems it wouldn't be a lot of work to
> adapt it, if somebody feels up to the task.
...when I googled that the last time I only found implementations that
depended on the wordexp builtin in their /bin/sh, which didn't look like
an improvement to me. And then there's the one in glibc of course...
Where should I look?
Cheers,
Peter