Prevent crash from negative array index for visibility_cache

[inogenous]

Summary

Bugfixes "Prevent crash from negative array index for visibility_cache"

Purpose of change

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

Describe the solution

The function pixel_minimap::render_critters is, for example, called with center=(64,59,-5), which gives start=(4,-1) and then p=(4,-1,-5), which previously crashed because visibility_cache[p.x][p.y] then gives a negative array index. Such values were seen when peeking using X at a submap boundary.

Gdb backtrace of previous crash being fixed:

(gdb) bt
 #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  0x000055555668a04e in std::array<lit_level, 132ul>::operator[] (__n=18446744073709551615, this=0x555599f30348) at /usr/include/c++/13/array:211
 #6  0x000055555668ab7a in std::array<lit_level, 132ul>::operator[] (__n=18446744073709551615, this=0x555599f30348) at /usr/include/c++/13/array:211
 #7  pixel_minimap::render_critters (this=this@entry=0x555558108960, center=...) at src/pixel_minimap.cpp:521
 #8  0x000055555668ada2 in pixel_minimap::render (this=0x555558108960, center=...) at src/pixel_minimap.cpp:447
 #9  0x000055555668bb69 in pixel_minimap::draw (this=<optimized out>, screen_rect=..., center=...) at src/pixel_minimap.cpp:555
 #10 0x0000555555b3fd5c in cata_tiles::draw_minimap (this=this@entry=0x555558093020, dest=..., center=..., width=width@entry=352, height=height@entry=352) at src/cata_tiles.cpp:1919
 #11 0x00005555567f43d3 in cata_cursesport::curses_drawwindow (w=...) at src/sdltiles.cpp:1428
 #12 0x000055555666f5e2 in std::function<void(draw_args const&)>::operator() (__args#0=..., this=<optimized out>) at /usr/include/c++/13/bits/std_function.h:591
 #13 operator() (d=..., __closure=<optimized out>) at src/panels.cpp:54
 #14 std::__invoke_impl<int, window_panel::window_panel(const std::function<void(const draw_args&)>&, const std::string&, const translation&, int, int, bool, const std::function<bool()>&, bool)::<lambda(const draw_args&)>&, const draw_args&> (__f=...) at /usr/include/c++/13/bits/invoke.h:61
 #15 std::__invoke_r<int, window_panel::window_panel(const std::function<void(const draw_args&)>&, const std::string&, const translation&, int, int, bool, const std::function<bool()>&, bool)::<lambda(const draw_args&)>&, const draw_args&> (__fn=...) at /usr/include/c++/13/bits/invoke.h:114
 #16 std::_Function_handler<int(const draw_args&), window_panel::window_panel(const std::function<void(const draw_args&)>&, const std::string&, const translation&, int, int, bool, const std::function<bool()>&, bool)::<lambda(const draw_args&)> >::_M_invoke(const std::_Any_data &, const draw_args &) (__functor=...,     __args#0=...) at /usr/include/c++/13/bits/std_function.h:290
 #17 0x0000555555e7c9ec in std::function<int(draw_args const&)>::operator() (__args#0=..., this=0x55559461d638) at /usr/include/c++/13/bits/std_function.h:591
 #18 game::draw_panels (this=this@entry=0x555558276c40, force_draw=force_draw@entry=true) at src/game.cpp:4006
 #19 0x0000555555e984f8 in game::draw (this=0x555558276c40, ui=...) at src/game.cpp:3960
 #20 0x000055555695df35 in ui_adaptor::redraw_invalidated () at src/ui_manager.cpp:440
 #21 0x000055555695e039 in ui_adaptor::redraw () at src/ui_manager.cpp:345
 #22 0x000055555695e060 in ui_manager::redraw () at src/ui_manager.cpp:506
 #23 0x0000555555ea95cf in game::look_around (this=this@entry=0x555558276c40, show_window=show_window@entry=true, center=..., start_point=..., has_first_point=has_first_point@entry=false, select_zone=false, peeking=true, is_moving_zone=<optimized out>, end_point=..., change_lv=true) at src/game.cpp:7529
 #24 0x0000555555eadd32 in game::look_around (this=this@entry=0x555558276c40, looka_params=...) at src/game.cpp:7697
 #25 0x0000555555eadecb in game::peek (this=this@entry=0x555558276c40, p=...) at src/game.cpp:6071
 #26 0x0000555555ec441a in game::peek (this=this@entry=0x555558276c40) at src/game.cpp:6050
 #27 0x0000555555f124ec in game::do_regular_action (this=this@entry=0x555558276c40, act=@0x7fffffffd104: ACTION_PEEK, player_character=..., mouse_target=std::optional [no contained value]) at src/handle_action.cpp:2432
 #28 0x0000555555f15b09 in game::handle_action (this=0x555558276c40) at src/handle_action.cpp:3174
 #29 0x0000555555d8a108 in do_turn () at /usr/include/c++/13/bits/unique_ptr.h:199
 #30 0x0000555555781b4e in main (argc=<optimized out>, argv=<optimized out>) at src/main.cpp:873

