hush: fix a bug where we don't properly handle f() { a=A; b=B; }; a= f
authorDenys Vlasenko <vda.linux@googlemail.com>
Wed, 4 Apr 2018 22:51:55 +0000 (00:51 +0200)
committerDenys Vlasenko <vda.linux@googlemail.com>
Wed, 4 Apr 2018 22:51:55 +0000 (00:51 +0200)
function                                             old     new   delta
unset_local_var                                       20     274    +254
leave_var_nest_level                                   -      98     +98
set_vars_and_save_old                                128     164     +36
enter_var_nest_level                                   -      32     +32
builtin_local                                         46      50      +4
pseudo_exec_argv                                     554     544     -10
redirect_and_varexp_helper                            77      64     -13
run_pipe                                            1890    1641    -249
unset_local_var_len                                  267       -    -267
------------------------------------------------------------------------------
(add/remove: 2/1 grow/shrink: 3/3 up/down: 424/-539)         Total: -115 bytes

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
shell/hush.c
shell/hush_test/hush-vars/var_nested1.right [new file with mode: 0644]
shell/hush_test/hush-vars/var_nested1.tests [new file with mode: 0755]
shell/hush_test/hush-vars/var_nested2.right [new file with mode: 0644]
shell/hush_test/hush-vars/var_nested2.tests [new file with mode: 0755]

index 9fb9b5f325efc6cc935daae81d9149d2ce9d7e0b..00d86b4e4c7a75f9aa17376db4f833f25d0ca4e2 100644 (file)
@@ -475,7 +475,6 @@ static const char hush_version_str[] ALIGN1 = "HUSH_VERSION="BB_VER;
  */
 #if !BB_MMU
 typedef struct nommu_save_t {
-       char **new_env;
        struct variable *old_vars;
        char **argv;
        char **argv_from_re_execing;
@@ -2176,10 +2175,16 @@ static int set_local_var(char *str, unsigned flags)
                        *eq_sign = '=';
                }
                if (cur->var_nest_level < local_lvl) {
-                       /* New variable is declared as local,
+                       /* New variable is local ("local VAR=VAL" or
+                        * "VAR=VAL cmd")
                         * and existing one is global, or local
-                        * from enclosing function.
-                        * Remove and save old one: */
+                        * on a lower level that new one.
+                        * Remove and save old one:
+                        */
+                       debug_printf_env("shadowing %s'%s'/%u by '%s'/%u\n",
+                               cur->flg_export ? "exported " : "",
+                               cur->varstr, cur->var_nest_level, str, local_lvl
+                       );
                        *cur_pp = cur->next;
                        cur->next = *G.shadowed_vars_pp;
                        *G.shadowed_vars_pp = cur;
@@ -2197,6 +2202,7 @@ static int set_local_var(char *str, unsigned flags)
                }
 
                if (strcmp(cur->varstr + name_len, eq_sign + 1) == 0) {
+                       debug_printf_env("assignement '%s' does not change anything\n", str);
  free_and_exp:
                        free(str);
                        goto exp;
@@ -2204,6 +2210,7 @@ static int set_local_var(char *str, unsigned flags)
                if (cur->max_len != 0) {
                        if (cur->max_len >= strlen(str)) {
                                /* This one is from startup env, reuse space */
+                               debug_printf_env("reusing startup env for '%s'\n", str);
                                strcpy(cur->varstr, str);
                                goto free_and_exp;
                        }
@@ -2221,7 +2228,7 @@ static int set_local_var(char *str, unsigned flags)
                goto set_str_and_exp;
        }
 
-       /* Not found - create new variable struct */
+       /* Not found or shadowed - create new variable struct */
        cur = xzalloc(sizeof(*cur));
        cur->var_nest_level = local_lvl;
        cur->next = *cur_pp;
@@ -2250,7 +2257,7 @@ static int set_local_var(char *str, unsigned flags)
                        /* unsetenv was already done */
                } else {
                        int i;
-                       debug_printf_env("%s: putenv '%s'\n", __func__, cur->varstr);
+                       debug_printf_env("%s: putenv '%s'/%u\n", __func__, cur->varstr, cur->var_nest_level);
                        i = putenv(cur->varstr);
                        /* only now we can free old exported malloced string */
                        free(free_me);
@@ -2313,21 +2320,6 @@ static int unset_local_var(const char *name)
 }
 #endif
 
