This is the mail archive of the
newlib@sourceware.org
mailing list for the newlib project.
Re: [patch] wordexp resource cleanup
On 2012-09-06 23:22, Peter Rosin wrote:
> On 2012-08-26 23:52, Peter Rosin wrote:
>> On 2012-08-20 14:01, Peter Rosin wrote:
>>> 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?)
>>>>>
>>>>> Whoops! There were more bugs in that function, so here's an update. This
>>>>> one has actually been tested and works AFAICT.
>>>>
>>>> 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:
>>>
>>> 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.
>>
>> Whoops again. It was wishful thinking that made me conclude that we_offs
>> could be reused like that. When rereading I can see that we_offs is
>> only sacred if WRDE_DOOFFS is given. We therefore need to store the
>> non-use of an offset someplace else. But we can't expand wordexp_t, as
>> that would break existing apps already compiled to allocate the existing
>> size of memory needed for wordexp_t.
>>
>> The best I could come up with was to allocate one extra pointer and no
>> longer have we_wordv point directly at the allocated memory, but instead
>> point one pointer into the allocation. That way, we get a private area
>> at the start of the allocation to use as we see fit.
>>
>> I added a comment in the code explaining this abnormality.
>>
>> If you consider this too convoluted and prefer that application using
>> the we_offs member for their own needs when WEDE_DOOFFS is not used
>> simply deserve to lose, I'm perfectly happy with the wordexp-3.patch in
>> the previous mail.
>
> Whoops again. Anyway, this time when looking up the thread to PING the
> patch, I noticed that I used exit instead of _exit in the child. So,
> continuing the monologue, I have now replaced exit(1) with
> _exit(EXIT_FAILURE) for the cases where things go south in the child.
> I'm still pretty happy with wordexp-3.patch, but it too needs this
> s/exit/_exit/ change of course.
>
> Is the patch too big? Should I split it? Are we waiting for a new Cygwin
> bash to appear to simplify testing? Anything?
Forgot the patch...
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 6 Sep 2012 21:01:36 -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,42 @@
{
offs = pwordexp->we_offs;
- if(!(pwordexp->we_wordv = (char **)realloc(pwordexp->we_wordv, (pwordexp->we_wordc + offs + 1) * sizeof(char *))))
- return WRDE_NOSPACE;
+ /* See the comment near the *other* realloc call for an explanation of
+ * this mess.
+ */
+ wordv = pwordexp->we_wordv;
+ if (wordv)
+ --wordv;
+ wordv = (char **)realloc(wordv, (1 + pwordexp->we_wordc + offs + 1) * sizeof(char *));
+ if (!wordv)
+ return err;
+ *wordv = (char *)(wordv + 1 + offs);
+ pwordexp->we_wordv = wordv + 1;
for (i = 0; i < offs; i++)
pwordexp->we_wordv[i] = NULL;
}
- 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 +107,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 +133,95 @@
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);
+ /* POSIX does not give us the liberty to trust the we_offs member if
+ * WRDE_DOOFFS is not given, and we can therefore not use it to store
+ * the fact that no offset has been given. Changing the size of
+ * wordexp_t breaks existing applications. Therefore, when allocating
+ * storage for the expanded words, allocate one extra pointer at the
+ * start and make the we_wordv member point one pointer into the
+ * allocated storage. Use the extra pointer to store where the
+ * real expanded words begin, i.e. always keep track of the offset
+ * even if WRDE_DOOFFS is not given.
+ */
+ wordv = pwordexp->we_wordv;
+ if (wordv)
+ --wordv;
+ wordv = (char **)realloc(wordv,
+ (1 + pwordexp->we_wordc + num_words + offs + 1) * sizeof(char *));
+ if (!wordv)
+ goto cleanup;
+ *wordv = (char *)(wordv + 1 + offs);
+ pwordexp->we_wordv = wordv + 1;
+
+ /* 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 +234,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(EXIT_FAILURE);
/* fd[1] no longer required. */
close(fd[1]);
}
@@ -170,7 +243,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(EXIT_FAILURE);
/* fd_err[1] no longer required. */
close(fd_err[1]);
}
@@ -179,6 +253,7 @@
execl("/bin/bash", "bash", "--protected", "--wordexp", words, (char *)0);
else
execl("/bin/bash", "bash", "--wordexp", words, (char *)0);
+ _exit(EXIT_FAILURE);
}
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 6 Sep 2012 21:01:36 -0000
@@ -25,6 +25,7 @@
wordfree(wordexp_t *pwordexp)
{
int i;
+ char **wordv;
if (pwordexp == NULL)
return;
@@ -32,10 +33,15 @@
if (pwordexp->we_wordv == NULL)
return;
+ /* we_wordv points one pointer into the allocated storage, the
+ * the extra pointer points at the address of the first expanded
+ * word.
+ */
+ wordv = (char **)(*(pwordexp->we_wordv - 1));
for(i = 0; i < pwordexp->we_wordc; i++)
- free(pwordexp->we_wordv[i]);
+ free(*wordv++);
- free(pwordexp->we_wordv);
+ free(pwordexp->we_wordv - 1);
pwordexp->we_wordv = NULL;
}