src/timeout.c(2033): warning: Reading invalid data from 'gl.level.locations'.
Analyzer couldn't tell that isok(x, y) had validated x and y to be
safe indexes into gl.level.locations[x][y].
Code it a bit differently, so that the static analyzer becomes perfectly
aware that the indexes are, indeed, in range.
src/uhitm.c(1172): warning: Reading invalid data from 'mons'.
Analyzer wasn't happy with the index into mons[] array only
being validated by '!= -1'.
Update the check for the index to include the full array
index range, including ensuring that it is also '< NUMMONS'.
Yesterday I said that I'd done all of pager.c and part of objnam.c,
but I was talking about the prototypes in extern.h. This does more
of the same, this time for the local prototypes in pager.c so "all of
pager.c" should be accurate now.
Some functions are passed an obj or monst chain,
and the callers typically don't check them
against 0, so mark them explicitly as NO_NONNULLS
(NO_NONNULLS expands to nothing, but it flags that
some null arg analysis has been done)
Update several places where lazy lastseentyp[] might be an issue.
I think it isn't updated in a timely fashion when newsym() shows
a spot covered by an object or trap, but didn't manage to find any
cases where that caused a problem. This is more in the nature of
a precaution.
Callgrind showed recalc_mapseen was three times more expensive (in terms
of instructions read) than anything else in our codebase. It was being
called in every vision change, re-evaluating the last seen map terrain
type for every map location in sight.
Remove updating the lastseen info in the vision code, and make a small
change so newsym() uses update_lastseentyp.
From my short tests, this seems to work correctly ...
If tutorial is entered, we get following leak on exit:
=================================================================
==81358==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 96 byte(s) in 3 object(s) allocated from:
#0 0x7f6996edefdf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x5601c255bcbb in alloc /home/miku/src/NetHack/src/alloc.c:71
Indirect leak of 5064 byte(s) in 3 object(s) allocated from:
#0 0x7f6996edefdf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x5601c255be1e in alloc /home/miku/src/NetHack/src/alloc.c:71
#2 0x5601c255be1e in dupstr /home/miku/src/NetHack/src/alloc.c:236
SUMMARY: AddressSanitizer: 5160 byte(s) leaked in 6 allocation(s).
Fix this by freeing the cloned selection before returning.
Replace one recenly added 'croom' test with assert(croom != NULL);
keep the other one. Mark fill_ordinary_room() as requiring that its
first argument be non-Null. Check for malformed subroom data before
calling it.
Plus miscellaneous reformatting.
Fix some of the extreme verbosity for null vs non-null triggered
by mklev.c. dungeon_branch() never returns Null.
'#include <assert.h>' should probably be moved out of multiple .c
files and into cstd.h or some such but this doesn't do that.
Checking the callers:
newsym() the use of see_with_infrared() is guarded by
} else if ((mon = m_at(x, y)) != 0 [...]
do_mgivenname() the use of see_with_infrared is guarded by !mtmp:
&& (!mtmp
|| (!sensemon(mtmp)
&& (!(cansee(cx, cy) || see_with_infrared(mtmp))
howmonseen(mon) dereferences mon in other places, so it would
segfault if mon were NULL; howmonseen has NONNULLARG1.
callers were checked:
domove_attackmon_at(mtmp, x, y, displaceu) has mtmp declared nonnull;
there are dereferences of mtmp in the first line of code in
the function.
In domove_core():
The 1st occurrence of is_safemon(mtmp) is guarded by if (mtmp) { }.
The 2nd occurrence of is_safemon(mtmp) is inside an if (mtmp) { } block.
The 3rd occurrence of is_safemon(mtmp) was just remediated by 987be7e8.
In lookaround():
The only occurrence of is_safemon(mtmp) is inside an
if ((mtmp = m_at(x, y)) != 0 [...] { } block.
In do_attack(mtmp), in uhitm.c:
The parameter is declared NONNULLARG1, and the 1st line of
code contains a dereference with mtmp->data, which would
segfault if mtmp were NULL.
Following line 2425 of hack.c, in domove_core():
mtmp = m_at(x, y);
mtmp can be null.
There were two if blocks following that, both of which
only make sense when mtmp is not null.
One of them was explicitly checking for mtmp being non-null,
and the other was avoiding catastrophe by relying on a
hidden check buried within an _is_safepet(mon) macro.
Place both of those blocks into an
if (mtmp) { }
block.
99% of the diff is just indentation.
Checking the callers:
toss_up() would have segfaulted prior to use of stone_missile() if obj were NULL.
thitu() now has a guard prior to use of stone_missile()
ohitmon() would have crashed from earlier dereference otmp->dknown if it were NULL,
otmp arg is declared nonnull
thitm() now has a guard prior to use of stone_missile().
hmon_hitmon_do_hit() null obj takes a different code path than the code path
using stone_missile(); comment asserting that added
The static analyzer complained about use of 'obj' maybe being Null
when used in an impossible warning, but that warning will never
appear for the case where obj is actually Null. Add an assert()
that should let it figure that out, and move the impossible check
inside the 'else' clause where the check matters. (Either of those
by itself ought to be adequate to pacify the analyzer.)