-static void unset_vars(char **strings)
-{
-       char **v;
-
-       if (!strings)
-               return;
-       v = strings;
-       while (*v) {
-               const char *eq = strchrnul(*v, '=');
-               unset_local_var_len(*v, (int)(eq - *v));
-               v++;
-       }
-       free(strings);
-}
-
 #if BASH_HOSTNAME_VAR || ENABLE_FEATURE_SH_MATH || ENABLE_HUSH_READ || ENABLE_HUSH_GETOPTS
 static void FAST_FUNC set_local_var_from_halves(const char *name, const char *val)
 {
@@ -2349,15 +2341,20 @@ static void add_vars(struct variable *var)
                var->next = G.top_var;
                G.top_var = var;
                if (var->flg_export) {
-                       debug_printf_env("%s: restoring exported '%s'\n", __func__, var->varstr);
+                       debug_printf_env("%s: restoring exported '%s'/%u\n", __func__, var->varstr, var->var_nest_level);
                        putenv(var->varstr);
                } else {
-                       debug_printf_env("%s: restoring variable '%s'\n", __func__, var->varstr);
+                       debug_printf_env("%s: restoring variable '%s'/%u\n", __func__, var->varstr, var->var_nest_level);
                }
                var = next;
        }
 }
 
+/* We put strings[i] into variable table and possibly putenv them.
+ * If variable is read only, we can free the strings[i]
+ * which attempts to overwrite it.
+ * The strings[] vector itself is freed.
+ */
 static struct variable *set_vars_and_save_old(char **strings)
 {
        char **s;
@@ -2365,6 +2362,7 @@ static struct variable *set_vars_and_save_old(char **strings)
 
        if (!strings)
                return old;
+
        s = strings;
        while (*s) {
                struct variable *var_p;
@@ -2398,11 +2396,15 @@ static struct variable *set_vars_and_save_old(char **strings)
                                var_p->next = old;
                                old = var_p;
                        }
-                       set_local_var(*s, SETFLAG_EXPORT);
+                       //bb_error_msg("G.var_nest_level:%d", G.var_nest_level);
+                       set_local_var(*s, (G.var_nest_level << SETFLAG_VARLVL_SHIFT) | SETFLAG_EXPORT);
+               } else if (HUSH_DEBUG) {
+                       bb_error_msg_and_die("BUG in varexp4");
                }
                s++;
  next: ;
        }
+       free(strings);
        return old;
 }
 
@@ -6339,7 +6341,7 @@ static char **expand_strvec_to_strvec_singleword_noglob(char **argv)
 #endif
 
 /* Used for expansion of right hand of assignments,
- * $((...)), heredocs, variable espansion parts.
+ * $((...)), heredocs, variable expansion parts.
  *
  * NB: should NOT do globbing!
  * "export v=/bin/c*; env | grep ^v=" outputs "v=/bin/c*"
@@ -7385,6 +7387,7 @@ static void exec_function(char ***to_free,
 static void enter_var_nest_level(void)
 {
        G.var_nest_level++;
+       debug_printf_env("var_nest_level++ %u\n", G.var_nest_level);
 
        /* Try: f() { echo -n .; f; }; f
         * struct variable::var_nest_level is uint16_t,
@@ -7408,17 +7411,22 @@ static void leave_var_nest_level(void)
                        continue;
                }
                /* Unexport */
-               if (cur->flg_export)
+               if (cur->flg_export) {
+                       debug_printf_env("unexporting nested '%s'/%u\n", cur->varstr, cur->var_nest_level);
                        bb_unsetenv(cur->varstr);
+               }
                /* Remove from global list */
                *cur_pp = cur->next;
                /* Free */
