ui_adaptor: don't invalidate uis below when exiting game

[andrei8l]

Summary

None

Purpose of change

Fixes: #54886 Fixes: #73667 (duplicate) Closes: #75992 (invalidates)

The destructor for ui_adaptor() relies on UB wrt the state of static global objects.

Describe the solution

Don't try to invalidate ui_adaptors below if we're quitting the game

Describe alternatives you've considered

Figure out a stable order of initialization for g (game) and ui_stack so that g and its ui_adaptors die before ui_stack: this PR is simpler and less invasive

A better solution is probably to move the code from ~ui_adaptor() to some kind of helper guard struct.

Testing

Using the binaries from this release:

1. Sleep
2. While the Wait till you wake up static popup is running its animation, send a SIGTERM to the game
3. No segfault

Additional context

N/A

[andrei8l]

@Qrox can you take a look please? It has been a minute since I've looked at this code

[Qrox]

Looks good to me, thanks for fixing it! I'm a bit surprised this hasn't been caught by static analysis though...

[akrieger]

It seems to only happen on handling signals with ui present and requires using a user dir flag apparently. We don't run tests with ui present.

[andrei8l]

Thanks

[NetSysFire]

I think this needs to be backported to the 0.H release candidate. I was testing and when I exited the curses version (probably applicable to tiles, too) it spat out this backtrace, which looks to me as if this fix would address this.

    ./cataclysm(debug_write_backtrace(std::ostream&)+0x40) [0x55e9483c5a65]
    ./cataclysm(+0xbc1692) [0x55e94839d692]
    ./cataclysm(+0xbc19b6) [0x55e94839d9b6]
    /usr/lib/libc.so.6(+0x3d1d0) [0x7f3d9321e1d0]
    ./cataclysm(game::draw(ui_adaptor&)+0x34) [0x55e94850927e]
    ./cataclysm(ui_adaptor::redraw_invalidated()+0x239) [0x55e948e1dd03]
    ./cataclysm(+0x1024075) [0x55e948800075]
    /usr/lib/libc.so.6(+0x3d1d0) [0x7f3d9321e1d0]
    /usr/lib/libc.so.6(poll+0x14) [0x7f3d932ec604]
    /usr/lib/libncursesw.so.6(_nc_timed_wait+0x107) [0x7f3d937da937]
    /usr/lib/libncursesw.so.6(+0x18a5f) [0x7f3d937aca5f]
    /usr/lib/libncursesw.so.6(wgetch+0x3c) [0x7f3d937ad79c]
    ./cataclysm(input_manager::get_input_event(keyboard_mode)+0x130) [0x55e948a819fc]
    ./cataclysm(input_context::handle_input[abi:cxx11](int)+0x68) [0x55e9485eb9e0]
    ./cataclysm(game::handle_mouseview(input_context&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)+0x3c) [0x55e9484f155e]
    ./cataclysm(game::get_player_input(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)+0xc21) [0x55e948576ac9]
    ./cataclysm(game::handle_action()+0x161) [0x55e948585675]
    ./cataclysm(do_turn()+0x775) [0x55e948431874]
    ./cataclysm(main+0x1ebe) [0x55e947f445d6]
    /usr/lib/libc.so.6(+0x25e08) [0x7f3d93206e08]

[akrieger]

@andrei8l does this stack look related? https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/10713363162/job/29705360471?pr=76153#step:18:298

(~[slow] ~[.],starting_items)=> ==7192==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603005886280 at pc 0x0000077e40bf bp 0x7ffe05da0860 sp 0x7ffe05da0858
(~[slow] ~[.],starting_items)=> READ of size 8 at 0x603005886280 thread T0
(~[slow] ~[.],starting_items)=>     #0 0x77e40be in std::reference_wrapper<ui_adaptor>::get() const /opt/mock-gcc-11/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/refwrap.h:338:17
(~[slow] ~[.],starting_items)=>     #1 0x77e40be in ui_adaptor::~ui_adaptor() /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/ui_manager.cpp:93:18
(~[slow] ~[.],starting_items)=>     #2 0x25aeaa7 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /opt/mock-gcc-11/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:168:6
(~[slow] ~[.],starting_items)=>     #3 0x5222042 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /opt/mock-gcc-11/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:705:11
(~[slow] ~[.],starting_items)=>     #4 0x5222042 in std::__shared_ptr<ui_adaptor, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() /opt/mock-gcc-11/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:1154:31
(~[slow] ~[.],starting_items)=>     #5 0x5222042 in overmap_ui::overmap_draw_data_t::~overmap_draw_data_t() /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/overmap_ui.h:102:8
(~[slow] ~[.],starting_items)=>     #6 0x50e6f29 in game::~game() /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/game.cpp:480:13
(~[slow] ~[.],starting_items)=>     #7 0x521fa0e in std::default_delete<game>::operator()(game*) const /opt/mock-gcc-11/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/unique_ptr.h:85:2
(~[slow] ~[.],starting_items)=>     #8 0x521fa0e in std::unique_ptr<game, std::default_delete<game> >::~unique_ptr() /opt/mock-gcc-11/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/unique_ptr.h:361:4
(~[slow] ~[.],starting_items)=>     #9 0x7f96e4045494  (/lib/x86_64-linux-gnu/libc.so.6+0x45494)
(~[slow] ~[.],starting_items)=>     #10 0x7f96e404560f in exit (/lib/x86_64-linux-gnu/libc.so.6+0x4560f)
(~[slow] ~[.],starting_items)=>     #11 0x7f96e4029d96  (/lib/x86_64-linux-gnu/libc.so.6+0x29d96)
(~[slow] ~[.],starting_items)=>     #12 0x7f96e4029e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f)
(~[slow] ~[.],starting_items)=>     #13 0x24ed414 in _start (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x24ed414)

That should have been on new enough code because of auto rebase, but I'll resubmit after a manual rebase and see if it comes back...

[andrei8l]

Definitely. This might fix it:

diff --git a/src/main.cpp b/src/main.cpp
index d2506b6024..17c4813494 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -151,9 +151,8 @@ void exit_handler( int s )
         deinitDebug();
 
         int exit_status = 0;
-        g.reset();
-
         catacurses::endwin();
+        g.reset();
 
 #if defined(__ANDROID__)
         // Avoid capturing SIGABRT on exit on Android in crash report
diff --git a/tests/test_main.cpp b/tests/test_main.cpp
index 107195f3e6..05b0f49dc6 100644
--- a/tests/test_main.cpp
+++ b/tests/test_main.cpp
@@ -458,5 +458,7 @@ int main( int argc, const char *argv[] )
         return 1;
     }
 
+    catacurses::endwin();
+
     return result;
 }

I'll test it out tonight.