[U-Boot] [PATCH V2] hush: fix some quoted variable expansion issues
Simon Glass
sjg at chromium.org
Sun Mar 2 01:10:07 CET 2014
Hi Stephen,
On 27 February 2014 22:00, Stephen Warren <swarren at 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 at arm.linux.org.uk>
> Signed-off-by: Stephen Warren <swarren at wwwdotorg.org>
>
Acked-by: Simon Glass <sjg at 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
More information about the U-Boot
mailing list