Re: segfault in 'ls' completion

[Zach Riggle <zachriggle@xxxxxxxxx>]

Has anybody actually tried running ZSH with memory safety options
enabled (e.g. Address Sanitizer)?

I assume there is a test suite available to run -- I can build Zsh
from source, but I'm not sure the "right" way to build the tests.

I've subscribed to the zsh-devel mailing list as I expect this is
better suited for that ML.

Zach Riggle

On Wed, Dec 15, 2021 at 9:41 PM Vin Shelton <acs@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Stops the segfault, and generates a conforming list.
>
> Thanks,
>   Vin
>
> On Wed, Dec 15, 2021 at 6:57 PM Oliver Kiddle <opk@xxxxxxx> wrote:
>>
>> Bart Schaefer wrote:
>> >     I was able to reproduce this
>>
>> I couldn't initially but as you could, I thought I'd better try again.
>>
>> > reverted to revision e40938c128 (before the workers/49499 changes to
>> > computil.c) and was no longer able to reproduce in that version, but that does
>> > also revert some changes to _arguments.
>>
>> It actually seems it was 49518 / 7cb980b which was only applied
>> yesterday having been posted in October and forgotten. I had a nagging
>> suspicion that I needed to further check over that. My mistake was
>> mixing up hex and decimal when looking at the ASCII table to work out
>> how to rearrange the single character option letters within the lookup
>> array. 20 should have been 0x20 or 32.
>>
>> 'y' appears before the tab and the word starts with something that isn't
>> '-'. So it uses the + options offset which are later and as y is within
>> the difference between decimal and hex 20 from the end of the characters
>> this caused it index beyond the end of the array.
>>
>> Following this, I also wondered what it's doing strcmping '/usr/libpy'
>> against every possible ls option. That's nothing new. Note that
>> _arguments only lets you start options with - or + and we check for
>> those explicitly in a few places. I think it's worth optimising this
>> away. The check could perhaps be factored into ca_get_opt() and
>> ca_get_sopt() ?
>>
>> If someone has a moment, please check that the calculation in
>> single_index() makes sense. The array is allocated as
>>   ret->single = (Caopt *) zalloc(188 * sizeof(Caopt));
>>
>> Oliver
>>
>> diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
>> index c49d688c8..59abb4cc4 100644
>> --- a/Src/Zle/computil.c
>> +++ b/Src/Zle/computil.c
>> @@ -1088,10 +1088,10 @@ bslashcolon(char *s)
>>  static int
>>  single_index(char pre, char opt)
>>  {
>> -    if (opt <= 20 || opt > 0x7e)
>> +    if (opt <= 0x20 || opt > 0x7e)
>>         return -1;
>>
>> -    return opt + (pre == '-' ? -21 : 94 - 21);
>> +    return opt + (pre == '-' ? -0x21 : 94 - 0x21);
>>  }
>>
>>  /* Parse an argument definition. */
>> @@ -2158,7 +2158,8 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
>>
>>         /* See if it's an option. */
>>
>> -       if (state.opt == 2 && (state.curopt = ca_get_opt(d, line, 0, &pe)) &&
>> +       if (state.opt == 2 && (*line == '-' || *line == '+') &&
>> +           (state.curopt = ca_get_opt(d, line, 0, &pe)) &&
>>             (state.curopt->type == CAO_OEQUAL ?
>>              (compwords[cur] || pe[-1] == '=') :
>>              (state.curopt->type == CAO_EQUAL ?
>> @@ -2206,6 +2207,7 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
>>                 state.curopt = NULL;
>>             }
>>         } else if (state.opt == 2 && d->single &&
>> +                  (*line == '-' || *line == '+') &&
>>                    ((state.curopt = ca_get_sopt(d, line, &pe, &sopts)) ||
>>                     (cur != compcurrent && sopts && nonempty(sopts)))) {
>>             /* Or maybe it's a single-letter option? */