This is mostly just adding some Null guards ahead of
code that was already dereferencing pointers, so there
should be no change in behavior.
Also adds one validation of an array index that was
drawing a complaint.
add a static function maybe_destroy_armor() to replace
the DESTROY_ARM() macro.
The DESTROY_ARM() macro expanded to legal code, but did include
side-effects of making assignments to local variable otmp (not an
argument to the macro), referenced local variable atmp (not an
argument to the macro), and sometimes setting the in_use field
on the impacted armor obj.
The assignment statements within the if (...) caused some compilers
and code analyzers to complain and suggest that perhaps
'=' should have been '==', which was incorrect.
The maybe_destroy_armor() function provides the
caller with information about whether the armor resisted,
so that appropriate action can be taken within the caller.
src/restore.c(275): warning: Dereferencing NULL pointer. 'otmp' contains the
same NULL value as 'first' did.
src/restore.c(402): warning: Dereferencing NULL pointer. 'mtmp' contains the
same NULL value as 'first' did.
src/bones.c(646): warning: Using uninitialized memory 'oldbonesid'.
src/bones.c(646): warning: String 'oldbonesid' might not be zero-terminated.
Also help prevent a buffer overflow on corrupt or ill-formed bones.
src/read.c(2889): warning: Dereferencing NULL pointer 'sobj'.
The analyzer doesn't know that the one caller that passes a NULL
sobj argument, angrygods(), checks !Punished before doing so.
Use a NULL guard before dereferencing sobj so that it doesn't
matter anyway.
The reason this wasn't caught sooner was because the version
of the Glyphinfo_at(x,y,glyph) macro, that was defined when
UNBUFFERED_GLYPHINFO is not defined (when glyphinfo's are
buffered - the default), does not use the 3rd argument at
all.
src/weapon.c(451): warning: Dereferencing NULL pointer 'uleft'.
The analyzer couldn't tell that a STRANGE_OBJECT not being made of
SILVER material, was sufficient to guard against dereference of
NULL uleft or uright in the l_ag and r_ag assignments.
Test and dereference each once to set indicator booleans, and use
the booleans afterwards.
src/wield.c(745): warning: Dereferencing NULL pointer 'obj'.
See line 685 for an earlier location where this can occur
In wield_tool(), the comparisons against uwep were intended
for when uwep wasn't null.
gcc/clang analyzers now have some awareness of obj arg being
notnull for wield_tool() since the extern.h prototypes
were changed to declare that, but other compilers/analyzers
do not necessarily have that information, and this:
'if (obj == uwep)'
would be a match if both were NULL.
src/wield.c(254): warning: Dereferencing NULL pointer 'wep'.
See line 190 for an earlier location where this can occur
This seems to be a case where an unnecessary null test (A) caused
the analyzer to call into question whether or not wep
is null at (B):
if (!wep) {
} else if (wep->otyp == CORPSE && cant_wield_corpse(wep)) {
} else if (uarms && bimanual(wep)) {
} else if (!retouch_object(&wep, FALSE)) {
} else {
/* Weapon WILL be wielded after this point */
if (will_weld(wep)) {
} else {
}
if (was_twoweap && !u.twoweap && flags.verbose) {
}
/* KMH -- Talking artifacts are finally implemented */
A ==> if (wep && wep->oartifact) {
}
if (artifact_light(wep) && !wep->lamplit) {
}
B ==> if (wep->unpaid) {
}
}
Removing the extraneous wep test from (A) resolves the complaint.
src/artifact.c(1589): warning C6011: Dereferencing NULL pointer 'magr'.
The 'struct monst *magr' parameter to artifact_hit() can be Null
if 'mdef' is youmonst. mdef is nonnull.
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.
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'.
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)
Before using this updated packaging you will need
to do the following (one time):
sh sys/msdos/fetch-cross-compiler.sh
And you'll need to update your Makefiles as follows.
On Linux:
sh sys/msdos/setup.sh sys/unix/hints/linux.370
or on macOS;
sh sys/msdos/setup.sh sys/unix/hints/macOS.370
Create the msdos package with:
make CROSS_TO_MSDOS=1 package
For now, this will prevent the NONNULLxxx arguments from being
defined under a djgpp compiler or crosscompiler.
paxed reported a segfault under msdos:
nethack.exe
Exiting due to signal SIGSEGV
Page fault at eip=000c3f8c, error=0004
eax=00000000 ebx=00000000 ecx=00000000 edx=0000000f esi=00000000 edi=00000001
ebp=00589988 esp=00589970 program=C:\NH370\NETHACK.EXE
cs: sel=00a7 base=00400000 limit=0063ffff
ds: sel=00af base=00400000 limit=0063ffff
es: sel=00af base=00400000 limit=0063ffff
fs: sel=008f base=00001a20 limit=0000ffff
gs: sel=00bf base=00000000 limit=0010ffff
ss: sel=00af base=00400000 limit=0063ffff
App stack: [00589ba8..00389ba8] Exceptn stack: [00389af4..00387bb4]
Call frame traceback EIPs:
0x000c3f8c _read_config_file+19
0x0017619f _initoptions_finish+577
0x00176371 _initoptions+157
0x0025cec4 _pcmain+365
0x0025d8d9 _main+41
He was able to 'git bisect' to the macro definitions change,
and confirmed that the segfault no longer occurs after this commit.
There may be further investigation on this later.
nhmall