This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] wordexp resource cleanup


On 2012-08-20 12:29, Peter Rosin wrote:
> On 2012-08-20 12:17, Peter Rosin wrote:
>> On 2012-08-20 00:00, Peter Rosin wrote:
>>> 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?)
>>>
>>> 	* 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.
>>>
>>
>> Whoops! There were more bugs in that function, so here's an update. This
>> one has actually been tested and works AFAICT.
>>
>> 	* libc/posix/wordexp.c (wordexp): Allocate enough storage for
>> 	all returned words. Handle expanded words longer than 500 bytes.
>> 	Return WRDE_NOSPACE on resource allocation failure, and cleanup
>> 	leftover resources for that case.
> 
> Whoops, that's a misleading ChangeLog entry. The first part of it
> referred to fixing a bug that I had introduced myself when sanitizing
> the realloc calls. So, a better ChangeLog would be:
> 
> 	* libc/posix/wordexp.c (wordexp): Handle expanded words longer
> 	than 500 bytes.	Return WRDE_NOSPACE on resource allocation
> 	failure. Cleanup leftover resources when failing.

Whoops again, more bugs. The following program leaks like hell (with a
self-built Cygwin bash supporting --wordexp, pending an update from Eric):

---------------8<------------------------------
#include <wordexp.h>

int
main (void)
{
  wordexp_t we;

  we.we_offs = 3000;
  while (1) {
    wordexp ("/usr/bin/*", &we, WRDE_DOOFFS);
    wordfree (&we);
  }
  return 0;
}
---------------8<------------------------------

So, here's an update fixing that too. Reading opengroup docs on wordexp
(http://pubs.opengroup.org/onlinepubs/009695399/functions/wordexp.html)
suggests that it should be ok to rely on the offset count in the given
wordexp_t, and I find nothing on that page suggesting that it is not
ok to store the non-use of an offset in the we_offs member. So, that's
what I did.

Cheers,
Peter

	* libc/posix/wordexp.c (wordexp): Handle expanded words longer
	than 500 bytes.	Don't leak file streams. Help wordfree step past
	we_offs entries when freeing. Return WRDE_NOSPACE on resource
	allocation failure. Cleanup leftover resources when failing.
	* libc/posix/wordfree.c (wordfree): Step past we_offs words
	before starting to free.

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	20 Aug 2012 11:11:33 -0000
@@ -29,17 +29,21 @@
 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;
   char *iter;
   pid_t pid;
   int num_words = 0;
+  int num_bytes = 0;
   int fd[2];
   int fd_err[2];
-  int err = 0;
+  int err = WRDE_NOSPACE;
+  char **wordv;
+  char *ewords = NULL;
+  char *eword;
 
   if (pwordexp == NULL)
     {
@@ -59,18 +63,37 @@
     {
       offs = pwordexp->we_offs;
 
-      if(!(pwordexp->we_wordv = (char **)realloc(pwordexp->we_wordv, (pwordexp->we_wordc + offs + 1) * sizeof(char *))))
-        return WRDE_NOSPACE;
+      wordv = (char **)realloc(pwordexp->we_wordv, (pwordexp->we_wordc + offs + 1) * sizeof(char *));
+      if (!wordv)
+        return err;
+      pwordexp->we_wordv = wordv;
 
       for (i = 0; i < offs; i++)
         pwordexp->we_wordv[i] = NULL;
     }
+  else
+    pwordexp->we_offs = 0;
 
-  pipe(fd);
-  pipe(fd_err);
+  if (pipe(fd))
+    return err;
+  if (pipe(fd_err))
+    {
+      close(fd[0]);
+      close(fd[1]);
+      return err;
+    }
   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 err;
+    }
+  else if (pid > 0)
     {
       /* In parent process. */
 
@@ -79,7 +102,8 @@
       close(fd_err[1]);
 
       /* f_err is the standard error from the shell command. */
-      f_err = fdopen(fd_err[0], "r");
+      if (!(f_err = fdopen(fd_err[0], "r")))
+        goto cleanup;
 
       /* Check for errors. */
       if (fgets(tmp, MAXLINELEN, f_err))
@@ -104,52 +128,81 @@
                 fprintf(stderr, tmp);
             }
 
-          return err;
+          goto cleanup;
         }
 
       /* f is the standard output from the shell command. */
-      f = fdopen(fd[0], "r");
+      if (!(f = fdopen(fd[0], "r")))
+        goto cleanup;
 
       /* Get number of words expanded by shell. */
-      fgets(tmp, MAXLINELEN, f);
+      if (!fgets(tmp, MAXLINELEN, f))
+        goto cleanup;
 
       if((iter = strchr(tmp, '\n')))
           *iter = '\0';
 
       num_words = atoi(tmp);
 
