Re: Consolidate recommended options?

[Chet Ramey]

On 12/31/24 6:30 PM, Greg Wooledge wrote:
> On Tue, Dec 31, 2024 at 12:15:27 -0500, Chet Ramey wrote:
> > On 12/29/24 4:34 PM, Lawrence Velázquez wrote:
> > > I'm aware of the pitfalls described therein, and more.  I don't
> > > think anyone thinks multiple evaluation in indexes is good per se,
> > > but many find the side effects of the attempted cures worse than
> > > the disease (e.g., https://mywiki.wooledge.org/BashPitfalls#pf62).
> >
> > In its zeal to demonstrate how bad bash is, that little code snippet
> > ignores the obvious solution:
> >
> > declare -A hash
> > key=\'\]
> > hash[$key]=17
> > (( hash[key]++ ))
> > declare -p hash
> >
> > Since arithmetic evaluation performs its own expansion of identifiers.
> > assoc_expand_once also works. But that wasn't really the point, was it?
>
> What's obvious to you is not obvious to the rest of us.  Also, the main
> point of the BashPitfalls page isn't just to show how "bad" bash is; it's
> to tell users how to safely and correctly do the thing they want to do.

I think we have different goals here. You want to show users how to cope
with the (incorrect) behavior in bash versions before 5.2. I want to
figure out what the right behavior is and make bash do that. I'm willing
to make incompatible changes because of how bitterly people have complained
about the historical bash behavior.

These aren't perfectly aligned, but they're not necessarily incompatible.

There was a long discussion about this and what ended up being in bash-5.2
in March and April, 2021, starting at

https://lists.gnu.org/archive/html/bug-bash/2021-03/msg00056.html

The upshot is that bash-5.2 does the right thing, with or without
assoc_expand_once, in all of these test cases. But since it no longer
needs the workarounds that were required to avoid double expansion of
subscripts in previous versions, those workarounds do not work as they
did before.

The bash-5.1 behavior is available if you set BASH_COMPAT=51, so the
workarounds will act as they did previously. I obviously don't recommend
that.

> hobbit:~$ cat x61
> declare -A hash
> key=\'\]
> hash[$key]=foo
> if [[ -v hash[$key] ]]; then echo yes; else echo no; fi
>
> It should say "yes".

This works in bash-5.2, with or without assoc_expand_once. That was kind
of the point of the changes.


> hobbit:~$ cat x61
> declare -A hash
> key=\'\]
> hash[$key]=foo
> if [[ -v 'hash[$key]' ]]; then echo yes; else echo no; fi

Same.

> Now, the second failure mode: if key contains a command substitution,
> the evaluation may execute it.
>
> hobbit:~$ cat x61
> declare -A hash
> key='x$(date >&2)'
> hash[$key]=foo
> if [[ -v hash[$key] ]]; then echo yes; else echo no; fi

This works in bash-5.2, with or without assoc_expand_once. This was the
other thing the changes intended to address.


> With the single quotes:
>
> hobbit:~$ cat x61
> declare -A hash
> key='x$(date >&2)'
> hash[$key]=foo
> if [[ -v 'hash[$key]' ]]; then echo yes; else echo no; fi

> hobbit:~$ for v in 4.0 4.1 4.2 4.3 4.4 5.0 5.1 5.2; do printf '%s: ' "$v"; bash-"$v" x61; done; for 
> v in 5.0 5.1 5.2; do printf '%s with shopt: ' "$v"; bash-"$v" y61; done
> 4.0: x61: line 4: conditional binary operator expected
> x61: line 4: syntax error near `'hash[$key]''
> x61: line 4: `if [[ -v 'hash[$key]' ]]; then echo yes; else echo no; fi'
> 4.1: x61: line 4: conditional binary operator expected
> x61: line 4: syntax error near `'hash[$key]''
> x61: line 4: `if [[ -v 'hash[$key]' ]]; then echo yes; else echo no; fi'
> 4.2: no
> 4.3: yes
> 4.4: yes
> 5.0: yes
> 5.1: yes
> 5.2: yes
> 5.0 with shopt: yes
> 5.1 with shopt: no
> 5.2 with shopt: yes
>
> The single quotes successfully stop the code injection in all versions,
> which is good.
>
> The conclusion here is that this feature simply doesn't exist before
> version 4.2, has no known way to make it work correctly in 4.2, can be
> made to work correctly with a single-quote workaround in versions 4.3
> to 5.1 (as long as assoc_expand_once is NOT set), and works out of the
> box only in 5.2.

Kind of. I had hoped using an option that was off by default would have
been a less intrusive way to improve the behavior, but finally concluded
that correct behavior was more important. The issue in bash-5.1 was the
result of an unrelated change making the (now not-needed) workaround
unnecessary. The test is for ${hash[$key]}.

> Next, we have pitfall 62: (( hash[$key]++ ))
>
> hobbit:~$ cat x62
> declare -A hash
> key=\'\]
> hash[$key]=3
> (( hash[$key]++ ))
> printf '%s\n' "${hash[$key]}"
>
> This one should print "4". 

This works in bash-5.2 with or without assoc_expand_once.


> The single-quote workaround from pitfall 61 only works in some versions:

Yes. If you make the workaround unnecessary, a script that defeats those
changes won't produce the same results as in previous versions.

> This gives the right answer in 4.0 through 5.0, gives the wrong answer with
> NO error message in 5.0 with shopt, and gives the wrong answer with an
> error message in 5.1 and 5.2 (regardless of shopt).

Again.

> The second known workaround is a backslash:
>
> hobbit:~$ cat x62
> declare -A hash
> key=\'\]
> hash[$key]=3
> (( hash[\$key]++ ))
> printf '%s\n' "${hash[$key]}"
>
> hobbit:~$ for v in 4.0 4.1 4.2 4.3 4.4 5.0 5.1 5.2; do printf '%s: ' "$v"; bash-"$v" x62; done; for 
> v in 5.0 5.1 5.2; do printf '%s with shopt: ' "$v"; bash-"$v" y62; done
> 4.0: 4
> 4.1: 4
> 4.2: 4
> 4.3: 4
> 4.4: 4
> 5.0: 4
> 5.1: 4
> 5.2: 3
> 5.0 with shopt: 3
> 5.1 with shopt: 3
> 5.2 with shopt: 3
>
> This one gives the right answer in 4.0 through 5.1 (without shopt),
> gives the wrong answer in 5.0 and 5.1 with shopt, and gives the wrong
> answer in 5.2 regardless of the shopt.

I know you understand that you're changing the key you're testing if you
don't have the double expansion behavior.

> Pitfall 62 gives a few workarounds which are purported to work across
> all versions.  The first is using a temporary string variable:


> The third is to use "let" with a single-quoted expression instead of "((":

Those are not equivalent, of course.

> hobbit:~$ cat x62
> declare -A hash
> key=\'\]
> hash[$key]=3
> let 'hash[$key]++'
> printf '%s\n' "${hash[$key]}"
>
> hobbit:~$ for v in 4.0 4.1 4.2 4.3 4.4 5.0 5.1 5.2; do printf '%s: ' "$v"; bash-"$v" x62; done; for 
> v in 5.0 5.1 5.2; do printf '%s with shopt: ' "$v"; bash-"$v" y62; done
> 4.0: 4
> 4.1: 4
> 4.2: 4
> 4.3: 4
> 4.4: 4
> 5.0: 4
> 5.1: 4
> 5.2: 4
> 5.0 with shopt: 3
> 5.1 with shopt: 3
> 5.2 with shopt: 3
>
> (Note: gotta mention that the shopt breaks this one!)

I'm actually torn about this one. `let' is a builtin, not a compound
command, so its arguments undergo the standard simple command word
expansions. This is unchangeable.

Since the index that's actually evaluated is `$key', assoc_expand_once
forces `let' to increment the element with key '$key'. This would be
clearer if you displayed the contents of the array with `declare -p'.

Leaving assoc_expand_once unset allows the array expansion to
expand $key to "']" and you get different results. Now, I think that's
probably wrong, and now that I think about it, I should change it to
be consistent with other builtins. If bash-5.2 is going to act like
assoc_expand_once is set in most contexts, it should have done so here.
But still, you don't need this workaround.

> Now, Chet has proposed a fifth one:
>
> hobbit:~$ cat x62
> declare -A hash
> key=\'\]
> hash[$key]=3
> (( hash[key]++ ))
> printf '%s\n' "${hash[$key]}"

No, that was my mistake. I pasted the wrong script into my message. I
thought I said that in a message to the list, but I can't find it now.

Chet
--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    chet@case.edu    http://tiswww.cwru.edu/~chet/

OpenPGP_signature.asc
Description: OpenPGP digital signature