Skip to content

Fish 3.6.0 crashes every time when trying to search cyrillic text in history #9628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ZaWertun opened this issue Mar 2, 2023 · 4 comments
Closed
Labels
bug Something that's not working as intended
Milestone

Comments

@ZaWertun
Copy link

ZaWertun commented Mar 2, 2023

Steps to reproduce:

  1. Start fish shell;
  2. Type тест, press up arrow;
  3. Crash.

Error displayed after crash:

error: /builddir/build/BUILD/fish-3.6.0/src/reader.cpp:431: failed assertion: offset != wcstring::npos && "Should have found a match in the search result"

Backtrace from gdb after installing debug symbols:

#0  0x00007ffff79cfe5c in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x00007ffff797fa76 in raise () from /lib64/libc.so.6
#2  0x00007ffff79697fc in abort () from /lib64/libc.so.6
#3  0x000055555557907b in __fish_assert (msg=0x5555556ac790 "offset != wcstring::npos && \"Should have found a match in the search result\"", file=0x5555556a9e18 "/builddir/build/BUILD/fish-3.6.0/src/reader.cpp", line=431, error=<optimized out>)
    at /usr/src/debug/fish-3.6.0-1.fc37.x86_64/src/common.cpp:1860
#4  0x00005555555dc4e9 in (anonymous namespace)::reader_history_search_t::append_matches_from_search (this=0x5555557b2750) at /usr/src/debug/fish-3.6.0-1.fc37.x86_64/src/reader.cpp:431
#5  (anonymous namespace)::reader_history_search_t::move_backwards (this=0x5555557b2750) at /usr/src/debug/fish-3.6.0-1.fc37.x86_64/src/reader.cpp:472
#6  (anonymous namespace)::reader_history_search_t::move_in_direction (dir=history_search_direction_t::backward, this=0x5555557b2750) at /usr/src/debug/fish-3.6.0-1.fc37.x86_64/src/reader.cpp:497
#7  reader_data_t::handle_readline_command (this=0x5555557b2070, c=<optimized out>, rls=...) at /usr/src/debug/fish-3.6.0-1.fc37.x86_64/src/reader.cpp:3715
#8  0x00005555555deb6c in reader_data_t::readline[abi:cxx11](int) (this=0x5555557b2070, nchars_or_0=<optimized out>) at /usr/src/debug/fish-3.6.0-1.fc37.x86_64/src/reader.cpp:4484
#9  0x00005555555e4788 in read_i (parser=...) at /usr/src/debug/fish-3.6.0-1.fc37.x86_64/src/reader.cpp:3201
#10 reader_read (parser=..., fd=<optimized out>, io=...) at /usr/src/debug/fish-3.6.0-1.fc37.x86_64/src/reader.cpp:4749
#11 0x000055555556924c in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/fish-3.6.0-1.fc37.x86_64/src/fish.cpp:576

Reproducable: always.

Fish version: 3.6.0, from Fedora package fish-3.6.0-1.fc37.x86_64.

When testing in clean environment using command sh -c 'env HOME=$(mktemp -d) fish' it could be reproduced as follows:

  1. Run sh -c 'env HOME=$(mktemp -d) fish';
  2. Enter command echo Тест and hit Enter (notice capital Т);
  3. Type тест and press up arrow key;
  4. Crash.

Looks like it's something wrong when searching history case insensitive.

@ZaWertun
Copy link
Author

ZaWertun commented Mar 2, 2023

Crash report on retrace.fedoraproject.org: https://retrace.fedoraproject.org/faf/reports/618430/

@ZaWertun
Copy link
Author

ZaWertun commented Mar 2, 2023

Forgot to mention - I'm using this locale settings (if it matters):

$ locale
LANG=ru_RU.UTF-8
LC_CTYPE="ru_RU.UTF-8"
LC_NUMERIC="ru_RU.UTF-8"
LC_TIME="ru_RU.UTF-8"
LC_COLLATE="ru_RU.UTF-8"
LC_MONETARY="ru_RU.UTF-8"
LC_MESSAGES="ru_RU.UTF-8"
LC_PAPER="ru_RU.UTF-8"
LC_NAME="ru_RU.UTF-8"
LC_ADDRESS="ru_RU.UTF-8"
LC_TELEPHONE="ru_RU.UTF-8"
LC_MEASUREMENT="ru_RU.UTF-8"
LC_IDENTIFICATION="ru_RU.UTF-8"
LC_ALL=

@faho faho added this to the fish 3.6.1 milestone Mar 2, 2023
@faho faho added the bug Something that's not working as intended label Mar 2, 2023
@faho
Copy link
Member

faho commented Mar 2, 2023

Okay, let's remove the ridiculous assert for now. If we do that, it won't match case-insensitively (it appears either std::upper is borked, or how we use it is wrong), but it'll match case-sensitively and won't crash.

Honestly I would prefer if we didn't use asserts in the reader like that.

It doesn't really pay to invest too much effort into the C++ code right now because it'll be gone next release, and locale-stuff is really quite awkward in C++.

@zanchey I would like this for 3.6.1 if possible?

@zanchey
Copy link
Member

zanchey commented Mar 2, 2023

No worries

@faho faho closed this as completed in 7c91d00 Mar 2, 2023
faho added a commit that referenced this issue Mar 2, 2023
This isn't a great use of `assert` because it turns a benign "oh I
need to search again" bug into a crash.

Fixes #9628

(cherry picked from commit 7c91d00)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that's not working as intended
Projects
None yet
Development

No branches or pull requests

3 participants