Why code review is hard

[David Hobach]

Dear all,

just stumbled across it and was wondering what a reviewer would expect from this code to do:

```
#!/bin/bash

function badCode {
echo "bad code executed"
}

function testCode {
#pick some existing file, nonexisting works too though
echo "/etc/passwd"
}

function tfunc {
local foo=
foo="$(testCode)" || {echo "foo";}
cat "$foo" || {
badCode
case $? in
*)
exit 1
esac
}
}

echo "Finished."
```

At least on my amchine it executes "badCode" in both domU and dom0.
I guess it's a bash bug and reported it accordingly, but anyway...

Have a nice weekend & BR
David

[Brendan Hoar]

I’m not exactly sure what’s going on (and can’t bash right now) but output grouping requires white space around { and } so that’s at least one problem on the foo= assignment line. Maybe it’s being interpreted as closing the function definition early?

B

[HW42]

Brendan Hoar:

Yes you are on the right track here, I think. One problem is the missing
space after the opening {

$ false || {echo "foo";
bash: {echo: command not found
$ false || {echo "foo";}
bash: syntax error near unexpected token `}'
$

So the } after the "foo"; closes the function. But why don't we get an
error for the later } that don't match? Because bash operates line by
line (remember those self extracting archives, that are shell scripts
followed by a tar). If you change the *) into, say, x) (or anything that
doesn't match $?) you see it:

$ ./t2
cat: '': No such file or directory
bad code executed
./t2: line 22: syntax error near unexpected token `}'
./t2: line 22: `}'
$

So this is probably not even a bug. Thanks for the nice example David
(apropos shell: set -e semantics are also "fun").

Simon

[Brendan Hoar]

 On Sat, Feb 12, 2022 at 11:31 AM HW42 <hw...@ipsumj.de> wrote:

So this is probably not even a bug. Thanks for the nice example David
(apropos shell: set -e semantics are also "fun").

Simon

I’m going to guess (again, away from Linux terminal right now) that the shellcheck command would probably flag this…

…and that’s part of why the Qubes CI pipeline utilizes shellcheck.

B

[David Hobach]

On 2/12/22 17:52, Brendan Hoar wrote:
> On Sat, Feb 12, 2022 at 11:31 AM HW42 <hw...@ipsumj.de> wrote:
>
>> So this is probably not even a bug. Thanks for the nice example David
>> (apropos shell: set -e semantics are also "fun").
>>
>> Simon
>
>
> I’m going to guess (again, away from Linux terminal right now) that the
> shellcheck command would probably flag this…

That's true unless you do something more obvious such as

```
#!/bin/bash

function badCode {
echo "bad code executed"
}

function testCode {
#pick some existing file

echo "/etc/passwd"
}

function tfunc {
local foo=
foo="$(testCode)" || "{echo" "foo";}
cat "$foo" || {
badCode
case $? in
*)
exit 1
esac
}

echo "Finished."
```

> …and that’s part of why the Qubes CI pipeline utilizes shellcheck.

Definitely a good thing.

Btw I now believe that the {echo in the original example is considered a string and the second } closes the function. I just don't understand why the case/esac can be used to remove the syntax error caused by the double } }. But oh well...

[HW42]

David Hobach:
[...]

> Btw I now believe that the {echo in the original example is considered
> a string and the second } closes the function.

Yes.

> I just don't understand why the case/esac can be used to remove the
> syntax error caused by the double } }. But oh well...

See my previous mail. It's a syntax error. But the first case matches
and after the 'exit 1' bash terminates and never parses the following
lines.

[Demi Marie Obenour]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

I consider this a good reason to not use shell scripts for anything
non-trivial. There are some scripts in the builder which definitely
violate this rule. Also, `bash -n` will catch this.

- --
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEdodNnxM2uiJZBxxxsoi1X/+cIsEFAmIIIgoACgkQsoi1X/+c
IsGQbQ//VtA3XGI5Cqcpa9sBSsChICtoHKq7m/qz2Fu3D/CDyAs0sNVrtq+ujLD8
1Mk/di/nfUzv7aBM6DBJoaaXMsmNpgQcx41aDM/AA7W+2MGKSPwVlcYYbxO7VsST
nQWEv3me4JRevvWNrQQUcPdkzV/alFSehKWfR05oHEAb7QMSIpwQs19YEprfRWV4
i6Dv1PTVSARTYry42Do9NT7lUi7YF786JpqV9H3/vkJOhf4IWm7Hb8CBasYykF1A
h6B2gx66PoiwK+qnJJesxoM8ePGhpvCjCw272FNy2TGuxeDa/zUYzWgkCgQNeUI/
dyEqmbDT410n4wDZtsHVvQLvoNYPGYjCMH0KoX95QJt/12ufzewPqaOTh2GvHr0w
hcIETVL02D5fgC0Rgt23SpB0aGJVQ13NM9nTi7Vy3kyhYv2N8/j5pQQpYszlLC2I
QOLmFDbPnaCvRU8BrhA4oQ6jaAZJNrKUtysOuQz3RJnXKV5kwNla9ys7b2ykCdJ5
oFWhxYgwk0HKuaFwpAbyJ+pXsb2Nq8rHYZrnzH4JKDUYX95SCCHcv4tSs+zcUpnW
D1a+PvR6NUSQUbncbt0eJq9UCyCioA2KlwZzfVf7iLdmdJvICB/kIszeDwt4BBQO
Lpjx1vFj4f+Db4zHi3ATOx9JazVxS6Lm7sCKgSnJcxUtzx9DGUI=
=iCAe
-----END PGP SIGNATURE-----

[Holger Levsen]

On Sat, Feb 12, 2022 at 01:03:35PM +0100, David Hobach wrote:
> just stumbled across it and was wondering what a reviewer would expect from this code to do:

[...]

> At least on my amchine it executes "badCode" in both domU and dom0.

I might miss where you stumbled upon this, but how is this related
to qubes? (and why should this code run differently in domU and dom0?)

> I guess it's a bash bug and reported it accordingly, but anyway...

where did you report it?


--
cheers,
Holger

⢀⣴⠾⠻⢶⣦⠀
⣾⠁⢠⠒⠀⣿⡁ holger@(debian|reproducible-builds|layer-acht).org
⢿⡄⠘⠷⠚⠋⠀ OpenPGP: B8BF54137B09D35CF026FE9D 091AB856069AAA1C
⠈⠳⣄

The system isn't broken. It was built this way.

[Frédéric Pierret]

Hi,

Le 2/13/22 à 10:05, Holger Levsen a écrit :

> On Sat, Feb 12, 2022 at 01:03:35PM +0100, David Hobach wrote:
>> just stumbled across it and was wondering what a reviewer would expect from this code to do:
> [...]
>> At least on my amchine it executes "badCode" in both domU and dom0.
>
> I might miss where you stumbled upon this, but how is this related
> to qubes? (and why should this code run differently in domU and dom0?)

Holger, you are right, I don't see at all how it related to Qubes OS as it looks like simply to be a BASH coding issue.

David, As Brendan said, a spell checker like ShellCheck would have spot it to you. I've just copy pasted your piece of code and:

```
$ shellcheck badcode.sh

In badcode.sh line 14:
foo="$(testCode)" || {echo "foo";}
^-- SC1054: You need a space after the '{'.

For more information:
https://www.shellcheck.net/wiki/SC1054 -- You need a space after the '{'.
```

I personally like BASH very much but it can be exhausting sometimes. I admit it without any shame and I don't hesitate to massively use ShellCheck to help me in spotting coding errors I could make or even learn new things sometimes :).

Best,
Frédéric

[David Hobach]

On 2/13/22 10:05, Holger Levsen wrote:
> On Sat, Feb 12, 2022 at 01:03:35PM +0100, David Hobach wrote:
>> just stumbled across it and was wondering what a reviewer would expect from this code to do:
> [...]
>> At least on my amchine it executes "badCode" in both domU and dom0.
>
> I might miss where you stumbled upon this, but how is this related
> to qubes? (and why should this code run differently in domU and dom0?)

I never stated it is. It was just a general comment on code review and bash in specific - especially since I'm aware that Qubes has a lot of bash in sometimes security relevant places (qubes-dom0-update, qubes-rpc, ...).
I fully agree with Demi's conclusion even though I often don't follow it myself. Mostly I attempt to outsource more complex stuff to well-tested library code.

>> I guess it's a bash bug and reported it accordingly, but anyway...
>
> where did you report it?

At bug-bash, but they bashed me [1]. ^^
Apparently it's considered regular syntax = no bug. You're simply allowed to use { in literals, var names, strings etc. as HW42 already correctly stated.

[1] https://lists.gnu.org/archive/html/bug-bash/2022-02/msg00120.html

[Holger Levsen]

On Sun, Feb 13, 2022 at 11:52:53AM +0100, David Hobach wrote:
> I never stated it is. It was just a general comment on code review and
> bash in specific - especially since I'm aware that Qubes has a lot of
> bash in sometimes security relevant places (qubes-dom0-update, qubes-rpc, ...).

ah, makes sense now, thanks.

> > where did you report it?
> At bug-bash, but they bashed me [1]. ^^

heh.

--
cheers,
Holger

⢀⣴⠾⠻⢶⣦⠀
⣾⠁⢠⠒⠀⣿⡁ holger@(debian|reproducible-builds|layer-acht).org
⢿⡄⠘⠷⠚⠋⠀ OpenPGP: B8BF54137B09D35CF026FE9D 091AB856069AAA1C
⠈⠳⣄

All data, over time, approaches deleted, or public. (@quinnnorton)