[PATCH] hush: Fix assignments being misinterpreted as commands

[PATCH] hush: Fix assignments being misinterpreted as commands

[Sean Anderson]

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, "},
-- 
2.30.1

[PATCH] hush: Fix assignments being misinterpreted as commands

[Heinrich Schuchardt]

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, "},

[PATCH] hush: Fix assignments being misinterpreted as commands

[Sean Anderson]

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, "},
> 

[PATCH] hush: Fix assignments being misinterpreted as commands

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210301/c126f507/attachment.sig>

[PATCH] hush: Fix assignments being misinterpreted as commands

[Heinrich Schuchardt]

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.
>

[PATCH] hush: Fix assignments being misinterpreted as commands

[Sean Anderson]

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

[PATCH] hush: Fix assignments being misinterpreted as commands

[Tom Rini]

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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210302/c9fc4641/attachment.sig>

[PATCH] hush: Fix assignments being misinterpreted as commands

[Sean Anderson]

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

[PATCH] hush: Fix assignments being misinterpreted as commands

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210302/277c60b0/attachment.sig>

[PATCH] hush: Fix assignments being misinterpreted as commands

[Sean Anderson]

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

[PATCH] hush: Fix assignments being misinterpreted as commands

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What is research but a blind date with knowledge?      -- Will Harvey

[PATCH] hush: Fix assignments being misinterpreted as commands

[Tom Rini]

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

$

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210301/a57b77f6/attachment.sig>

[PATCH] hush: Fix assignments being misinterpreted as commands

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210413/7c3f2795/attachment.sig>