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

Prevent negative array index for floor_cache #75453

Merged
merged 1 commit into from
Aug 4, 2024

Conversation

inogenous
Copy link
Contributor

@inogenous inogenous commented Aug 4, 2024

Summary

Bugfixes "Prevent negative array index for floor_cache"

Purpose of change

Prevents referencing floor_cache using out-of-bounds array indexes such as negative values and values greater than 132. Using out-of-bound indexes for this array previously caused crashes when compiled with -D_GLIBCXX_ASSERTIONS.

Fixes #75039 .

Describe the solution

The function map::dont_draw_lower_floor is, for example, called from cata_tiles::draw with values such as p=(0,132,0) or p=(-36,5,0). Such values can be seen when zooming out with Z so draw also handles tiles that do not fit into local bub coordinates.

Previous crash happened here:

#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
#1  0x00007ffff787840f in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  0x00007ffff78294f2 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x00007ffff78124ed in __GI_abort () at ./stdlib/abort.c:79
#4  0x00007ffff7ad30be in std::__glibcxx_assert_fail(char const*, int, char const*, char const*) () from /lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00005555561d9d96 in std::array<bool, 132ul>::operator[] (__n=<optimized out>, this=<optimized out>) at /usr/include/c++/13/array:202
#6  0x00005555561dd407 in std::array<bool, 132ul>::operator[] (__n=<optimized out>, this=<optimized out>) at /usr/include/c++/13/array:200
#7  map::dont_draw_lower_floor (this=this@entry=0x5555582581a0, p=...) at src/map.cpp:7476
#8  0x0000555555b59d8f in cata_tiles::draw (this=0x5555580cd790, dest=..., center=..., width=<optimized out>, height=<optimized out>, overlay_strings=std::multimap with 0 elements, color_blocks={...}) at src/cata_tiles.cpp:1657
#9  0x00005555567f09fc in cata_cursesport::curses_drawwindow (w=...) at src/sdltiles.cpp:1317
#10 0x0000555555cfab62 in catacurses::wnoutrefresh (win_=...) at src/cursesport.cpp:189
#11 0x0000555555e9826f in game::draw (this=0x555558257c60, ui=...) at src/game.cpp:3953
#12 0x000055555695aadf in ui_adaptor::redraw_invalidated () at src/ui_manager.cpp:440
#13 0x000055555695abe3 in ui_adaptor::redraw () at src/ui_manager.cpp:345
#14 0x000055555695ac0a in ui_manager::redraw () at src/ui_manager.cpp:506
#15 0x0000555555d8a15e in do_turn () at src/do_turn.cpp:571
#16 0x0000555555781a1e in main (argc=<optimized out>, argv=<optimized out>) at src/main.cpp:873

Describe alternatives you've considered

  • A bit hesitant to call inbounds for each tile during the draw call since that might have an effect on performance. It might be possible to do some smarter out-of-bounds checking?
  • All of this would probably be better if we used _ib coordinates, which would make a bug such as this a compile-time problem instead. But that's for a future refactoring.

Testing

  1. Compiled with -D_GLIBCXX_ASSERTIONS.
  2. Ran the steps in Crash when zooming main view #75039 .
  3. Can reliably reproduce the crash on master since this function is called with p=(-36,5,0).
  4. Applied these code changes. Ran the steps in Crash when zooming main view #75039 again. The game no longer crashes when zooming out to max.

Additional context

Prevents referencing `floor_cache` using out-of-bounds array indexes
such as negative values and values greater than 132. Using out-of-bound
indexes for this array previously caused crashes when compiled with
`-D_GLIBCXX_ASSERTIONS`.

The function `map::dont_draw_lower_floor` is, for example, called from
`cata_tiles::draw``with values such as `p=(0,132,0)` or `p=(-36,5,0)`.
Such values can be seen when zooming out with `Z` so `draw` also handles
tiles that do not fit into local bub coordinates.
@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Aug 4, 2024
@dseguin dseguin merged commit 711754c into CleverRaven:master Aug 4, 2024
25 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when zooming main view
2 participants