(gdb) frame 7
(gdb) print p
$1 = {static dimension = 3, x = 4, y = -1, z = -5}
(gdb) print center
$2 = (const tripoint &) @0x7fffffffbda8: {static dimension = 3, x = 64, y = 59, z = -5}

Describe alternatives you've considered

• If we instead protected all access to these caches so that accessing them requires an instance of an already inbounds-checked coordinate (such as _ib), then problems such as these could be a compile-time error instead of silent out-of-bounds reading.

Testing

• Could reliably reproduce the crash when peeking around a corner.
• With this change, this particular crash can no longer be reproduced.
• Have verified that monsters on minimap are still shown correctly.

Additional context

[PatrikLundell]

Since you create a variable for the map, you might as well use it on the next line getting the level cache (and check if there are any other get_map calls that can be replaced).

I don't see how you'd be able to convert these checks into compile time checks by using a pre checked type. It ought to result in it blowing up on assignment of the variable of said type when out of bounds instead (although I can't find the code for it in the template hell, and while it should still be better than seg faults, it's probably worse than your correction, where you reject the out of bounds parameters and treats it appropriately (by ignoring the out of bounds locations), rather than having a type conversion check blow up and probably crash the game with an at least some somewhat helpful error message).

[inogenous]

I don't see how you'd be able to convert these checks into compile time checks by using a pre checked type

Thinking on it some more, you're probably right that it would need to be some runtime check anyway, like you say. My thinking was that at the moment, we allow reading these caches like cache.visibility_cache[p.x][p.y], and might need to check before each access that x and y is inbounds. But what if raw xy access like that was private , and instead there's only a visible getter that only accepts an _ib type. That would mean that the caller must make sure to validate that xy is inbounds before accessing the caches. But you're probably right here that this would just be another runtime check anyway when creating the _ib coord. Oh well.

Since you create a variable for the map, you might as well use it on the next line getting the level cache

Thanks, have updated this now. No other usages in this method.

[PatrikLundell]

I'm not opposed to a check resulting in a somewhat controlled failure, such as hiding the data behind a public operation that would perform a check and produce a report (preferably with a call chain) and then either crash after that by raising an exception, or return some garbage (such as the cache data for some legal entry, such as (0,0)). That would have saved a whole lot of effort trying to track this bugger down.

However, none of that beats detecting and handling the issue in a suitable manner at a level where it can actually be done (like you did here).
There was a similar case a while back where a check in a radius around a position could result in a position out of the reality bubble bounds if the position was close to the edges. In that case the proper action was to check and reject the processing for those points, as you did here.

[inogenous]

To reviewers: the above discussions are about a potential overall improvement, for a future change.

I believe this change is ready to merge, since it fixes an actual crash.