| Message ID | 20210228212951.1175231-1-seanga2@gmail.com |
|---|---|
| State | Accepted |
| Commit | 9539f71675c40485e448efb3c4e06afc8d102f94 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | hush: Fix assignments being misinterpreted as commands | expand |
Am 28. Februar 2021 22:29:51 MEZ schrieb Sean Anderson <seanga2@gmail.com>: >If there were no variable substitutions in a command, then initial >assignments would be misinterpreted as commands, instead of being >skipped >over. This is demonstrated by the following example: > > => foo=bar echo baz The commit message does not explain why this patch is needed. What shall be the value off foo after this line? What will be the output of foo=bar echo ${foo} with and without yor patch? Best regards Heinrich > Unknown command 'foo=bar' - try 'help' > >Signed-off-by: Sean Anderson <seanga2@gmail.com> >--- > > common/cli_hush.c | 2 +- > test/cmd/test_echo.c | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > >diff --git a/common/cli_hush.c b/common/cli_hush.c >index b7f0f0ff41..1b9bef64b6 100644 >--- a/common/cli_hush.c >+++ b/common/cli_hush.c >@@ -1672,7 +1672,7 @@ static int run_pipe_real(struct pipe *pi) > return -1; > } > /* Process the command */ >- return cmd_process(flag, child->argc, child->argv, >+ return cmd_process(flag, child->argc - i, child->argv + i, > &flag_repeat, NULL); > #endif > } >diff --git a/test/cmd/test_echo.c b/test/cmd/test_echo.c >index 4183cf75bb..13e1fb7c82 100644 >--- a/test/cmd/test_echo.c >+++ b/test/cmd/test_echo.c >@@ -33,6 +33,8 @@ static struct test_data echo_data[] = { > */ > {"setenv jQx X; echo \"a)\" ${jQx} 'b)' '${jQx}' c) ${jQx}; setenv >jQx", > "a) X b) ${jQx} c) X"}, >+ /* Test shell variable assignments without substitutions */ >+ {"foo=bar echo baz", "baz"}, > /* Test handling of shell variables. */ > {"setenv jQx; for jQx in 1 2 3; do echo -n \"${jQx}, \"; done; echo;", > "1, 2, 3, "},
On 2/28/21 6:40 PM, Heinrich Schuchardt wrote: > Am 28. Februar 2021 22:29:51 MEZ schrieb Sean Anderson <seanga2@gmail.com>: >> If there were no variable substitutions in a command, then initial >> assignments would be misinterpreted as commands, instead of being >> skipped >> over. This is demonstrated by the following example: >> >> => foo=bar echo baz > > The commit message does not explain why this patch is needed. This is a bug I noticed while writing some tests of hush. > What shall be the value off foo after this line? It should be bar. This is an existing difference when compared with bash. For example, without this patch, we have => foo=bar echo $foo bar => echo $foo bar > > What will be the output of > > foo=bar echo ${foo} > > with and without yor patch? It is the same. --Sean > > Best regards > > Heinrich > > >> Unknown command 'foo=bar' - try 'help' >> >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> --- >> >> common/cli_hush.c | 2 +- >> test/cmd/test_echo.c | 2 ++ >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/common/cli_hush.c b/common/cli_hush.c >> index b7f0f0ff41..1b9bef64b6 100644 >> --- a/common/cli_hush.c >> +++ b/common/cli_hush.c >> @@ -1672,7 +1672,7 @@ static int run_pipe_real(struct pipe *pi) >> return -1; >> } >> /* Process the command */ >> - return cmd_process(flag, child->argc, child->argv, >> + return cmd_process(flag, child->argc - i, child->argv + i, >> &flag_repeat, NULL); >> #endif >> } >> diff --git a/test/cmd/test_echo.c b/test/cmd/test_echo.c >> index 4183cf75bb..13e1fb7c82 100644 >> --- a/test/cmd/test_echo.c >> +++ b/test/cmd/test_echo.c >> @@ -33,6 +33,8 @@ static struct test_data echo_data[] = { >> */ >> {"setenv jQx X; echo \"a)\" ${jQx} 'b)' '${jQx}' c) ${jQx}; setenv >> jQx", >> "a) X b) ${jQx} c) X"}, >> + /* Test shell variable assignments without substitutions */ >> + {"foo=bar echo baz", "baz"}, >> /* Test handling of shell variables. */ >> {"setenv jQx; for jQx in 1 2 3; do echo -n \"${jQx}, \"; done; echo;", >> "1, 2, 3, "}, >
On Sun, Feb 28, 2021 at 06:51:53PM -0500, Sean Anderson wrote: > On 2/28/21 6:40 PM, Heinrich Schuchardt wrote: > > Am 28. Februar 2021 22:29:51 MEZ schrieb Sean Anderson <seanga2@gmail.com>: > > > If there were no variable substitutions in a command, then initial > > > assignments would be misinterpreted as commands, instead of being > > > skipped > > > over. This is demonstrated by the following example: > > > > > > => foo=bar echo baz > > > > The commit message does not explain why this patch is needed. > > This is a bug I noticed while writing some tests of hush. > > > What shall be the value off foo after this line? > > It should be bar. This is an existing difference when compared with > bash. For example, without this patch, we have > > => foo=bar echo $foo > bar > => echo $foo > bar > > > > > What will be the output of > > > > foo=bar echo ${foo} > > > > with and without yor patch? > > It is the same. bash works as you describe. dash and busybox-sh both function like this: $ foo=bar echo $foo $ echo $foo $ That we error out entirely is different from everyone. Is that a good thing? Maybe. I know I've caught myself making thinkos due to that logic. It does also violate the principal of least surprise, that we don't act like anything else. But I would suggest the behavior of busybox-sh (what we forked long long ago) is what we should model here rather than be more bash-like. I'm not all that firm on this opinion frankly, especially given the one-line nature of the change to bring us that behavior and I assume dash/busybox are acting like pure sh would in this case, which we aren't anyhow.
On 3/1/21 3:17 PM, Tom Rini wrote: > On Sun, Feb 28, 2021 at 06:51:53PM -0500, Sean Anderson wrote: >> On 2/28/21 6:40 PM, Heinrich Schuchardt wrote: >>> Am 28. Februar 2021 22:29:51 MEZ schrieb Sean Anderson <seanga2@gmail.com>: >>>> If there were no variable substitutions in a command, then initial >>>> assignments would be misinterpreted as commands, instead of being >>>> skipped >>>> over. This is demonstrated by the following example: >>>> >>>> => foo=bar echo baz >>> >>> The commit message does not explain why this patch is needed. >> >> This is a bug I noticed while writing some tests of hush. >> >>> What shall be the value off foo after this line? >> >> It should be bar. This is an existing difference when compared with >> bash. For example, without this patch, we have >> >> => foo=bar echo $foo >> bar >> => echo $foo >> bar This seems really awkward. In bash I get: $ foo=bar ./test.sh bar $ echo $foo $ Where test.sh #!/bin/sh echo $foo I did not expect an assignment made before a command to stick. >> >>> >>> What will be the output of >>> >>> foo=bar echo ${foo} >>> >>> with and without your patch? >> >> It is the same. Please, provide an example where the patch makes a difference. Best regards Heinrich > > bash works as you describe. dash and busybox-sh both function like > this: > $ foo=bar echo $foo > > $ echo $foo > > $ > > That we error out entirely is different from everyone. Is that a good > thing? Maybe. I know I've caught myself making thinkos due to that > logic. It does also violate the principal of least surprise, that we > don't act like anything else. But I would suggest the behavior of > busybox-sh (what we forked long long ago) is what we should model here > rather than be more bash-like. I'm not all that firm on this opinion > frankly, especially given the one-line nature of the change to bring us > that behavior and I assume dash/busybox are acting like pure sh would in > this case, which we aren't anyhow. >
On Sun, Feb 28, 2021 at 04:29:51PM -0500, Sean Anderson wrote: > If there were no variable substitutions in a command, then initial > assignments would be misinterpreted as commands, instead of being skipped > over. This is demonstrated by the following example: > > => foo=bar echo baz > Unknown command 'foo=bar' - try 'help' > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > common/cli_hush.c | 2 +- > test/cmd/test_echo.c | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/common/cli_hush.c b/common/cli_hush.c > index b7f0f0ff41..1b9bef64b6 100644 > --- a/common/cli_hush.c > +++ b/common/cli_hush.c > @@ -1672,7 +1672,7 @@ static int run_pipe_real(struct pipe *pi) > return -1; > } > /* Process the command */ > - return cmd_process(flag, child->argc, child->argv, > + return cmd_process(flag, child->argc - i, child->argv + i, > &flag_repeat, NULL); > #endif > } > diff --git a/test/cmd/test_echo.c b/test/cmd/test_echo.c > index 4183cf75bb..13e1fb7c82 100644 > --- a/test/cmd/test_echo.c > +++ b/test/cmd/test_echo.c > @@ -33,6 +33,8 @@ static struct test_data echo_data[] = { > */ > {"setenv jQx X; echo \"a)\" ${jQx} 'b)' '${jQx}' c) ${jQx}; setenv jQx", > "a) X b) ${jQx} c) X"}, > + /* Test shell variable assignments without substitutions */ > + {"foo=bar echo baz", "baz"}, > /* Test handling of shell variables. */ > {"setenv jQx; for jQx in 1 2 3; do echo -n \"${jQx}, \"; done; echo;", > "1, 2, 3, "}, OK, I'm really confused now. With and without this patch, in sandbox I see: => foo=bar echo $foo bar => echo $foo bar Which doesn't match dash, busybox-sh nor bash 4.4.20, all of which do something like: $ foo=bar echo $foo $ echo $foo $
On 3/1/21 1:26 PM, Heinrich Schuchardt wrote: > On 3/1/21 3:17 PM, Tom Rini wrote: >> On Sun, Feb 28, 2021 at 06:51:53PM -0500, Sean Anderson wrote: >>> On 2/28/21 6:40 PM, Heinrich Schuchardt wrote: >>>> Am 28. Februar 2021 22:29:51 MEZ schrieb Sean Anderson <seanga2@gmail.com>: >>>>> If there were no variable substitutions in a command, then initial >>>>> assignments would be misinterpreted as commands, instead of being >>>>> skipped >>>>> over. This is demonstrated by the following example: >>>>> >>>>> => foo=bar echo baz >>>> >>>> The commit message does not explain why this patch is needed. >>> >>> This is a bug I noticed while writing some tests of hush. >>> >>>> What shall be the value off foo after this line? >>> >>> It should be bar. This is an existing difference when compared with >>> bash. For example, without this patch, we have >>> >>> => foo=bar echo $foo >>> bar >>> => echo $foo >>> bar > > This seems really awkward. In bash I get: > > $ foo=bar ./test.sh > bar > $ echo $foo > > $ > > Where test.sh > > #!/bin/sh > echo $foo > > I did not expect an assignment made before a command to stick. Yeah, this is because hush does not have the concept of per-command assignments (scope). So everything happens in the global scope. >>> >>>> >>>> What will be the output of >>>> >>>> foo=bar echo ${foo} >>>> >>>> with and without your patch? >>> >>> It is the same. > > Please, provide an example where the patch makes a difference. > > Best regards > > Heinrich > >> >> bash works as you describe. dash and busybox-sh both function like >> this: >> $ foo=bar echo $foo >> >> $ echo $foo >> >> $ >> >> That we error out entirely is different from everyone. Is that a good >> thing? Maybe. I know I've caught myself making thinkos due to that >> logic. It does also violate the principal of least surprise, that we >> don't act like anything else. But I would suggest the behavior of >> busybox-sh (what we forked long long ago) is what we should model here >> rather than be more bash-like. I'm not all that firm on this opinion >> frankly, especially given the one-line nature of the change to bring us >> that behavior and I assume dash/busybox are acting like pure sh would in >> this case, which we aren't anyhow. Ok, I'd like to clear things up. Here is the current behavior of U-Boot: => foo=bar echo $foo bar => echo $foo bar => baz=bar echo qux Unknown command 'baz=bar' - try 'help' with this patch, this changes to => foo=bar echo $foo bar => echo $foo bar => baz=bar echo qux qux This patch *only* affects cases where there is an assignment at the beginning of the line, but there is *no* variable reference in the command. I know this is an edge case, but the current logic is clearly wrong here. --Sean
On Mon, Mar 01, 2021 at 06:07:36PM -0500, Sean Anderson wrote: > On 3/1/21 1:26 PM, Heinrich Schuchardt wrote: > > On 3/1/21 3:17 PM, Tom Rini wrote: > > > On Sun, Feb 28, 2021 at 06:51:53PM -0500, Sean Anderson wrote: > > > > On 2/28/21 6:40 PM, Heinrich Schuchardt wrote: > > > > > Am 28. Februar 2021 22:29:51 MEZ schrieb Sean Anderson <seanga2@gmail.com>: > > > > > > If there were no variable substitutions in a command, then initial > > > > > > assignments would be misinterpreted as commands, instead of being > > > > > > skipped > > > > > > over. This is demonstrated by the following example: > > > > > > > > > > > > => foo=bar echo baz > > > > > > > > > > The commit message does not explain why this patch is needed. > > > > > > > > This is a bug I noticed while writing some tests of hush. > > > > > > > > > What shall be the value off foo after this line? > > > > > > > > It should be bar. This is an existing difference when compared with > > > > bash. For example, without this patch, we have > > > > > > > > => foo=bar echo $foo > > > > bar > > > > => echo $foo > > > > bar > > > > This seems really awkward. In bash I get: > > > > $ foo=bar ./test.sh > > bar > > $ echo $foo > > > > $ > > > > Where test.sh > > > > #!/bin/sh > > echo $foo > > > > I did not expect an assignment made before a command to stick. > > Yeah, this is because hush does not have the concept of per-command > assignments (scope). So everything happens in the global scope. > > > > > > > > > > > > > > > What will be the output of > > > > > > > > > > foo=bar echo ${foo} > > > > > > > > > > with and without your patch? > > > > > > > > It is the same. > > > > Please, provide an example where the patch makes a difference. > > > > Best regards > > > > Heinrich > > > > > > > > bash works as you describe. dash and busybox-sh both function like > > > this: > > > $ foo=bar echo $foo > > > > > > $ echo $foo > > > > > > $ > > > > > > That we error out entirely is different from everyone. Is that a good > > > thing? Maybe. I know I've caught myself making thinkos due to that > > > logic. It does also violate the principal of least surprise, that we > > > don't act like anything else. But I would suggest the behavior of > > > busybox-sh (what we forked long long ago) is what we should model here > > > rather than be more bash-like. I'm not all that firm on this opinion > > > frankly, especially given the one-line nature of the change to bring us > > > that behavior and I assume dash/busybox are acting like pure sh would in > > > this case, which we aren't anyhow. > > Ok, I'd like to clear things up. Here is the current behavior of U-Boot: > > > => foo=bar echo $foo > bar > => echo $foo > bar > => baz=bar echo qux > Unknown command 'baz=bar' - try 'help' I see this as well (which isn't what I said in another part of this thread, so I re-checked just now). > with this patch, this changes to > > => foo=bar echo $foo > bar > => echo $foo > bar > => baz=bar echo qux > qux > > This patch *only* affects cases where there is an assignment at the > beginning of the line, but there is *no* variable reference in the > command. I know this is an edge case, but the current logic is clearly > wrong here. OK. But I don't see that behavior in bash 4.4.20 (Ubuntu 18.04). Where does one get a shell that works like you're changing our hush to?
On 3/2/21 8:20 AM, Tom Rini wrote: > On Mon, Mar 01, 2021 at 06:07:36PM -0500, Sean Anderson wrote: >> On 3/1/21 1:26 PM, Heinrich Schuchardt wrote: >>> On 3/1/21 3:17 PM, Tom Rini wrote: >>>> On Sun, Feb 28, 2021 at 06:51:53PM -0500, Sean Anderson wrote: >>>>> On 2/28/21 6:40 PM, Heinrich Schuchardt wrote: >>>>>> Am 28. Februar 2021 22:29:51 MEZ schrieb Sean Anderson <seanga2@gmail.com>: >>>>>>> If there were no variable substitutions in a command, then initial >>>>>>> assignments would be misinterpreted as commands, instead of being >>>>>>> skipped >>>>>>> over. This is demonstrated by the following example: >>>>>>> >>>>>>> => foo=bar echo baz >>>>>> >>>>>> The commit message does not explain why this patch is needed. >>>>> >>>>> This is a bug I noticed while writing some tests of hush. >>>>> >>>>>> What shall be the value off foo after this line? >>>>> >>>>> It should be bar. This is an existing difference when compared with >>>>> bash. For example, without this patch, we have >>>>> >>>>> => foo=bar echo $foo >>>>> bar >>>>> => echo $foo >>>>> bar >>> >>> This seems really awkward. In bash I get: >>> >>> $ foo=bar ./test.sh >>> bar >>> $ echo $foo >>> >>> $ >>> >>> Where test.sh >>> >>> #!/bin/sh >>> echo $foo >>> >>> I did not expect an assignment made before a command to stick. >> >> Yeah, this is because hush does not have the concept of per-command >> assignments (scope). So everything happens in the global scope. >> >>>>> >>>>>> >>>>>> What will be the output of >>>>>> >>>>>> foo=bar echo ${foo} >>>>>> >>>>>> with and without your patch? >>>>> >>>>> It is the same. >>> >>> Please, provide an example where the patch makes a difference. >>> >>> Best regards >>> >>> Heinrich >>> >>>> >>>> bash works as you describe. dash and busybox-sh both function like >>>> this: >>>> $ foo=bar echo $foo >>>> >>>> $ echo $foo >>>> >>>> $ >>>> >>>> That we error out entirely is different from everyone. Is that a good >>>> thing? Maybe. I know I've caught myself making thinkos due to that >>>> logic. It does also violate the principal of least surprise, that we >>>> don't act like anything else. But I would suggest the behavior of >>>> busybox-sh (what we forked long long ago) is what we should model here >>>> rather than be more bash-like. I'm not all that firm on this opinion >>>> frankly, especially given the one-line nature of the change to bring us >>>> that behavior and I assume dash/busybox are acting like pure sh would in >>>> this case, which we aren't anyhow. >> >> Ok, I'd like to clear things up. Here is the current behavior of U-Boot: >> >> >> => foo=bar echo $foo >> bar >> => echo $foo >> bar >> => baz=bar echo qux >> Unknown command 'baz=bar' - try 'help' > > I see this as well (which isn't what I said in another part of this > thread, so I re-checked just now). > >> with this patch, this changes to >> >> => foo=bar echo $foo >> bar >> => echo $foo >> bar >> => baz=bar echo qux >> qux >> >> This patch *only* affects cases where there is an assignment at the >> beginning of the line, but there is *no* variable reference in the >> command. I know this is an edge case, but the current logic is clearly >> wrong here. > > OK. But I don't see that behavior in bash 4.4.20 (Ubuntu 18.04). Where > does one get a shell that works like you're changing our hush to? > I'm not changing hush to work like the first two examples, it's already like this. Only the third example is affected by this patch. --Sean
On Tue, Mar 02, 2021 at 08:24:20AM -0500, Sean Anderson wrote: > On 3/2/21 8:20 AM, Tom Rini wrote: > > On Mon, Mar 01, 2021 at 06:07:36PM -0500, Sean Anderson wrote: > > > On 3/1/21 1:26 PM, Heinrich Schuchardt wrote: > > > > On 3/1/21 3:17 PM, Tom Rini wrote: > > > > > On Sun, Feb 28, 2021 at 06:51:53PM -0500, Sean Anderson wrote: > > > > > > On 2/28/21 6:40 PM, Heinrich Schuchardt wrote: > > > > > > > Am 28. Februar 2021 22:29:51 MEZ schrieb Sean Anderson <seanga2@gmail.com>: > > > > > > > > If there were no variable substitutions in a command, then initial > > > > > > > > assignments would be misinterpreted as commands, instead of being > > > > > > > > skipped > > > > > > > > over. This is demonstrated by the following example: > > > > > > > > > > > > > > > > => foo=bar echo baz > > > > > > > > > > > > > > The commit message does not explain why this patch is needed. > > > > > > > > > > > > This is a bug I noticed while writing some tests of hush. > > > > > > > > > > > > > What shall be the value off foo after this line? > > > > > > > > > > > > It should be bar. This is an existing difference when compared with > > > > > > bash. For example, without this patch, we have > > > > > > > > > > > > => foo=bar echo $foo > > > > > > bar > > > > > > => echo $foo > > > > > > bar > > > > > > > > This seems really awkward. In bash I get: > > > > > > > > $ foo=bar ./test.sh > > > > bar > > > > $ echo $foo > > > > > > > > $ > > > > > > > > Where test.sh > > > > > > > > #!/bin/sh > > > > echo $foo > > > > > > > > I did not expect an assignment made before a command to stick. > > > > > > Yeah, this is because hush does not have the concept of per-command > > > assignments (scope). So everything happens in the global scope. > > > > > > > > > > > > > > > > > > > > > > > What will be the output of > > > > > > > > > > > > > > foo=bar echo ${foo} > > > > > > > > > > > > > > with and without your patch? > > > > > > > > > > > > It is the same. > > > > > > > > Please, provide an example where the patch makes a difference. > > > > > > > > Best regards > > > > > > > > Heinrich > > > > > > > > > > > > > > bash works as you describe. dash and busybox-sh both function like > > > > > this: > > > > > $ foo=bar echo $foo > > > > > > > > > > $ echo $foo > > > > > > > > > > $ > > > > > > > > > > That we error out entirely is different from everyone. Is that a good > > > > > thing? Maybe. I know I've caught myself making thinkos due to that > > > > > logic. It does also violate the principal of least surprise, that we > > > > > don't act like anything else. But I would suggest the behavior of > > > > > busybox-sh (what we forked long long ago) is what we should model here > > > > > rather than be more bash-like. I'm not all that firm on this opinion > > > > > frankly, especially given the one-line nature of the change to bring us > > > > > that behavior and I assume dash/busybox are acting like pure sh would in > > > > > this case, which we aren't anyhow. > > > > > > Ok, I'd like to clear things up. Here is the current behavior of U-Boot: > > > > > > > > > => foo=bar echo $foo > > > bar > > > => echo $foo > > > bar > > > => baz=bar echo qux > > > Unknown command 'baz=bar' - try 'help' > > > > I see this as well (which isn't what I said in another part of this > > thread, so I re-checked just now). > > > > > with this patch, this changes to > > > > > > => foo=bar echo $foo > > > bar > > > => echo $foo > > > bar > > > => baz=bar echo qux > > > qux > > > > > > This patch *only* affects cases where there is an assignment at the > > > beginning of the line, but there is *no* variable reference in the > > > command. I know this is an edge case, but the current logic is clearly > > > wrong here. > > > > OK. But I don't see that behavior in bash 4.4.20 (Ubuntu 18.04). Where > > does one get a shell that works like you're changing our hush to? > > > > I'm not changing hush to work like the first two examples, it's already like this. > > Only the third example is affected by this patch. OK. But to what end? Historically we have a buggy but mostly compatible "hush" that acts like "sh" does. A more flexible shell could solve a lot of different use cases including making boot scripts that people end up writing being clearer and easier to write/debug/maintain. What I worry about here is making our shell not act like any regular shell people use.
On 3/2/21 8:34 AM, Tom Rini wrote: > On Tue, Mar 02, 2021 at 08:24:20AM -0500, Sean Anderson wrote: >> On 3/2/21 8:20 AM, Tom Rini wrote: >>> On Mon, Mar 01, 2021 at 06:07:36PM -0500, Sean Anderson wrote: >>>> On 3/1/21 1:26 PM, Heinrich Schuchardt wrote: >>>>> On 3/1/21 3:17 PM, Tom Rini wrote: >>>>>> On Sun, Feb 28, 2021 at 06:51:53PM -0500, Sean Anderson wrote: >>>>>>> On 2/28/21 6:40 PM, Heinrich Schuchardt wrote: >>>>>>>> Am 28. Februar 2021 22:29:51 MEZ schrieb Sean Anderson <seanga2@gmail.com>: >>>>>>>>> If there were no variable substitutions in a command, then initial >>>>>>>>> assignments would be misinterpreted as commands, instead of being >>>>>>>>> skipped >>>>>>>>> over. This is demonstrated by the following example: >>>>>>>>> >>>>>>>>> => foo=bar echo baz >>>>>>>> >>>>>>>> The commit message does not explain why this patch is needed. >>>>>>> >>>>>>> This is a bug I noticed while writing some tests of hush. >>>>>>> >>>>>>>> What shall be the value off foo after this line? >>>>>>> >>>>>>> It should be bar. This is an existing difference when compared with >>>>>>> bash. For example, without this patch, we have >>>>>>> >>>>>>> => foo=bar echo $foo >>>>>>> bar >>>>>>> => echo $foo >>>>>>> bar >>>>> >>>>> This seems really awkward. In bash I get: >>>>> >>>>> $ foo=bar ./test.sh >>>>> bar >>>>> $ echo $foo >>>>> >>>>> $ >>>>> >>>>> Where test.sh >>>>> >>>>> #!/bin/sh >>>>> echo $foo >>>>> >>>>> I did not expect an assignment made before a command to stick. >>>> >>>> Yeah, this is because hush does not have the concept of per-command >>>> assignments (scope). So everything happens in the global scope. >>>> >>>>>>> >>>>>>>> >>>>>>>> What will be the output of >>>>>>>> >>>>>>>> foo=bar echo ${foo} >>>>>>>> >>>>>>>> with and without your patch? >>>>>>> >>>>>>> It is the same. >>>>> >>>>> Please, provide an example where the patch makes a difference. >>>>> >>>>> Best regards >>>>> >>>>> Heinrich >>>>> >>>>>> >>>>>> bash works as you describe. dash and busybox-sh both function like >>>>>> this: >>>>>> $ foo=bar echo $foo >>>>>> >>>>>> $ echo $foo >>>>>> >>>>>> $ >>>>>> >>>>>> That we error out entirely is different from everyone. Is that a good >>>>>> thing? Maybe. I know I've caught myself making thinkos due to that >>>>>> logic. It does also violate the principal of least surprise, that we >>>>>> don't act like anything else. But I would suggest the behavior of >>>>>> busybox-sh (what we forked long long ago) is what we should model here >>>>>> rather than be more bash-like. I'm not all that firm on this opinion >>>>>> frankly, especially given the one-line nature of the change to bring us >>>>>> that behavior and I assume dash/busybox are acting like pure sh would in >>>>>> this case, which we aren't anyhow. >>>> >>>> Ok, I'd like to clear things up. Here is the current behavior of U-Boot: >>>> >>>> >>>> => foo=bar echo $foo >>>> bar >>>> => echo $foo >>>> bar >>>> => baz=bar echo qux >>>> Unknown command 'baz=bar' - try 'help' >>> >>> I see this as well (which isn't what I said in another part of this >>> thread, so I re-checked just now). >>> >>>> with this patch, this changes to >>>> >>>> => foo=bar echo $foo >>>> bar >>>> => echo $foo >>>> bar >>>> => baz=bar echo qux >>>> qux >>>> >>>> This patch *only* affects cases where there is an assignment at the >>>> beginning of the line, but there is *no* variable reference in the >>>> command. I know this is an edge case, but the current logic is clearly >>>> wrong here. >>> >>> OK. But I don't see that behavior in bash 4.4.20 (Ubuntu 18.04). Where >>> does one get a shell that works like you're changing our hush to? >>> >> >> I'm not changing hush to work like the first two examples, it's already like this. >> >> Only the third example is affected by this patch. > > OK. But to what end? Historically we have a buggy but mostly > compatible "hush" that acts like "sh" does. A more flexible shell could > solve a lot of different use cases including making boot scripts that > people end up writing being clearer and easier to write/debug/maintain. > What I worry about here is making our shell not act like any regular > shell people use. > Good news: this patch brings hush *more* in-line with other shells. dash: $ foo=bar echo qux qux sh: sh-5.1$ foo=bar echo qux qux bash: $ foo=bar echo qux qux U-Boot without this patch: => foo=bar echo qux Unknown command 'foo=bar' - try 'help' U-Boot with this patch: => foo=bar echo qux qux --Sean
Dear Tom, In message <20210302133435.GZ10169@bill-the-cat> you wrote: > > > Only the third example is affected by this patch. > > OK. But to what end? Historically we have a buggy but mostly > compatible "hush" that acts like "sh" does. A more flexible shell could > solve a lot of different use cases including making boot scripts that > people end up writing being clearer and easier to write/debug/maintain. > What I worry about here is making our shell not act like any regular > shell people use. I am always surprised why people try to add minor fixes (and sometimes bells and whistles) to ancient versions of the software without checking recent code first. The shell in busybox v1.32 shows this behaviour: $ foo=bar echo $foo $ echo $foo $ baz=bar echo qux qux $ So the specific problem has (long) been fixed in upstream, and instead of adding a patch to our old version, thus cementing the broken behaviour, we should upgrade hush to recent upstream code. Yes, I am aware that this is a lot more effort that this little patch, but if we could combine the efforts that have already been wasted over the years for such little fixes here and there wew could have upgraded long ago. Grooming a dead horse makes little sense to me. Best regards, Wolfgang Denk
On Sun, Feb 28, 2021 at 04:29:51PM -0500, Sean Anderson wrote: > If there were no variable substitutions in a command, then initial > assignments would be misinterpreted as commands, instead of being skipped > over. This is demonstrated by the following example: > > => foo=bar echo baz > Unknown command 'foo=bar' - try 'help' > > Signed-off-by: Sean Anderson <seanga2@gmail.com> Applied to u-boot/master, thanks!
diff --git a/common/cli_hush.c b/common/cli_hush.c index b7f0f0ff41..1b9bef64b6 100644 --- a/common/cli_hush.c +++ b/common/cli_hush.c @@ -1672,7 +1672,7 @@ static int run_pipe_real(struct pipe *pi) return -1; } /* Process the command */ - return cmd_process(flag, child->argc, child->argv, + return cmd_process(flag, child->argc - i, child->argv + i, &flag_repeat, NULL); #endif } diff --git a/test/cmd/test_echo.c b/test/cmd/test_echo.c index 4183cf75bb..13e1fb7c82 100644 --- a/test/cmd/test_echo.c +++ b/test/cmd/test_echo.c @@ -33,6 +33,8 @@ static struct test_data echo_data[] = { */ {"setenv jQx X; echo \"a)\" ${jQx} 'b)' '${jQx}' c) ${jQx}; setenv jQx", "a) X b) ${jQx} c) X"}, + /* Test shell variable assignments without substitutions */ + {"foo=bar echo baz", "baz"}, /* Test handling of shell variables. */ {"setenv jQx; for jQx in 1 2 3; do echo -n \"${jQx}, \"; done; echo;", "1, 2, 3, "},
If there were no variable substitutions in a command, then initial assignments would be misinterpreted as commands, instead of being skipped over. This is demonstrated by the following example: => foo=bar echo baz Unknown command 'foo=bar' - try 'help' Signed-off-by: Sean Anderson <seanga2@gmail.com> --- common/cli_hush.c | 2 +- test/cmd/test_echo.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)