Shell crashes with core dump on certain tab completions

[torkelrogstad]

I'm getting a very unexpected crash when trying to tab complete a specific string. This is the minimum example I've been able to create:

$ echo "/?$

When hitting <TAB> while the cursor is at the end of the line (right next to the $) I get a crash:

❯ echo "/?$error: /build/fish-6NpQP7/fish-3.5.1/src/fallback.cpp:209: failed assertion: width > 0 && "Unexpectedly nonpositive width"
<E> fish: Backtrace:
0   fish(+0x61972) [0x55e6ac675972]
1   fish(+0xcc592) [0x55e6ac6e0592]
2   fish(+0xc25cd) [0x55e6ac6d65cd]
3   fish(+0xacffb) [0x55e6ac6c0ffb]
4   fish(+0xbd70f) [0x55e6ac6d170f]
5   fish(+0xba347) [0x55e6ac6ce347]
6   fish(+0xbad40) [0x55e6ac6ced40]
7   fish(+0x1613a) [0x55e6ac62a13a]
8   /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7f7a40b2fd90]
9   __libc_start_main + 128
10  fish(+0x17aee) [0x55e6ac62baee]

Aborted (core dumped)

System info:

❯ fish --version
fish, version 3.5.1

❯ echo $version
3.5.1

❯ echo $TERM
xterm-256color

❯ uname -a
Linux torkel 5.15.0-43-generic #46-Ubuntu SMP Tue Jul 12 10:30:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

[faho]

Okay, I fixed the crash - this was caused by an incomplete switch/case. Our thing to figure out the width got a new case, and we forgot to add it.

This case was reached however, because one of our internal signalling chars got through - we use a bunch of in-band signalling to say "this is an expansion" and such. Now to figure out how exactly that happened.

The stacktrace is:

#4  0x00005555555ab80e in fish_wcwidth (wc=64979 L'\xfdd3') at /home/alfa/dev/fish-shell/src/fallback.cpp:209
#5  fish_wcwidth (wc=64979 L'\xfdd3') at /home/alfa/dev/fish-shell/src/fallback.cpp:172
#6  0x000055555561899e in fish_wcwidth_visible (widechar=<optimized out>) at /home/alfa/dev/fish-shell/src/wcstringutil.cpp:316
#7  0x000055555560f04d in compute_layout (s=0x5555557af3e0, autosuggestion_str=L"", commandline=L"echo /\\?\xfdd3 ", right_untrunc_prompt=..., 
    left_untrunc_prompt=<error reading variable: Cannot access memory at address 0x10>, screen_width=162) at /home/alfa/dev/fish-shell/src/screen.cpp:998
#8  screen_t::write (this=this@entry=0x5555557af3e0, left_prompt=L"\033[38;2;243;158;207m~/o/d/f/build\033(B\033[m►", 
    right_prompt=L"\033(B\033[m(\033[31m(96a064d6…)\033(B\033[m|\033[1m\033[34m?\033(B\033[m) \033[90m17:28\033(B\033[m", commandline=L"echo /\\?\xfdd3 ", 
    explicit_len=explicit_len@entry=10, colors=std::vector of length 10, capacity 10 = {...}, indent=std::vector of length 10, capacity 10 = {...}, 
    cursor_pos=10, vars=..., pager=..., page_rendering=..., cursor_is_within_pager=false) at /home/alfa/dev/fish-shell/src/screen.cpp:1151
#9  0x00005555555fc667 in reader_data_t::paint_layout (this=this@entry=0x5555557aefc0, reason=reason@entry=0x5555556cce10 L"toplevel")
    at /home/alfa/dev/fish-shell/src/reader.cpp:1193
#10 0x00005555556093ba in reader_data_t::layout_and_repaint (this=this@entry=0x5555557aefc0, reason=reason@entry=0x5555556cce10 L"toplevel", mcolors=...)
    at /home/alfa/dev/fish-shell/src/reader.cpp:782
#11 0x000055555560695a in operator() (__closure=<synthetic pointer>) at /home/alfa/dev/fish-shell/src/reader.cpp:4314
#12 reader_data_t::readline[abi:cxx11](int) (this=this@entry=0x5555557aefc0, nchars_or_0=nchars_or_0@entry=0) at /home/alfa/dev/fish-shell/src/reader.cpp:4331

So, while repainting, the commandline passes through to wcwidth in "unescaped" state - that commandline=L"echo /\\?\xfdd3 " has a private-use character that we use for these things.

[torkelrogstad]

Wow, incredible response time here! Thanks for working hard on my favorite shell 🐟 ❤️

[faho]

Ah, found it:

We have started to expand globs now before doing any other completions. That means we go through our "expand" machinery and tell it to only expand wildcards.

In this case, without the "qmark-noglob" feature flag, the ? is, at first, seen as a wildcard (you can also use * instead of ? to trigger it even with the feature flag). That's why it is necessary. The / is probably required so it looks like an absolute path at first.

And, if you tell expand to skip something, it'll first mark it ("unescape" it), and then put it back. Only "$ outside of quotes" and "$ inside quotes" are two different symbols, and we forgot to put the "VARIABLE_EXPAND_SINGLE" (i.e. "dollar inside quotes") back.

@ridiculousfish would probably be interested and might be able to explain how to add a test for this.

So:

• db20356 removes the crash - we should handle "non_characters" here
• 7b2f4f6 fixes the actual completion

I'm calling this a regression because I'm fairly certain it was introduced in 3.5.0 (when we started expanding globs here).