Skip to content

Commit 488cc92

Browse files
committed
Fix: multiple issues in allmain.c found via code review.
Mixed bag of defensive fixes, one potential crash, and cleanup landed in moveloop() and argcheck(): * src/allmain.c: in the pet-overage untaming loop, skip monsters with isminion set. EDOG() expands to mextra->edog, which is NULL for monsters that carry emin instead, so the abuse read at `rn2(EDOG(weakdog)->abuse + 1)` would NULL-deref. Not currently reachable on Astral (EvilHack spawns the Red Horse via MM_EDOG rather than a guardian angel with emin), but matches the canonical `mtame && !isminion` invariant used at mon.c:107 and ~16 other sites. * src/allmain.c: in the Gauntlets of Purity force-disarm block, clear u.twoweap before dropping both weapons. dropz() clears the uwep/uswapwep globals but never touches u.twoweap, so the two-weapon path was leaving a stale u.twoweap=TRUE with both weapon slots NULL. Any later guard of the form `u.twoweap && uswapwep->...` (flees_light, the elf/orc regen checks in this same function, weapon property reads) would NULL-deref on the next read. * src/allmain.c: add set_malign() after clearing mpeaceful in the pet-overage untame path, and after the tame/peaceful branch in the banes anger block. setmangry() early-returns when mtame is still set, so in both cases malign was never resynced after the transition to hostile. * src/allmain.c: add (void) casts to the six dropx() calls in the Gauntlets of Purity block. dropx() returns boolean (TRUE when flooreffects destroyed the object) and was being silently discarded; no immediate UAF since nothing dereferences the object afterwards, but aligns with the surrounding (void) Shield_off() / (void) Gloves_off() style and the convention established by prior dropy/dropx audits. * src/allmain.c: fix -version:foo error path in argcheck() to return 2 (exit) instead of TRUE (continue), matching the success path and the other exit-on-error arguments. Previous behavior printed the diagnostic and then launched the game, scrolling the error off-screen. * src/allmain.c: ARG_WINDOWS case without an extended_opt now returns 1, matching the "found and skip" contract documented at the top of argcheck(). Previously fell through the switch and returned FALSE despite a successful match. * src/allmain.c: retype debug_fields() parameter from `const char *` to `char *` to match its actual behavior - the body writes through the pointer at both the comma-splitting site and the trailing-whitespace strip. Callers pass argv, so the cast-through-const was harmless but dishonest. Drop the `(char *)` cast on the eos() call now that it is unneeded. * src/allmain.c: convert the `while` loop at the top of debug_fields() to `if`. After `*op++ = 0` cuts the string at the first comma, opts has no further commas, so the loop always exits after one iteration - the comma-list walk is actually driven by the recursive debug_fields(op) call. * src/allmain.c: cache elf_can_regen() and orc_can_regen() per-iteration. Each helper does a full worn-inventory scan for iron/mithril and was being called up to three times per player turn in the regen-state gate block. * src/allmain.c: remove stray `;` after the switch in argcheck(). Harmless empty statement.
1 parent de3c433 commit 488cc92

2 files changed

Lines changed: 50 additions & 30 deletions

File tree

doc/evilhack-changelog.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5210,4 +5210,5 @@ The following changes to date are:
52105210
- Fix: multiple issues in wield.c found via code review
52115211
- Fix: use-after-free in hitmu() when rustm() destroys attacker's
52125212
weapon
5213+
- Fix: multiple issues in allmain.c found via code review
52135214

src/allmain.c

Lines changed: 49 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ STATIC_DCL void NDECL(do_positionbar);
1717
#endif
1818
STATIC_DCL void FDECL(regen_hp, (int));
1919
STATIC_DCL void FDECL(interrupt_multi, (const char *));
20-
STATIC_DCL void FDECL(debug_fields, (const char *));
20+
STATIC_DCL void FDECL(debug_fields, (char *));
2121
STATIC_DCL void NDECL(init_mchest);
2222

2323
#ifdef EXTRAINFO_FN
@@ -371,12 +371,16 @@ boolean resuming;
371371
int numdogs = 0;
372372
int num_candidates = 0;
373373
int limit = ACURR(A_CHA) / 3;
374+
/* cap: overflow is self-correcting across turns */
374375
struct monst *candidates[50]; /* untameable pets (excludes steed) */
375376
struct monst *curmon;
376377

