[patch] wordexp resource cleanup (was: Re: wordexp bug)

[Peter Rosin <peda at lysator dot liu dot se>]

On 2012-08-17 22:03, Peter Rosin wrote:
> On 2012-08-17 21:49, Peter Rosin wrote:
>> On 2012-08-17 15:35, Yaakov (Cygwin/X) wrote:
>>> AFAICS wordexp(3) is completely broken, returning WRDE_SYNTAX no
>>> matter what I supply as the string.  STC attached.
>>
>> I suspect you are using Cygwin, and if so I suspect that we have been
>> bitten by this change in bash-4.1-rc:
>>
>> c.  The (undocumented) --wordexp option is no longer included by default.
>>
>> The documentation states that you have to enable the WORDEXP_OPTION
>> when building bash, but I'll leave the details of how to do that to Eric,
>> the Cygwin bash maintainer.
> 
> Maybe that was a bit too terse, the facts are that newlib forks
> "bash --wordexp" when wordexp(3) is called. Newer Cygwin bashes doesn't
> support the undocumented --wordexp option, presumably since bash no
> longer includes that option by default.  I found this out by looking
> up the documentation for the undocumented option in bash (i.e. the
> source).
> 
> I should perhaps also mention, for new Cygwin recipients, that the
> original message appeared on the newlib list, which is why I could
> only suspect that Yaakov was using Cygwin, but that guess seemed
> kind of safe...

While looking up how wordexp worked I noticed that the resource handling
in the implementation was "somewhat lacking". So here's a follow-up to
take care of a few corner cases. (Not even compile-tested as I don't
know how to set that up without reading up on the subject, but you
wouldn't trust a newcomer anyway, right?)

Cheers,
Peter

	* libc/posix/wordexp.c (wordexp): Free all file descriptors
	and make sure the child is reaped in case of failure. Remove
	dead code while at it.

Index: libc/posix/wordexp.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/posix/wordexp.c,v
retrieving revision 1.2
diff -u -r1.2 wordexp.c
--- libc/posix/wordexp.c	8 Aug 2012 11:04:17 -0000	1.2
+++ libc/posix/wordexp.c	19 Aug 2012 21:44:40 -0000
@@ -66,11 +66,26 @@
         pwordexp->we_wordv[i] = NULL;
     }
 
-  pipe(fd);
-  pipe(fd_err);
+  if (pipe(fd))
+    return WRDE_NOSPACE;
+  if (pipe(fd_err))
+    {
+      close(fd[0]);
+      close(fd[1]);
+      return WRDE_NOSPACE;
+    }
   pid = fork();
 
-  if (pid > 0)
+  if (pid == -1)
+    {
+      /* In "parent" process, but fork failed */
+      close(fd_err[0]);
+      close(fd_err[1]);
+      close(fd[0]);
+      close(fd[1]);
+      return WRDE_NOSPACE;
+    }
+  else if (pid > 0)
     {
       /* In parent process. */
 
@@ -104,7 +119,7 @@
                 fprintf(stderr, tmp);
             }
 
-          return err;
+          goto cleanup;
         }
 
       /* f is the standard output from the shell command. */
@@ -120,14 +135,14 @@
 
       if(!(pwordexp->we_wordv = (char **)realloc(pwordexp->we_wordv,
                                                  (pwordexp->we_wordc + num_words + offs + 1) * sizeof(char *))))
-        return WRDE_NOSPACE;
+        {
+          err = WRDE_NOSPACE;
+          goto cleanup;
+        }
 
-      /* Get number of bytes required for storage of num_words words. */
+      /* Read past number of bytes required for storage of num_words words. */
       fgets(tmp, MAXLINELEN, f);
 
-      if((iter = strchr(tmp, '\n')))
-          *iter = '\0';
-
       /* Get each expansion from the shell output, and store each in
          pwordexp's we_wordv vector. */
       for(i = 0; i < num_words; i++)
@@ -142,14 +157,16 @@
 
       pwordexp->we_wordv[pwordexp->we_wordc + offs + i] = NULL;
       pwordexp->we_wordc += num_words;
+      err = WRDE_SUCCESS;
 
+cleanup:
       close(fd[0]);
       close(fd_err[0]);
 
       /* Wait for child to finish. */
       waitpid (pid, NULL, 0);
 
-      return WRDE_SUCCESS;
+      return err;
     }
   else
     {