src/role.c(1543): warning: Reading invalid data from 'roles'.
src/role.c(1765): warning: Reading invalid data from 'roles'.
src/role.c(1780): warning: Reading invalid data from 'races'.
Two variations:
IndexOk(idx, array) validate that idx is a valid index into the array
IndexOkT(idx, array) validate that idx is a valid index into the
array, excluding the final Terminator element
src/mkobj.c(419): warning: '((obj2))->oextra->omonst' could be '0'
: this does not adhere to the specification for the
function 'memcpy'.
src/mkobj.c(421): warning: Dereferencing NULL pointer
'((obj2))->oextra->omonst'.
See line 419 for an earlier location where this can occur
The analyzer was not aware that newoextra() sets up an oextra block:
if (!obj2->oextra)
obj2->oextra = newoextra();
The analyzer was also not aware that newomonst() was setting up a valid
OMONST pointer.
if (!OMONST(obj2))
newomonst(obj2);
Add an assert(has_omonst(obj2)) before copying the content from
OMONST(obj1) into OMONST(obj2).
src/mklev.c(137): warning: Using uninitialized memory 'ri'.
There was a for-loop assigning values to some elements of
ri[], but not all of them.
Initialize the array.
Four kinds of timers are defined but only two have ever been used.
Have sanity checking complain if the other two occur or if 'kind'
doesn't match any of the four.
Also, replacing a perfectly normal use of isok() with an inline test
just to pacify static analysis feels like a slippery slope, so handle
that a little differently.
I reordered the shrink_glob timer to put all object timers together.
Unfortunately that warrants incrementing EDITLEVEL which invalidates
existing save files.
src/options.c(711): warning: Reading invalid data from 'roleoptvals[roleoptindx]'.
Validate the roleoptvals[][] array indexes to appease the static
analyzer.
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.
