| Message ID | 1393563654-16490-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Stephen, On 27 February 2014 22:00, Stephen Warren <swarren@wwwdotorg.org> wrote: > The following shell command fails: > > if test -z "$x"; then echo "zero"; else echo "non-zero"; fi > > (assuming $x does not exist, it prints "non-zero" rather than "zero"). > > ... since "$x" expands to nothing, and the argument is completely > dropped, causing too few to be passed to -z, causing cmd_test() to > error out early. > > This is because when variable expansions are processed by make_string(), > the expanded results are concatenated back into a new string. However, > no quoting is applied when doing so, so any empty variables simply don't > generate any parameter when the combined string is parsed again. > > Fix this by explicitly replacing quoting any argument that was originally > quoted when re-generating a string from the already-parsed argument list. > > This also fixes loss of whitespace in commands such as: > > setenv space " " > setenv var " 1${space}${space} 2 " > echo ">>${var}<<" > Is there an upstream still for hush, or are we so far away that it doesn't matter? If there is, was this bug fixed there? A few thoughts below in case you re-issue. > > Reported-by: Russell King <linux@arm.linux.org.uk> > Signed-off-by: Stephen Warren <swarren@wwwdotorg.org> > Acked-by: Simon Glass <sjg@chromium.org> > --- > v2: > * Save the "nonnull" value from the argument parser, and pass this to > make_string(), so that it only quotes the values in the resultant > string if the original value was quoted. > * Add some more unit tests to ensure that whitespace in quoted text is > expanded and re-concatenated without loss. > * Revert accidental s/SUBSTED_VAR_SYMBOL/q/ > --- > common/hush.c | 24 +++++++++++++++++++----- > test/command_ut.c | 17 +++++++++++++++++ > 2 files changed, 36 insertions(+), 5 deletions(-) > > diff --git a/common/hush.c b/common/hush.c > index 3f3a79c..66ece41 100644 > --- a/common/hush.c > +++ b/common/hush.c > @@ -221,6 +221,7 @@ struct child_prog { > pid_t pid; /* 0 if exited */ > #endif > char **argv; /* program name and > arguments */ > + int *argv_nonnull; > This could do with a comment. > #ifdef __U_BOOT__ > int argc; /* number of program > arguments */ > #endif > @@ -467,7 +468,7 @@ static int process_command_subs(o_string *dest, struct > p_context *ctx, struct in > static int parse_group(o_string *dest, struct p_context *ctx, struct > in_str *input, int ch); > #endif > static char *lookup_param(char *src); > -static char *make_string(char **inp); > +static char *make_string(char **inp, int *nonnull); > static int handle_dollar(o_string *dest, struct p_context *ctx, struct > in_str *input); > #ifndef __U_BOOT__ > static int parse_string(o_string *dest, struct p_context *ctx, const char > *src); > @@ -1613,7 +1614,8 @@ static int run_pipe_real(struct pipe *pi) > if (child->sp) { > char * str = NULL; > > - str = make_string((child->argv + i)); > + str = make_string(child->argv + i, > + child->argv_nonnull + i); > parse_string_outer(str, FLAG_EXIT_FROM_LOOP | > FLAG_REPARSING); > free(str); > return last_return_code; > @@ -1940,7 +1942,8 @@ static int free_pipe(struct pipe *pi, int indent) > for (a = 0; a < child->argc; a++) { > free(child->argv[a]); > } > - free(child->argv); > + free(child->argv); > + free(child->argv_nonnull); > child->argc = 0; > #endif > child->argv=NULL; > @@ -2470,8 +2473,14 @@ static int done_word(o_string *dest, struct > p_context *ctx) > argc = ++child->argc; > child->argv = realloc(child->argv, > (argc+1)*sizeof(*child->argv)); > if (child->argv == NULL) return 1; > + child->argv_nonnull = realloc(child->argv_nonnull, > + > (argc+1)*sizeof(*child->argv_nonnull)); > + if (child->argv_nonnull == NULL) > + return 1; > child->argv[argc-1]=str; > + child->argv_nonnull[argc-1] = dest->nonnull; > child->argv[argc]=NULL; > + child->argv_nonnull[argc] = 0; > NULL to be consistent? > for (s = dest->data; s && *s; s++,str++) { > if (*s == '\\') s++; > *str = *s; > @@ -2537,6 +2546,7 @@ static int done_command(struct p_context *ctx) > prog->redirects = NULL; > #endif > prog->argv = NULL; > + prog->argv_nonnull = NULL; > #ifndef __U_BOOT__ > prog->is_stopped = 0; > #endif > @@ -3586,7 +3596,7 @@ static char **make_list_in(char **inp, char *name) > } > > /* Make new string for parser */ > -static char * make_string(char ** inp) > +static char *make_string(char **inp, int *nonnull) > Could do with a comment for the args I think. > { > char *p; > char *str = NULL; > @@ -3600,13 +3610,17 @@ static char * make_string(char ** inp) > noeval = 1; > for (n = 0; inp[n]; n++) { > p = insert_var_value_sub(inp[n], noeval); > - str = xrealloc(str, (len + strlen(p))); > + str = xrealloc(str, (len + strlen(p) + (2 * nonnull[n]))); > if (n) { > strcat(str, " "); > } else { > *str = '\0'; > } > + if (nonnull[n]) > + strcat(str, "'"); > strcat(str, p); > + if (nonnull[n]) > + strcat(str, "'"); > len = strlen(str) + 3; > if (p != inp[n]) free(p); > } > diff --git a/test/command_ut.c b/test/command_ut.c > index 620a297..56041e9 100644 > --- a/test/command_ut.c > +++ b/test/command_ut.c > @@ -137,6 +137,23 @@ static int do_ut_cmd(cmd_tbl_t *cmdtp, int flag, int > argc, char * const argv[]) > HUSH_TEST(or_1_0_inv_inv, "! ! aaa = aaa -o ! ! bbb != bbb", y); > HUSH_TEST(or_1_1_inv_inv, "! ! aaa = aaa -o ! ! bbb = bbb", y); > > + setenv("ut_var_nonexistent", NULL); > + setenv("ut_var_exists", "1"); > + HUSH_TEST(z_varexp_quoted, "-z \"$ut_var_nonexistent\"", y); > + HUSH_TEST(z_varexp_quoted, "-z \"$ut_var_exists\"", n); > + setenv("ut_var_exists", NULL); > + > + run_command("setenv ut_var_space \" \"", 0); > + assert(!strcmp(getenv("ut_var_space"), " ")); > + run_command("setenv ut_var_test $ut_var_space", 0); > + assert(!getenv("ut_var_test")); > + run_command("setenv ut_var_test \"$ut_var_space\"", 0); > + assert(!strcmp(getenv("ut_var_test"), " ")); > + run_command("setenv ut_var_test \" 1${ut_var_space}${ut_var_space} > 2 \"", 0); > + assert(!strcmp(getenv("ut_var_test"), " 1 2 ")); > + setenv("ut_var_space", NULL); > + setenv("ut_var_test", NULL); > Nice set of tests. > + > #ifdef CONFIG_SANDBOX > /* > * File existence > -- > 1.8.3.2 > > Regards, Simon
Hi Stephen, On 1 March 2014 17:10, Simon Glass <sjg@chromium.org> wrote: > Hi Stephen, > > On 27 February 2014 22:00, Stephen Warren <swarren@wwwdotorg.org> wrote: > >> The following shell command fails: >> >> if test -z "$x"; then echo "zero"; else echo "non-zero"; fi >> >> (assuming $x does not exist, it prints "non-zero" rather than "zero"). >> >> ... since "$x" expands to nothing, and the argument is completely >> dropped, causing too few to be passed to -z, causing cmd_test() to >> error out early. >> > One more thing - with sandbox I get an error with the tests: /test/command_ut.c:162: do_ut_cmd: Assertion `!strcmp("y", getenv("e" "_" "y"))' failed. This is because I'm not running sandbox in-tree. Is it possible to fix this with some call to os_getcwd() or similar? Or maybe use /tmp? Regards, Simon
On 03/01/2014 05:10 PM, Simon Glass wrote: > Hi Stephen, > > On 27 February 2014 22:00, Stephen Warren <swarren@wwwdotorg.org > <mailto:swarren@wwwdotorg.org>> wrote: > > The following shell command fails: > > if test -z "$x"; then echo "zero"; else echo "non-zero"; fi > > (assuming $x does not exist, it prints "non-zero" rather than "zero"). > > ... since "$x" expands to nothing, and the argument is completely > dropped, causing too few to be passed to -z, causing cmd_test() to > error out early. > > This is because when variable expansions are processed by make_string(), > the expanded results are concatenated back into a new string. However, > no quoting is applied when doing so, so any empty variables simply don't > generate any parameter when the combined string is parsed again. > > Fix this by explicitly replacing quoting any argument that was > originally > quoted when re-generating a string from the already-parsed argument > list. > > This also fixes loss of whitespace in commands such as: > > setenv space " " > setenv var " 1${space}${space} 2 " > echo ">>${var}<<" > > > Is there an upstream still for hush, or are we so far away that it > doesn't matter? If there is, was this bug fixed there? Well, the comments at the head of the file say it came from Busybox, but it's obviously diverged massively since it was imported: $ wc -l busybox/shell/hush.c u-boot/common/hush.c 9156 busybox/shell/hush.c 3682 u-boot/common/hush.c $ diff -u busybox/shell/hush.c u-boot/common/hush.c|wc -l 12264 Also, the function this patch touches doesn't seem to exist under that name any more. From a quick look at the source, I couldn't tell what the equivalent is. Perhaps replaying patches to that file in Busybox since the fork might be more useful. A quick Google search for "hush" or "hush shell" doesn't seem to yield any other alternative upstreams. > diff --git a/common/hush.c b/common/hush.c > index 3f3a79c..66ece41 100644 > --- a/common/hush.c > +++ b/common/hush.c > @@ -221,6 +221,7 @@ struct child_prog { > pid_t pid; /* 0 if > exited */ > #endif > char **argv; /* program name and > arguments */ > + int *argv_nonnull; > > > This could do with a comment. I did wonder if I should name this "quoted". I'd originally shied away from this to prevent changing the terminology, but since hush.c has diverged so much, perhaps that wouldn't be an issue...
On 03/01/2014 05:10 PM, Simon Glass wrote: > Hi Stephen, > > On 27 February 2014 22:00, Stephen Warren <swarren@wwwdotorg.org > <mailto:swarren@wwwdotorg.org>> wrote: > > The following shell command fails: > > if test -z "$x"; then echo "zero"; else echo "non-zero"; fi > > (assuming $x does not exist, it prints "non-zero" rather than "zero"). ... > @@ -2470,8 +2473,14 @@ static int done_word(o_string *dest, struct > p_context *ctx) > argc = ++child->argc; > child->argv = realloc(child->argv, > (argc+1)*sizeof(*child->argv)); > if (child->argv == NULL) return 1; > + child->argv_nonnull = realloc(child->argv_nonnull, > + > (argc+1)*sizeof(*child->argv_nonnull)); > + if (child->argv_nonnull == NULL) > + return 1; > child->argv[argc-1]=str; > + child->argv_nonnull[argc-1] = dest->nonnull; > child->argv[argc]=NULL; > + child->argv_nonnull[argc] = 0; > > > NULL to be consistent? This is assigning an entry in the array, and the entry type is int. I'll fix up the other issues.
On Sat, Mar 01, 2014 at 09:26:26PM -0700, Stephen Warren wrote: > On 03/01/2014 05:10 PM, Simon Glass wrote: > > Hi Stephen, > > > > On 27 February 2014 22:00, Stephen Warren <swarren@wwwdotorg.org > > <mailto:swarren@wwwdotorg.org>> wrote: > > > > The following shell command fails: > > > > if test -z "$x"; then echo "zero"; else echo "non-zero"; fi > > > > (assuming $x does not exist, it prints "non-zero" rather than "zero"). > > > > ... since "$x" expands to nothing, and the argument is completely > > dropped, causing too few to be passed to -z, causing cmd_test() to > > error out early. > > > > This is because when variable expansions are processed by make_string(), > > the expanded results are concatenated back into a new string. However, > > no quoting is applied when doing so, so any empty variables simply don't > > generate any parameter when the combined string is parsed again. > > > > Fix this by explicitly replacing quoting any argument that was > > originally > > quoted when re-generating a string from the already-parsed argument > > list. > > > > This also fixes loss of whitespace in commands such as: > > > > setenv space " " > > setenv var " 1${space}${space} 2 " > > echo ">>${var}<<" > > > > > > Is there an upstream still for hush, or are we so far away that it > > doesn't matter? If there is, was this bug fixed there? > > Well, the comments at the head of the file say it came from Busybox, but > it's obviously diverged massively since it was imported: > > $ wc -l busybox/shell/hush.c u-boot/common/hush.c > 9156 busybox/shell/hush.c > 3682 u-boot/common/hush.c > $ diff -u busybox/shell/hush.c u-boot/common/hush.c|wc -l > 12264 > > Also, the function this patch touches doesn't seem to exist under that > name any more. From a quick look at the source, I couldn't tell what the > equivalent is. > > Perhaps replaying patches to that file in Busybox since the fork might > be more useful. > > A quick Google search for "hush" or "hush shell" doesn't seem to yield > any other alternative upstreams. For reference the patch is at http://patchwork.ozlabs.org/patch/325023/ Since U-Boot and Barebox are the only users of this particular hush fork, and from G+ they also have this problem, maybe we can use this as a starting point of a little more friendly coordination between the projects?
diff --git a/common/hush.c b/common/hush.c index 3f3a79c..66ece41 100644 --- a/common/hush.c +++ b/common/hush.c @@ -221,6 +221,7 @@ struct child_prog { pid_t pid; /* 0 if exited */ #endif char **argv; /* program name and arguments */ + int *argv_nonnull; #ifdef __U_BOOT__ int argc; /* number of program arguments */ #endif @@ -467,7 +468,7 @@ static int process_command_subs(o_string *dest, struct p_context *ctx, struct in static int parse_group(o_string *dest, struct p_context *ctx, struct in_str *input, int ch); #endif static char *lookup_param(char *src); -static char *make_string(char **inp); +static char *make_string(char **inp, int *nonnull); static int handle_dollar(o_string *dest, struct p_context *ctx, struct in_str *input); #ifndef __U_BOOT__ static int parse_string(o_string *dest, struct p_context *ctx, const char *src); @@ -1613,7 +1614,8 @@ static int run_pipe_real(struct pipe *pi) if (child->sp) { char * str = NULL; - str = make_string((child->argv + i)); + str = make_string(child->argv + i, + child->argv_nonnull + i); parse_string_outer(str, FLAG_EXIT_FROM_LOOP | FLAG_REPARSING); free(str); return last_return_code; @@ -1940,7 +1942,8 @@ static int free_pipe(struct pipe *pi, int indent) for (a = 0; a < child->argc; a++) { free(child->argv[a]); } - free(child->argv); + free(child->argv); + free(child->argv_nonnull); child->argc = 0; #endif child->argv=NULL; @@ -2470,8 +2473,14 @@ static int done_word(o_string *dest, struct p_context *ctx) argc = ++child->argc; child->argv = realloc(child->argv, (argc+1)*sizeof(*child->argv)); if (child->argv == NULL) return 1; + child->argv_nonnull = realloc(child->argv_nonnull, + (argc+1)*sizeof(*child->argv_nonnull)); + if (child->argv_nonnull == NULL) + return 1; child->argv[argc-1]=str; + child->argv_nonnull[argc-1] = dest->nonnull; child->argv[argc]=NULL; + child->argv_nonnull[argc] = 0; for (s = dest->data; s && *s; s++,str++) { if (*s == '\\') s++; *str = *s; @@ -2537,6 +2546,7 @@ static int done_command(struct p_context *ctx) prog->redirects = NULL; #endif prog->argv = NULL; + prog->argv_nonnull = NULL; #ifndef __U_BOOT__ prog->is_stopped = 0; #endif @@ -3586,7 +3596,7 @@ static char **make_list_in(char **inp, char *name) } /* Make new string for parser */ -static char * make_string(char ** inp) +static char *make_string(char **inp, int *nonnull) { char *p; char *str = NULL; @@ -3600,13 +3610,17 @@ static char * make_string(char ** inp) noeval = 1; for (n = 0; inp[n]; n++) { p = insert_var_value_sub(inp[n], noeval); - str = xrealloc(str, (len + strlen(p))); + str = xrealloc(str, (len + strlen(p) + (2 * nonnull[n]))); if (n) { strcat(str, " "); } else { *str = '\0'; } + if (nonnull[n]) + strcat(str, "'"); strcat(str, p); + if (nonnull[n]) + strcat(str, "'"); len = strlen(str) + 3; if (p != inp[n]) free(p); } diff --git a/test/command_ut.c b/test/command_ut.c index 620a297..56041e9 100644 --- a/test/command_ut.c +++ b/test/command_ut.c @@ -137,6 +137,23 @@ static int do_ut_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) HUSH_TEST(or_1_0_inv_inv, "! ! aaa = aaa -o ! ! bbb != bbb", y); HUSH_TEST(or_1_1_inv_inv, "! ! aaa = aaa -o ! ! bbb = bbb", y); + setenv("ut_var_nonexistent", NULL); + setenv("ut_var_exists", "1"); + HUSH_TEST(z_varexp_quoted, "-z \"$ut_var_nonexistent\"", y); + HUSH_TEST(z_varexp_quoted, "-z \"$ut_var_exists\"", n); + setenv("ut_var_exists", NULL); + + run_command("setenv ut_var_space \" \"", 0); + assert(!strcmp(getenv("ut_var_space"), " ")); + run_command("setenv ut_var_test $ut_var_space", 0); + assert(!getenv("ut_var_test")); + run_command("setenv ut_var_test \"$ut_var_space\"", 0); + assert(!strcmp(getenv("ut_var_test"), " ")); + run_command("setenv ut_var_test \" 1${ut_var_space}${ut_var_space} 2 \"", 0); + assert(!strcmp(getenv("ut_var_test"), " 1 2 ")); + setenv("ut_var_space", NULL); + setenv("ut_var_test", NULL); + #ifdef CONFIG_SANDBOX /* * File existence
The following shell command fails: if test -z "$x"; then echo "zero"; else echo "non-zero"; fi (assuming $x does not exist, it prints "non-zero" rather than "zero"). ... since "$x" expands to nothing, and the argument is completely dropped, causing too few to be passed to -z, causing cmd_test() to error out early. This is because when variable expansions are processed by make_string(), the expanded results are concatenated back into a new string. However, no quoting is applied when doing so, so any empty variables simply don't generate any parameter when the combined string is parsed again. Fix this by explicitly replacing quoting any argument that was originally quoted when re-generating a string from the already-parsed argument list. This also fixes loss of whitespace in commands such as: setenv space " " setenv var " 1${space}${space} 2 " echo ">>${var}<<" Reported-by: Russell King <linux@arm.linux.org.uk> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org> --- v2: * Save the "nonnull" value from the argument parser, and pass this to make_string(), so that it only quotes the values in the resultant string if the original value was quoted. * Add some more unit tests to ensure that whitespace in quoted text is expanded and re-concatenated without loss. * Revert accidental s/SUBSTED_VAR_SYMBOL/q/ --- common/hush.c | 24 +++++++++++++++++++----- test/command_ut.c | 17 +++++++++++++++++ 2 files changed, 36 insertions(+), 5 deletions(-)