Re: [PATCH] devel: fix segfault by unset 'assoc[${x[0]}]'

[Chet Ramey]

On 10/5/21 7:05 AM, Koichi Murase wrote:

>   The segmentation fault is fixed by the above patch, but there
>   still remains the same error as bash 4.4.
> 
>     bash-patch1: ${x[0: bad substitution
> 
>   This is caused by an inconsistency between `valid_array_reference
>   (name,flags)' (arrayfunc.c:1187) and `unbind_array_element
>   (var,sub,flags)' (arrayfunc.c:1033) in the extraction of
>   associative-array subscripts.  Note that `valid_array_reference' is
>   called from `unset_builtin' (builtins/set.def:834) to check if the
>   unset name has the form of an array element.  Also,
>   `unbind_array_element' is called from `unset_builtin' to perform the
>   actual unset.  In `valid_array_reference', the length of the
>   associative-array subscripts are determined as
> 
>       else if (isassoc)
>         len = skipsubscript (t, 0, flags&VA_NOEXPAND);  /* VA_NOEXPAND
> must be 1 */

The difference is that valid_array_reference can be called before any of
the subscript is expanded, in which case you need to parse things that
can be expanded, where unbind_array_element is called after all the
expansions are performed (but see below).

So let's see if we can talk through this.

> 
>   whereas in `unbind_array_element', the length is determined as
> 
>       if (var && assoc_p (var) && (flags&VA_ONEWORD))
>         len = strlen (sub) - 1;
>       else
>         len = skipsubscript (sub, 0, (flags&VA_NOEXPAND) || (var &&
> assoc_p(var)));  /* XXX */
> 
>   `skipsubscript' does not consider the nesting of ${} and $() when
>   bit 1 is set to the third argument.  In the former code, nesting is
>   not considered only when VA_NOEXPAND is specified.  However, in the
>   latter code, nesting is never considered for associative arrays
>   (even when VA_NOEXPAND is not specified).  I believe the former code
>   should be the expected one.

You're right, there should be no `nesting' considered at all. By the time
unbind_array_element is called, since it's only called from unset_builtin,
the word and subscript should have already undergone all the expansion
they're going to. There should be no need to interpret ${} or $() in the
subscript: since associative arrays can have arbitrary subscripts, you
should not attempt to parse the subscript contents. That was obviously one
of the problems with the code through bash-5.1, one of the original reasons
I introduced `assoc_expand_once', and an issue that's still looking for a
final resolution.

However, there is backwards compatibility to consider, which is why
assoc_expand_once isn't set by default and the code attempts to run the
subscript through word expansion.

In this example, the quoting prevents the shell from recognizing the word
as an array reference before the quote removal occurs and the word gets
passed to the unset builtin, so it can't set any hints for unset. unset
sees `a[${b[0]}]', which is ambiguous.

It can shortcut and say "ok, if it passes valid_array_reference, we should
just consider the entire argument as one word as long as the final
character is `]'." This is again where backwards compatibility and
assoc_expand_once matter.

We can apply your change, but it is still incomplete (plus it breaks things
that currently work, like

declare -A a
key='$(echo foo)'

a['$key']=2
declare -p a

unset -v "a['\$key']"
declare -p a

). The real issue is that it's still going to expand the subscript for
backwards compatibility. I think that's going to have to be solved at a
higher level.

Maybe the unset builtin code should just punt as described above --
depending on the shell compatibility level -- and turn on
VA_ONEWORD|VA_NOEXPAND if there is an unquoted `[', the last character is a
`]', and valid_array_reference() returns true.

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/