-               if (!cur->max_len)
+               if (!cur->max_len) {
+                       debug_printf_env("freeing nested '%s'/%u\n", cur->varstr, cur->var_nest_level);
                        free(cur->varstr);
+               }
                free(cur);
        }
 
        G.var_nest_level--;
+       debug_printf_env("var_nest_level-- %u\n", G.var_nest_level);
        if (HUSH_DEBUG && (int)G.var_nest_level < 0)
                bb_error_msg_and_die("BUG: nesting underflow");
 }
@@ -7431,11 +7439,12 @@ static int run_function(const struct function *funcp, char **argv)
 
        save_and_replace_G_args(&sv, argv);
 
-       /* "we are in function, ok to use return" */
+       /* "We are in function, ok to use return" */
        sv_flg = G_flag_return_in_progress;
        G_flag_return_in_progress = -1;
 
-       enter_var_nest_level();
+       /* Make "local" variables properly shadow previous ones */
+       IF_HUSH_LOCAL(enter_var_nest_level();)
        IF_HUSH_LOCAL(G.func_nest_level++;)
 
        /* On MMU, funcp->body is always non-NULL */
@@ -7451,7 +7460,7 @@ static int run_function(const struct function *funcp, char **argv)
        }
 
        IF_HUSH_LOCAL(G.func_nest_level--;)
-       leave_var_nest_level();
+       IF_HUSH_LOCAL(leave_var_nest_level();)
 
        G_flag_return_in_progress = sv_flg;
 
