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 Oct 8 17:00, Peter Rosin wrote:
> 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...
Quite understandable :)
> > 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...
Oh, you're right. That's kind of the same just in another wrap. Too bad.
> Where should I look?
Glibc is not the best choice for newlib, due to licensing. Looks like
we have to stick to our implementation for the time being.
Do you want to submit a tweaked patch? Hmm, this might affect applying
your other patches as well. If you want, you can simply send a single
combined patch again. I had a look into the other patches and they look
good to me.
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat