[PATCH v4] shell: exchange Dijkstra $(( )) evaluator..

[Harald van Dijk]

On 06/09/2022 20:39, Steffen Nurpmeso wrote:
>   |What was the initial expression that did not work properly with the old
>   |implementation?
> 
> Well if you run the test of the patchset against busybox with the
> old implementation you will see multiple errors (of course >>>[=]
> and **= because they are not supported), mostly ?: related.  To
> make it plain, ?: is broken but for simple cases.

Relying on readers to run your test suite that contains lots of tests 
for functionality that busybox does not currently aim to implement, and 
sifting through the results to find out what is actually a bug in 
busybox as opposed to a missing feature. So here are two concrete bugs 
where busybox ash does not implement what POSIX requires:

#1:

In $(( a ? b : c )), busybox ash parses b incorrectly. This is a bug 
when b has an unparenthesised assignment operator. Test case:

   busybox ash -c 'echo $(( 1 ? a=1 : 0 )) $a'

This is a bug shared with osh and zsh. This is supposed to parse exactly 
the same way as $(( 1 ? (a=1) : 0 )) and set a to 1, and print 1 1. It 
currently results in an error "malformed ?: operator".

#2:

In $(( a ? b : c )), busybox ash evaluates both b and c. This is a bug 
when b or c have side effects, as only one or the other should be evaluated.

   busybox ash -c 'echo $(( 1 ? (a=1) : (a=2) )) $a'

This is a bug unique to busybox, as far as I can tell, and should print 
1 1. It currently prints 1 2.

If busybox has more bugs in its handling of ?:, whether in standard 
POSIX functionality or in bash extensions that are meant to be supported 
in busybox too, I would encourage you to write them up similarly to make 
it easier for everybody else here to see what is going on.

Note that #2 is not an inherent consequence of parsing ? and : as binary 
operators: bosh also does this, allowing $(( a ? (b : c) )), but still 
gets the parse and the evaluation right and handles #2 correctly. It 
does, however, have a bug in its handling in #1, different from what 
busybox/osh/zsh do, and it is possible that fixing #1 requires a 
redesign anyway.

The supplied patch appears to fix both #1 and #2, but it has alignment 
issues: when UBSAN is enabled, I see a lot of "runtime error: store to 
misaligned address <...> for type 'char *', which requires 4 byte 
alignment" warnings when using shell arithmetic.

Cheers,
Harald van Dijk