alefore / edge

Edge - a text editor
GNU General Public License v3.0
78 stars 2 forks source link

Failure for HelpCommand: Edge crashes with SIGABRT on ‘?’ #7

Closed kseistrup closed 12 months ago

kseistrup commented 1 year ago

Edge built from commit 8fdf7f76ffa167497a7e9e9228b26b1f2dcf95e9 crashes with SIGABRT if ? is pressed to get help while running Edge:

$ edge --tests=run --tests_filter=HelpCommand::GenerateContents.GenerateContents
# Test Groups

## Group: HelpCommand::GenerateContents

* GenerateContents
/usr/include/c++/13.2.1/bits/stl_vector.h:1125: constexpr std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = char; _Alloc = std::allocator<char>; reference = char&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
*** Aborted at 1699542650 (unix time) try "date -d @1699542650" if you are using GNU date ***
PC: @     0x7f08f028783c (unknown)
*** SIGABRT (@0x3e8000fc2f6) received by PID 1032950 (TID 0x7f08f068ad00) from PID 1032950; stack trace: ***
    @     0x7f08f086d81a google::(anonymous namespace)::FailureSignalHandler()
    @     0x7f08f0237710 (unknown)
    @     0x7f08f028783c (unknown)
    @     0x7f08f0237668 raise
    @     0x7f08f021f4b8 abort
    @     0x7f08f04dd3b2 std::__glibcxx_assert_fail()
    @     0x55b1b5225890 operator<<()
    @     0x55b1b553a1d5 afc::vm::operator<<()
    @     0x55b1b5372cf3 _ZZN3afc6editor12_GLOBAL__N_111HelpCommand15ShowEnvironmentERKNS0_10OpenBufferERNS_8language4text19MutableLineSequenceEENKUlRKNSt7__cxx1112basic_stringIwSt11char_traitsIwESaIwEEERKNS6_2gc3PtrINS_2vm5ValueEEEE_clESH_SO_
    @     0x55b1b55665dd afc::vm::Environment::ForEach()
    @     0x55b1b55664be afc::vm::Environment::ForEach()
    @     0x55b1b536b712 _ZN3afc6editor12_GLOBAL__N_111HelpCommand16GenerateContentsERKNS0_15MapModeCommandsERKNS0_10OpenBufferE.lto_priv.0
    @     0x55b1b5375b52 _ZNSt17_Function_handlerIFvvEN3afc6editor12_GLOBAL__N_1L19buffer_registrationMUlvE_EE9_M_invokeERKSt9_Any_data.lto_priv.0
    @     0x55b1b54e02d1 afc::tests::Run()
    @     0x55b1b5578e2d afc::command_line_arguments::Parse<>()
    @     0x55b1b521bbe5 main
    @     0x7f08f0220cd0 (unknown)
    @     0x7f08f0220d8a __libc_start_main
    @     0x55b1b5224f85 _start

This issue was originally included in #6.

alefore commented 1 year ago

Thank you for filing this issue, I hope we can figure this out. It's quite strange, looks like a corrupted string.

Would you be able to update the build to the last version (anything after 9520beb) and attach the output of the following command?

GLOG_alsologtostderr=y GLOG_vmodule=environment=10,help_command=10 edge --tests=run --tests_filter=HelpCommand::GenerateContents.GenerateContents

This should generate a lot of log lines as it runs (in my build that outputs 268 lines; it'll probably be fewer for you, since we expect it to crash), lines like this:

I1114 07:32:46.898674 18848 environment.cc:270] ForEachNonRecursive: Running callback on: terminal_control_d 
I1114 07:32:46.898676 18848 help_command.cc:254] Generating view for variable: terminal_control_d 
I1114 07:32:46.898679 18848 help_command.cc:255] Type: string 
I1114 07:32:46.898680 18848 help_command.cc:258] Adding to output.

If you could upload that output, that would be very helpful: seeing what symbol was being expanded when the crash is triggered may help me narrow down where the problem may be.

Thank you once again for your time/help with this.

kseistrup commented 1 year ago

Edge built from commit 9520bebb9824bae51761747229f2f9bdb8fa5d03

📎 edge-9520bebb9824bae51761747229f2f9bdb8fa5d03.txt

I don't understand the output, but I do notice that “LTO” occurs a few times. My troubles are not because my default CFLAGS have -flto, is it?

    @     0x5577e35ce40b _ZNK3afc10concurrent9ProtectedINS_2vm11Environment4DataENS0_14EmptyValidatorIS4_EELb1EE4lockIZNKS3_19ForEachNonRecursiveESt8functionIFvRKNSt7__cxx1112basic_stringIwSt11char_traitsIwESaIwEEERKNS_8language2gc3PtrINS2_5ValueEEEEEEUlRKS4_E_EEDaT_.lto_priv.0
    @     0x5577e33d1b3b _ZN3afc6editor12_GLOBAL__N_111HelpCommand16GenerateContentsERKNS0_15MapModeCommandsERKNS0_10OpenBufferE.lto_priv.0
    @     0x5577e33dbf12 _ZNSt17_Function_handlerIFvvEN3afc6editor12_GLOBAL__N_1L19buffer_registrationMUlvE_EE9_M_invokeERKSt9_Any_data.lto_priv.0
kseistrup commented 1 year ago

[One hour later…]

Recompiling everything without LTO doesn't change anything:

:paperclip: edge-err-2.txt

alefore commented 12 months ago

Thank you for posting that, that was very helpful and now I understand what's going on.

There's a bug where I'm storing negative numbers inside a std::wstring instances in a few places. I use the negative values (e.g., -7) to represent special characters (i.e., "the user pressed backspace", "the user pressed left arrow"). This happens in terminal_backspace and a few other related symbols. I had assumed that storing negative values is OK, but ... this is an abuse of the std::wstring type, at best. I don't know why it doesn't crash in my machine.

Fixing that shouldn't be too hard, but I'll need to come up with some separate representation for these values (perhaps a vector of ), store information in that representation, and adjust the consumers of this information to use the new representation. It might take me some time to get to that fix.

Thanks again!

alefore commented 12 months ago

Hello.

I expect this issue should be fixed by https://github.com/alefore/edge/commit/3f67b6ca91195365f941736127c96f4e3e2c36ee. That's a fairly large change. Hopefully I didn't break anything else accidentally. :-)

Could you sync to the latest version and confirm that the crash (from pressing ?) and the test failure (from edge --tests=run --tests_filter=HelpCommand::GenerateContents.GenerateContents) went away?

Thank you once again!

kseistrup commented 12 months ago

I hope commit 5cbeacc2db305270439dc71390b97628efc2611d (“Fix use-after-free bug”) is okay.

Pressing ? no longer crashes the editor, and the test failure doesn't produce any error output.

alefore commented 12 months ago

Great, thank you for confirming. Looks like we nailed it. :)