[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