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

ui_adaptor: don't invalidate uis below when exiting game #76000

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

andrei8l
Copy link
Contributor

@andrei8l andrei8l commented Aug 27, 2024

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

@github-actions github-actions bot added new contributor <Bugfix> This is a fix for a bug (or closes open issue) Info / User Interface Game - player communication, menus, etc. [C++] Changes (can be) made in C++. Previously named `Code` labels Aug 27, 2024
@andrei8l
Copy link
Contributor Author

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

@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Aug 27, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 28, 2024
@Qrox
Copy link
Contributor

Qrox commented Aug 28, 2024

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

@akrieger
Copy link
Member

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.

@akrieger akrieger merged commit aa7f6e2 into CleverRaven:master Aug 28, 2024
27 of 29 checks passed
@andrei8l andrei8l deleted the ui_adaptor-shutdown branch August 29, 2024 09:23
@andrei8l
Copy link
Contributor Author

Thanks

@NetSysFire
Copy link
Member

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
Copy link
Member

akrieger commented Sep 5, 2024

@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

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.H backport candidate astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

imgui related crash when quitting during a blocking activity (falling asleep) Crash on quit without saving
5 participants