@@ -7608,11 +7617,9 @@ static NOINLINE void pseudo_exec_argv(nommu_save_t *nommu_save,
 
 #if BB_MMU
        set_vars_and_save_old(new_env);
-       free(new_env); /* optional */
-       /* we can also destroy set_vars_and_save_old's return value,
+       /* we can destroy set_vars_and_save_old's return value,
         * to save memory */
 #else
-       nommu_save->new_env = new_env;
        nommu_save->old_vars = set_vars_and_save_old(new_env);
 #endif
 
@@ -8174,10 +8181,10 @@ static int checkjobs_and_fg_shell(struct pipe *fg_pipe)
  * subshell:     ( list ) [&]
  */
 #if !ENABLE_HUSH_MODE_X
-#define redirect_and_varexp_helper(new_env_p, old_vars_p, command, squirrel, argv_expanded) \
-       redirect_and_varexp_helper(new_env_p, old_vars_p, command, squirrel)
+#define redirect_and_varexp_helper(old_vars_p, command, squirrel, argv_expanded) \
+       redirect_and_varexp_helper(old_vars_p, command, squirrel)
 #endif
-static int redirect_and_varexp_helper(char ***new_env_p,
+static int redirect_and_varexp_helper(
                struct variable **old_vars_p,
                struct command *command,
                struct squirrel **sqp,
@@ -8190,11 +8197,10 @@ static int redirect_and_varexp_helper(char ***new_env_p,
        int rcode = setup_redirects(command, sqp);
        if (rcode == 0) {
                char **new_env = expand_assignments(command->argv, command->assignment_cnt);
-               *new_env_p = new_env;
                dump_cmd_in_x_mode(new_env);
                dump_cmd_in_x_mode(argv_expanded);
-               if (old_vars_p)
-                       *old_vars_p = set_vars_and_save_old(new_env);
+               /* this takes ownership of new_env[i] elements, and frees new_env: */
+               *old_vars_p = set_vars_and_save_old(new_env);
        }
        return rcode;
 }
@@ -8284,13 +8290,9 @@ static NOINLINE int run_pipe(struct pipe *pi)
        argv = command->argv ? command->argv : (char **) &null_ptr;
        {
                const struct built_in_command *x;
-#if ENABLE_HUSH_FUNCTIONS
-               const struct function *funcp;
-#else
-               enum { funcp = 0 };
-#endif
-               char **new_env = NULL;
-               struct variable *old_vars = NULL;
+               IF_HUSH_FUNCTIONS(const struct function *funcp;)
+               IF_NOT_HUSH_FUNCTIONS(enum { funcp = 0 };)
+               struct variable *old_vars;
 
 #if ENABLE_HUSH_LINENO_VAR
                if (G.lineno_var)
@@ -8303,28 +8305,6 @@ static NOINLINE int run_pipe(struct pipe *pi)
                         * Try "a=t >file"
                         */
                        G.expand_exitcode = 0;
-#if 0 /* A few cases in testsuite fail with this code. FIXME */
-                       rcode = redirect_and_varexp_helper(&new_env, /*old_vars:*/ NULL, command, &squirrel, /*argv_expanded:*/ NULL);
-                       /* Set shell variables */
-                       if (new_env) {
-                               argv = new_env;
-                               while (*argv) {
-                                       if (set_local_var(*argv, /*flag:*/ 0)) {
-                                               /* assignment to readonly var / putenv error? */
-                                               rcode = 1;
-                                       }
-                                       argv++;
-                               }
-                       }
-                       /* Redirect error sets $? to 1. Otherwise,
-                        * if evaluating assignment value set $?, retain it.
-                        * Try "false; q=`exit 2`; echo $?" - should print 2: */
-                       if (rcode == 0)
-                               rcode = G.expand_exitcode;
-                       /* Exit, _skipping_ variable restoring code: */
-                       goto clean_up_and_ret0;
-
-#else /* Older, bigger, but more correct code */
 
                        rcode = setup_redirects(command, &squirrel);
                        restore_redirects(squirrel);
@@ -8358,7 +8338,6 @@ static NOINLINE int run_pipe(struct pipe *pi)
                        debug_leave();
                        debug_printf_exec("run_pipe: return %d\n", rcode);
                        return rcode;
-#endif
                }
 
                /* Expand the rest into (possibly) many strings each */
@@ -8372,6 +8351,7 @@ static NOINLINE int run_pipe(struct pipe *pi)
                }
 
                /* if someone gives us an empty string: `cmd with empty output` */
+//TODO: what about: var=EXPR `` >FILE ? will var be set? Will FILE be created?
                if (!argv_expanded[0]) {
                        free(argv_expanded);
                        debug_leave();
@@ -8394,7 +8374,14 @@ static NOINLINE int run_pipe(struct pipe *pi)
                                        goto clean_up_and_ret1;
                                }
                        }
-                       rcode = redirect_and_varexp_helper(&new_env, &old_vars, command, &squirrel, argv_expanded);
+
+                       /* Without bumping var nesting level, this leaks
+                        * exported $a:
+                        * a=b true; env | grep ^a=
+                        */
+                       enter_var_nest_level();
+                       rcode = redirect_and_varexp_helper(&old_vars, command, &squirrel, argv_expanded);
+
                        if (rcode == 0) {
                                if (!funcp) {
                                        debug_printf_exec(": builtin '%s' '%s'...\n",
@@ -8405,24 +8392,23 @@ static NOINLINE int run_pipe(struct pipe *pi)
                                }
 #if ENABLE_HUSH_FUNCTIONS
                                else {
-# if ENABLE_HUSH_LOCAL
-                                       struct variable **sv;
-                                       sv = G.shadowed_vars_pp;
+                                       struct variable **sv = G.shadowed_vars_pp;
+                                       /* Make "local" builtins in the called function
+                                        * stash shadowed variables in old_vars list
+                                        */
                                        G.shadowed_vars_pp = &old_vars;
-# endif
+
                                        debug_printf_exec(": function '%s' '%s'...\n",
                                                funcp->name, argv_expanded[1]);
                                        rcode = run_function(funcp, argv_expanded) & 0xff;
-# if ENABLE_HUSH_LOCAL
+
                                        G.shadowed_vars_pp = sv;
-# endif
                                }
 #endif
                        }
  clean_up_and_ret:
-                       unset_vars(new_env);
+                       leave_var_nest_level();
                        add_vars(old_vars);
-/* clean_up_and_ret0: */
                        restore_redirects(squirrel);
                        /*
                         * Try "usleep 99999999" + ^C + "echo $?"
@@ -8453,7 +8439,8 @@ static NOINLINE int run_pipe(struct pipe *pi)
                if (ENABLE_FEATURE_SH_NOFORK && NUM_APPLETS > 1) {
                        int n = find_applet_by_name(argv_expanded[0]);
                        if (n >= 0 && APPLET_IS_NOFORK(n)) {
-                               rcode = redirect_and_varexp_helper(&new_env, &old_vars, command, &squirrel, argv_expanded);
+                               enter_var_nest_level();
+                               rcode = redirect_and_varexp_helper(&old_vars, command, &squirrel, argv_expanded);
                                if (rcode == 0) {
                                        debug_printf_exec(": run_nofork_applet '%s' '%s'...\n",
                                                argv_expanded[0], argv_expanded[1]);
@@ -8487,7 +8474,6 @@ static NOINLINE int run_pipe(struct pipe *pi)
                struct fd_pair pipefds;
 #if !BB_MMU
                volatile nommu_save_t nommu_save;
-               nommu_save.new_env = NULL;
                nommu_save.old_vars = NULL;
                nommu_save.argv = NULL;
                nommu_save.argv_from_re_execing = NULL;
@@ -8580,7 +8566,6 @@ static NOINLINE int run_pipe(struct pipe *pi)
                /* Clean up after vforked child */
                free(nommu_save.argv);
                free(nommu_save.argv_from_re_execing);
-               unset_vars(nommu_save.new_env);
                add_vars(nommu_save.old_vars);
 #endif
                free(argv_expanded);
@@ -10066,7 +10051,11 @@ static int FAST_FUNC builtin_local(char **argv)
                return EXIT_FAILURE; /* bash compat */
        }
        argv++;
-       return helper_export_local(argv, G.var_nest_level << SETFLAG_VARLVL_SHIFT);
+       /* Since all builtins run in a nested variable level,
+        * need to use level - 1 here. Or else the variable will be removed at once
+        * after builtin returns.
+        */
+       return helper_export_local(argv, (G.var_nest_level - 1) << SETFLAG_VARLVL_SHIFT);
 }
 #endif
 
