This is the mail archive of the
newlib@sourceware.org
mailing list for the newlib project.
Re: [patch] wordexp resource cleanup
On 2012-08-20 00:00, Peter Rosin wrote:
> 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.
>
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.
Cheers,
Peter
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 10:12:41 -0000
@@ -37,9 +37,13 @@
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,35 @@
{
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;
}
- 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. */
@@ -104,52 +125,74 @@
fprintf(stderr, tmp);
}
- return err;
+ goto cleanup;
}
/* f is the standard output from the shell command. */
f = fdopen(fd[0], "r");
/* 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 + offs + num_words + 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. */
- for(i = 0; i < num_words; i++)
- {
- fgets(tmp, MAXLINELEN, f);
+ num_bytes = atoi(tmp);
- if((iter = strchr(tmp, '\n')))
- *iter = '\0';
+ /* 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;
- pwordexp->we_wordv[pwordexp->we_wordc + offs + i] = strdup(tmp);
+ /* Store each entry in pwordexp's we_wordv vector. */
+ eword = ewords;
+ for(i = 0; i < num_words; i++)
+ {
+ if (eword && (iter = strchr(eword, '\n'))) {
+ *iter = '\0';}
+ 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;
+cleanup:
+ free(ewords);
close(fd[0]);
close(fd_err[0]);
/* Wait for child to finish. */
waitpid (pid, NULL, 0);
- return WRDE_SUCCESS;
+ return err;
}
else
{
@@ -162,7 +205,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 +214,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 +224,7 @@
execl("/bin/bash", "bash", "--protected", "--wordexp", words, (char *)0);
else
execl("/bin/bash", "bash", "--wordexp", words, (char *)0);
+ exit(1);
}
return WRDE_SUCCESS;
}