Crash with debug command :unit advances=yes

[sevu]

:unit advances=yes cause the game to crash.
(correct would be unit advances=3 times, i.e. a number is expected, string causes a crash) The correct behaviour would be to just ignore the command.
Build c9eae7c

/usr/src/debug/gcc/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.h:1201: std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::const_reference std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator[](size_type) const [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; const_reference = const char&; size_type = long unsigned int]: Assertion '__pos <= size()' failed.

SIGABRT with today's 1.17.13-1-gc9eae7c35bd
Backtrace:

#0  0x00007fa818ea08ec in ?? () from /usr/lib/libc.so.6
#1  0x00007fa818e51ea8 in raise () from /usr/lib/libc.so.6
#2  0x00007fa818e3b53d in abort () from /usr/lib/libc.so.6
#3  0x00007fa8190d30e2 in std::__glibcxx_assert_fail (file=file@entry=0x7fa8191b9b60 "/usr/src/debug/gcc/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.h", line=line@entry=1201, function=function@entry=0x7fa8191b9a30 "std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::const_reference std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator[](size_type) const [with _CharT = char; _Traits = std::char_traits<ch"..., condition=condition@entry=0x7fa8191b73f1 "__pos <= size()") at /usr/src/debug/gcc/gcc/libstdc++-v3/src/c++11/debug.cc:60
#4  0x00007fa81914c9bc in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::operator[] (this=<optimized out>, __pos=<optimized out>) at /usr/src/debug/gcc/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.h:1201
#5  std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::operator[] (this=<optimized out>, __pos=<optimized out>) at /usr/src/debug/gcc/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.h:1199
#6  0x000055ea46b3f6fa in edit_distance_approx (str_1="unitadvances:to", str_2="unit") at src/formula/string_utils.cpp:384
#7  0x000055ea4665f02d in events::map_command_handler<events::console_handler>::dispatch (this=0x7fff1e9b25b0, cmd="unitadvances:to") at src/map_command_handler.hpp:210
#8  0x000055ea4664b0f9 in events::menu_handler::do_command (this=0x7fff1e9b3e50, str="unitadvances:to") at src/menu_events.cpp:1445
#9  0x000055ea45eb660b in play_controller::enter_textbox (this=0x7fff1e9b3c00) at src/play_controller.cpp:651
#10 0x000055ea45eb7abd in play_controller::process_focus_keydown_event (this=0x7fff1e9b3c00, event=...) at src/play_controller.cpp:847
#11 0x000055ea464079b1 in controller_base::handle_event (this=0x7fff1e9b3c00, event=...) at src/controller_base.cpp:139
#12 0x000055ea46b1fd30 in events::pump () at src/events.cpp:723
#13 0x000055ea46408a25 in controller_base::play_slice (this=0x7fff1e9b3c00, is_delay_enabled=true) at src/controller_base.cpp:392
#14 0x000055ea45ec8ee5 in playmp_controller::play_slice (this=0x7fff1e9b3c00, is_delay_enabled=true) at src/playmp_controller.cpp:492
#15 0x000055ea45eb9e18 in play_controller::play_slice_catch (this=0x7fff1e9b3c00) at src/play_controller.cpp:1155
#16 0x000055ea45ec638a in playmp_controller::play_human_turn (this=0x7fff1e9b3c00) at src/playmp_controller.cpp:158
#17 0x000055ea45ecf414 in playsingle_controller::play_side_impl (this=0x7fff1e9b3c00) at src/playsingle_controller.cpp:429
#18 0x000055ea45ebb7ed in play_controller::play_side (this=0x7fff1e9b3c00) at src/play_controller.cpp:1329
#19 0x000055ea45ebbc3c in play_controller::play_turn (this=0x7fff1e9b3c00) at src/play_controller.cpp:1369
#20 0x000055ea45ecd51b in playsingle_controller::play_scenario_main_loop (this=0x7fff1e9b3c00) at src/playsingle_controller.cpp:178
#21 0x000055ea45ecdf86 in playsingle_controller::play_scenario (this=0x7fff1e9b3c00, level=...) at src/playsingle_controller.cpp:284
#22 0x000055ea45bd8cbb in campaign_controller::playmp_scenario (this=0x7fff1e9b4510, end_level=...) at src/game_initialization/playcampaign.cpp:223
#23 0x000055ea45bd8faa in campaign_controller::play_game (this=0x7fff1e9b4510) at src/game_initialization/playcampaign.cpp:286
#24 0x000055ea45bcb894 in mp::(anonymous namespace)::mp_manager::enter_staging_mode (this=0x7fff1e9b4ac0) at src/game_initialization/multiplayer.cpp:605
#25 0x000055ea45bcb4c0 in mp::(anonymous namespace)::mp_manager::enter_create_mode (this=0x7fff1e9b4ac0) at src/game_initialization/multiplayer.cpp:577
#26 0x000055ea45bcc463 in mp::start_local_game () at src/game_initialization/multiplayer.cpp:692
#27 0x000055ea45be35f2 in game_launcher::play_multiplayer (this=0x55ea47b70200, mode=game_launcher::mp_mode::LOCAL) at src/game_launcher.cpp:898
#28 0x000055ea45a2be8e in do_gameloop (args=std::vector of length 2, capacity 2 = {...}) at src/wesnoth.cpp:967
#29 0x000055ea45a2d7be in main (argc=2, argv=0x7fff1e9b7268) at src/wesnoth.cpp:1183

[stevecotton]

Looks like edit_distance_approx buffer-overflows if the longer string starts with the shorter string, for example unitadvances:to and unit. Looks like its test for swapped letters would buffer overflow even if the std::max call were changed to std::min.

Seems a good first issue, it's a self-contained C++ function doing standard C++ string handling. Reading PR #6688 would explain where the commands are being typed, and what this code is used for.

[Wedge009]

Confirmed on both master and 1.16 - log just makes a mention of stoi, guessing there's a presumption that the input is always a number.

[stevecotton]

Oh, interesting. It's good to have a fix for the other issue, but that bug isn't the one that sevu posted the stack trace for; the one that sevu found is only on 1.17 (edit_distance_approx isn't in 1.16).

Ah, the stack trace is for missing the space, so it's :unitadvances=3 or :unitadvances=yes.

As the PR is already open, probably repurpose this to be the :unit advances=yes bug, and make a new one for the stack trace.

[Wedge009]

Odd. Against master I've tried unitadvances=1, unitadvances=x, :unitadvances=3, :unitadvances=yes, in all cases I get an in-game 'unknown command' message, but no crashing.

[stevecotton]

I didn't replicate sevu's bug until now, previously I just looked at the stack trace he provided.

Edit: the rest of the comment is #7412, not #7402.

That assert only triggers if Wesnoth is built with -D_GLIBCXX_DEBUG or -D_GLIBCXX_ASSERTIONS. We have a CMake option for the former, but it needs Boost's non-header program_options to be compiled with that flag too; I think Fedora might be building everything with that flag.

Adding a CMake and SCons option for -D_GLIBCXX_ASSERTIONS seems a good option, looking at that now. With that flag added to the build, I can replicate the bug. To do it with a command such as :unit needs :debug to be enabled first, so using a non-debug command might be better for the rewritten replication steps.

[CelticMinstrel]

The fact that it's an assert with -D_GLIBCXX_DEBUG means it's probably undefined behaviour so we need to fix it regardless.