diff --git a/shell/hush_test/hush-vars/var_nested1.right b/shell/hush_test/hush-vars/var_nested1.right
new file mode 100644 (file)
index 0000000..f4bc5f4
--- /dev/null
@@ -0,0 +1,3 @@
+Expected:AB Actual:AB
+Expected:Ab Actual:Ab
+Expected:ab Actual:ab
diff --git a/shell/hush_test/hush-vars/var_nested1.tests b/shell/hush_test/hush-vars/var_nested1.tests
new file mode 100755 (executable)
index 0000000..59e4a14
--- /dev/null
@@ -0,0 +1,16 @@
+f() { a=A; b=B; }
+
+a=a
+b=b
+f
+echo Expected:AB Actual:$a$b
+
+a=a
+b=b
+b= f
+echo Expected:Ab Actual:$a$b
+
+a=a
+b=b
+a= b= f
+echo Expected:ab Actual:$a$b
diff --git a/shell/hush_test/hush-vars/var_nested2.right b/shell/hush_test/hush-vars/var_nested2.right
new file mode 100644 (file)
index 0000000..c930d97
--- /dev/null
@@ -0,0 +1 @@
+aB
diff --git a/shell/hush_test/hush-vars/var_nested2.tests b/shell/hush_test/hush-vars/var_nested2.tests
new file mode 100755 (executable)
index 0000000..e886586
--- /dev/null
@@ -0,0 +1,2 @@
+# the bug was easier to trigger in one-liner form
+a=a; b=b; f() { a=A; b=B; }; a= f; echo $a$b