Skip to content
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

GLIBCXX_ASSERTIONS reveals problem in string match subcommand #5479

Closed
zanchey opened this Issue Jan 4, 2019 · 10 comments

Comments

Projects
None yet
5 participants
@zanchey
Copy link
Member

zanchey commented Jan 4, 2019

Fedora 28+ sets some hardening options which tighten up potentially-invalid accesses, which cause an assert to fire in string match.

To reproduce, build with glibc (ie Linux only) from HEAD (or 3.0.0) with -D_GLIBCXX_ASSERTIONS, then run ./fish -c 'string match --entire "" -- banana (from the tests).

This crashes with:

/usr/include/c++/6/bits/basic_string.h:997: std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::reference std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::front() [with _CharT = wchar_t; _Traits = std::char_traits<wchar_t>; _Alloc = std::allocator<wchar_t>; std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::reference = wchar_t&]: Assertion '!empty()' failed.
fish: “./fish -c 'string match --entir…” terminated by signal SIGABRT (Abort)

The backtrace follows:

0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007eff100f842a in __GI_abort () at abort.c:89
#2  0x00005585fd330c82 in std::__replacement_assert (__file=__file@entry=0x5585fd40baf8 "/usr/include/c++/6/bits/basic_string.h",
    __line=__line@entry=997,
    __function=__function@entry=0x5585fd412020 <_ZZNSt7__cxx1112basic_stringIwSt11char_traitsIwESaIwEE5frontEvE19__PRETTY_FUNCTION__> "std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::reference std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::front() [with _CharT = wchar_t; _Traits = std::char_traits<wchar_t>; _Alloc = std::a"..., __condition=__condition@entry=0x5585fd40cd9a "!empty()")
    at /usr/include/x86_64-linux-gnu/c++/6/bits/c++config.h:446
#3  0x00005585fd3463cd in std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::front (
    this=<optimized out>) at /usr/include/c++/6/bits/basic_string.h:997
#4  wildcard_matcher_t::wildcard_matcher_t (streams=..., opts=..., pattern=L"", this=<optimized out>)
    at /home/wheel/zanchey/src/fish-shell/src/builtin_string.cpp:626
#5  make_unique<wildcard_matcher_t, wchar_t*&, wchar_t const*&, options_t&, io_streams_t&> ()
    at /home/wheel/zanchey/src/fish-shell/src/common.h:816
#6  string_match (parser=..., streams=..., argc=<optimized out>, argv=0x5585fe36be48)
    at /home/wheel/zanchey/src/fish-shell/src/builtin_string.cpp:857
... (boring bits removed)

where frame 4 looks like:

#4  wildcard_matcher_t::wildcard_matcher_t (streams=..., opts=..., pattern=L"", this=<optimized out>)
    at /home/wheel/zanchey/src/fish-shell/src/builtin_string.cpp:626
621                     wcpattern[i] = towlower(wcpattern[i]);
622                 }
623             }
624             if (opts.entire) {
625                 // If the pattern is empty, this becomes one ANY_STRING that matches everything.
626                 if (wcpattern.front() != ANY_STRING) wcpattern.insert(0, 1, ANY_STRING);
627                 if (wcpattern.back() != ANY_STRING) wcpattern.push_back(ANY_STRING);
628             }
629         }
630

@zanchey zanchey added this to the fish-future milestone Jan 4, 2019

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 4, 2019

@zanchey: I can't reproduce it, but I'm pretty sure 9d4e460 solves it.

Can you confirm?

@zanchey

This comment has been minimized.

Copy link
Member Author

zanchey commented Jan 4, 2019

Yes, it does. Merge at will!

@zanchey zanchey modified the milestones: fish-future, fish 3.1.0 Jan 4, 2019

@faho faho closed this in 9d4e460 Jan 4, 2019

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 4, 2019

It's a straight-up invalid memory access error which should be caught by asan or valgrind. Did none of the functions in the test suite cover this branch of the code?

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 4, 2019

@mqudsi: These functions happen to work that way, without assertions. @zanchey used an example from the tests - string match --entire "" -- banana. Which, going by the use of food, is mine.

So we probably got lucky that that bit was always filled alright.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 4, 2019

Ah thanks, @faho

@zanchey

This comment has been minimized.

Copy link
Member Author

zanchey commented Jan 4, 2019

I'm not sure why ASan didn't pick it up, though - we run the Travis builds with it.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 5, 2019

I'm not sure why ASan didn't pick it up, though - we run the Travis builds with it.

That's what I was saying, but I understood @faho to mean that this was an input-specific bug that wasn't triggered by the inputs we were providing in the test suite.

@zanchey

This comment has been minimized.

Copy link
Member Author

zanchey commented Jan 5, 2019

Nope - that function was straight from the test suite, picked up by make test on Fedora's RPM builders. @ignatenkobrain, you'll probably want to add 9d4e460 as a patch in your 3.0.0 spec.

@faho faho added the bug label Jan 5, 2019

zanchey added a commit to zanchey/fish-shell that referenced this issue Jan 5, 2019

@zanchey zanchey referenced this issue Jan 12, 2019

Closed

fish 3.0.1? #5520

7 of 7 tasks complete

@zanchey zanchey modified the milestones: fish 3.1.0, fish 3.0.1 Jan 18, 2019

ridiculousfish added a commit that referenced this issue Jan 21, 2019

string: Fix crash with _GLIBCXX_ASSERTIONS
This asserted because we accessed wcstring::front() when it was empty.

Instead, check explicitly for it being empty before.

Fixes #5479
@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jan 22, 2019

Picked into 3.0.1 as 0598046

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.