-      if(!(pwordexp->we_wordv = (char **)realloc(pwordexp->we_wordv,
-                                                 (pwordexp->we_wordc + num_words + offs + 1) * sizeof(char *))))
-        return WRDE_NOSPACE;
-
-      /* Get number of bytes required for storage of num_words words. */
-      fgets(tmp, MAXLINELEN, f);
+      wordv = (char **)realloc(pwordexp->we_wordv,
+                               (pwordexp->we_wordc + num_words + offs + 1) * sizeof(char *));
+      if (!wordv)
+        goto cleanup;
+      pwordexp->we_wordv = wordv;
+
+      /* Get number of bytes required for storage of all num_words words. */
+      if (!fgets(tmp, MAXLINELEN, f))
+        goto cleanup;
 
       if((iter = strchr(tmp, '\n')))
           *iter = '\0';
 
-      /* Get each expansion from the shell output, and store each in
-         pwordexp's we_wordv vector. */
+      num_bytes = atoi(tmp);
+
+      /* Get expansion from the shell output. */
+      if (!(ewords = (char *)malloc(num_bytes + num_words + 1)))
+        goto cleanup;
+      if (!fread(ewords, 1, num_bytes + num_words, f))
+        goto cleanup;
+      ewords[num_bytes + num_words] = 0;
+
+      /* Store each entry in pwordexp's we_wordv vector. */
+      eword = ewords;
       for(i = 0; i < num_words; i++)
         {
-          fgets(tmp, MAXLINELEN, f);
-
-          if((iter = strchr(tmp, '\n')))
+          if (eword && (iter = strchr(eword, '\n')))
             *iter = '\0';
-
-          pwordexp->we_wordv[pwordexp->we_wordc + offs + i] = strdup(tmp);
+          if (!eword ||
+              !(pwordexp->we_wordv[pwordexp->we_wordc + offs + i] = strdup(eword)))
+            {
+              pwordexp->we_wordv[pwordexp->we_wordc + offs + i] = NULL;
+              pwordexp->we_wordc += i;
+              goto cleanup;
+            }
+          eword = iter;
+          if (eword)
+            ++eword;
         }
 
       pwordexp->we_wordv[pwordexp->we_wordc + offs + i] = NULL;
       pwordexp->we_wordc += num_words;
+      err = WRDE_SUCCESS;
 
-      close(fd[0]);
-      close(fd_err[0]);
+cleanup:
+      free(ewords);
+      if (f)
+        fclose(f);
+      else
+        close(fd[0]);
+      if (f_err)
+        fclose(f_err);
+      else
+        close(fd_err[0]);
 
       /* Wait for child to finish. */
       waitpid (pid, NULL, 0);
 
-      return WRDE_SUCCESS;
+      return err;
     }
   else
     {
@@ -162,7 +215,8 @@
       /* Pipe standard output to parent process via fd. */
       if (fd[1] != STDOUT_FILENO)
         {
-          dup2(fd[1], STDOUT_FILENO);
+          if (dup2(fd[1], STDOUT_FILENO) == -1)
+            exit(1);
           /* fd[1] no longer required. */
           close(fd[1]);
         }
@@ -170,7 +224,8 @@
       /* Pipe standard error to parent process via fd_err. */
       if (fd_err[1] != STDERR_FILENO)
         {
-          dup2(fd_err[1], STDERR_FILENO);
+          if (dup2(fd_err[1], STDERR_FILENO) == -1)
+            exit(1);
           /* fd_err[1] no longer required. */
           close(fd_err[1]);
         }
@@ -179,6 +234,7 @@
         execl("/bin/bash", "bash", "--protected", "--wordexp", words, (char *)0);
       else
         execl("/bin/bash", "bash", "--wordexp", words, (char *)0);
+      exit(1);
     }
   return WRDE_SUCCESS;
 }
Index: libc/posix/wordfree.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/posix/wordfree.c,v
retrieving revision 1.1
diff -u -r1.1 wordfree.c
--- libc/posix/wordfree.c	31 Oct 2008 21:03:41 -0000	1.1
+++ libc/posix/wordfree.c	20 Aug 2012 11:11:33 -0000
@@ -33,7 +33,7 @@
     return;
 
   for(i = 0; i < pwordexp->we_wordc; i++)
-    free(pwordexp->we_wordv[i]);
+    free(pwordexp->we_wordv[pwordexp->we_offs + i]);
 
   free(pwordexp->we_wordv);
   pwordexp->we_wordv = NULL;

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]