[patch 1/4] wordexp: dont leak file streams

[Corinna Vinschen]

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