377378
/* single pass: count all pets, collect untameable candidates */
378379
for (curmon = fmon; curmon; curmon = curmon->nmon) {
379-
if (curmon->mtame && !curmon->msummoned) {
380+
/* skip minions: they carry emin rather than edog,
381+
so EDOG() below would NULL-deref */
382+
if (curmon->mtame && !curmon->msummoned
383+
&& !curmon->isminion) {
380384
numdogs++;
381385
/* steed counts toward limit but is never untamed */
382386
if (curmon != u.usteed && num_candidates < 50)
@@ -413,6 +417,7 @@ boolean resuming;
413417
at random, become hostile */
414418
if (rn2(EDOG(weakdog)->abuse + 1) || !rn2(3)) {
415419
weakdog->mpeaceful = 0;
420+
set_malign(weakdog);
416421
newsym(weakdog->mx, weakdog->my);
417422
}
418423
if (weakdog->mleashed)
@@ -760,6 +765,9 @@ boolean resuming;
760765
} else if (mtmp->mpeaceful || mtmp->mtame) {
761766
setmangry(mtmp, FALSE);
762767
mtmp->mpeaceful = mtmp->mtame = 0;
768+
/* setmangry early-returns if mtame was set,
769+
so ensure malign resyncs */
770+
set_malign(mtmp);
763771
newsym(mtmp->mx, mtmp->my); /* update display */
764772
if (mtmp->mleashed)
765773
m_unleash(mtmp, TRUE);
@@ -856,22 +864,27 @@ boolean resuming;
856864
curs_on_u();
857865
}
858866

859-
if (elf_regen != elf_can_regen()) {
860-
if (!Hallucination)
861-
You_feel("%s.", (elf_regen) ? "itchy" : "relief");
862-
else
863-
You_feel("%s.", (elf_can_regen()) ? "magnetic"
864-
: "like... gnarly dude");
865-
elf_regen = elf_can_regen();
866-
}
867+
{
868+
boolean can_elf_now = elf_can_regen();
869+
boolean can_orc_now = orc_can_regen();
870+
871+
if (elf_regen != can_elf_now) {
872+
if (!Hallucination)
873+
You_feel("%s.", (elf_regen) ? "itchy" : "relief");
874+
else
875+
You_feel("%s.", can_elf_now ? "magnetic"
876+
: "like... gnarly dude");
877+
elf_regen = can_elf_now;
878+
}
867879

868-
if (orc_regen != orc_can_regen()) {
869-
if (!Hallucination)
870-
You_feel("%s.", (orc_regen) ? "tingly" : "relief");
871-
else
872-
You_feel("%s.", (orc_can_regen()) ? "non-magnetic"
873-
: "like... whoa");
874-
orc_regen = orc_can_regen();
880+
if (orc_regen != can_orc_now) {
881+
if (!Hallucination)
882+
You_feel("%s.", (orc_regen) ? "tingly" : "relief");
883+
else
884+
You_feel("%s.", can_orc_now ? "non-magnetic"
885+
: "like... whoa");
886+
orc_regen = can_orc_now;
887+
}
875888
}
876889

877890
/* If wielding the Wand of Orcus and it's been uncursed,
@@ -903,24 +916,28 @@ boolean resuming;
903916
Your("%s and %s are forced from your %s!",
904917
simpleonames(uwep), simpleonames(uswapwep),
905918
makeplural(body_part(HAND)));
906-
dropx(uswapwep);
907-
dropx(uwep);
919+
/* clear before dropx(): dropz() clears the uwep/uswapwep
920+
globals but never touches u.twoweap, leaving a stale
921+
u.twoweap=TRUE with both slots NULL */
922+
u.twoweap = FALSE;
923+
(void) dropx(uswapwep);
924+
(void) dropx(uwep);
908925
} else if (uwep && (was_shield = uarms)) {
909926
Your("%s and %s are forced from your %s!",
910927
simpleonames(uwep), simpleonames(uarms),
911928
makeplural(body_part(HAND)));
912929
(void) Shield_off();
913-
dropx(uwep);
914-
dropx(was_shield);
930+
(void) dropx(uwep);
931+
(void) dropx(was_shield);
915932
} else if (!uwep && (was_shield = uarms)) {
916933
Your("%s is forced from your %s!",
917934
simpleonames(uarms), body_part(HAND));
918935
(void) Shield_off();
919-
dropx(was_shield);
936+
(void) dropx(was_shield);
920937
} else if (uwep) {
921938
Your("%s is forced from your %s!",
922939
simpleonames(uwep), body_part(HAND));
923-
dropx(uwep);
940+
(void) dropx(uwep);
924941
}
925942
}
926943

@@ -1485,7 +1502,7 @@ enum earlyarg e_arg;
14851502
}
14861503

14871504
if (match) {
1488-
const char *extended_opt = index(userea, ':');
1505+
char *extended_opt = index(userea, ':');
14891506

14901507
if (!extended_opt)
14911508
extended_opt = index(userea, '=');
@@ -1507,7 +1524,7 @@ enum earlyarg e_arg;
15071524
raw_printf(
15081525
"-%sversion can only be extended with -%sversion:paste.\n",
15091526
dashdash, dashdash);
1510-
return TRUE;
1527+
return 2;
15111528
}
15121529
}
15131530
early_version_info(insert_into_pastebuf);
@@ -1526,12 +1543,13 @@ enum earlyarg e_arg;
15261543
extended_opt++;
15271544
return windows_early_options(extended_opt);
15281545
}
1546+
return 1;
15291547
}
15301548
#endif
15311549
default:
15321550
break;
15331551
}
1534-
};
1552+
}
15351553
return FALSE;
15361554
}
15371555

@@ -1549,14 +1567,15 @@ enum earlyarg e_arg;
15491567
*/
15501568
STATIC_OVL void
15511569
debug_fields(opts)
1552-
const char *opts;
1570+
char *opts;
15531571
{
15541572
char *op;
15551573
boolean negated = FALSE;
15561574

1557-
while ((op = index(opts, ',')) != 0) {
1575+
/* split on first comma and recurse on the remainder; after the
1576+
cut, opts has no further commas so this is effectively an 'if' */
1577+
if ((op = index(opts, ',')) != 0) {
15581578
*op++ = 0;
1559-
/* recurse */
15601579
debug_fields(op);
15611580
}
15621581
if (strlen(opts) > BUFSZ / 2)
@@ -1565,7 +1584,7 @@ const char *opts;
15651584
/* strip leading and trailing white space */
15661585
while (isspace((uchar) *opts))
15671586
opts++;
1568-
op = eos((char *) opts);
1587+
op = eos(opts);
15691588
while (--op >= opts && isspace((uchar) *op))
15701589
*op = '\0';
15711590

0 commit comments

Comments
 (0)