Skip to content

Commit

Permalink
Fix: use-after-free when applying an object that got destroyed
Browse files Browse the repository at this point in the history
Found this running the fuzzer with address sanitizer. After applying an
object, the game checks whether it's a talking artifact so that it can
speak to you afterwards.

If, however, the object got destroyed in the process of applying it,
this will read and dereference obj after they have been freed. I found
this with a cream pie, which is always destroyed when someone applies
it.

To fix this, I tracked the obj pointer for what I think are the only two
cases in the big switch statement that don't track this - royal jelly
and cream pie. Royal jelly is only conditionally destroyed depending on
further input, so its function had to be refactored to take a struct
obj**, but cream pies are unconditionally destroyed so it can just be
set to null.

(xNetHack cherry pick conflict note: royal jelly rub handling had been
moved to a slightly different place in the code.)
  • Loading branch information
copperwater committed May 23, 2023
1 parent 0314561 commit fb42cb0
Showing 1 changed file with 9 additions and 4 deletions.
13 changes: 9 additions & 4 deletions src/apply.c
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ static int set_trap(void); /* occupation callback */
static void display_polearm_positions(int);
static int use_cream_pie(struct obj *);
static int jelly_ok(struct obj *);
static int use_royal_jelly(struct obj *);
static int use_royal_jelly(struct obj **);
static int grapple_range(void);
static boolean can_grapple_location(coordxy, coordxy);
static int use_grapple(struct obj *);
Expand Down Expand Up @@ -1900,7 +1900,7 @@ dorub(void)
return ECMD_OK;
}
} else if (obj->otyp == LUMP_OF_ROYAL_JELLY) {
return use_royal_jelly(obj);
return use_royal_jelly(&obj);
} else if (obj->material == SILVER && Withering) {
if (use_silver_on_withering(obj) == ECMD_TIME) {
return ECMD_TIME;
Expand Down Expand Up @@ -3785,10 +3785,11 @@ jelly_ok(struct obj *obj)
}

static int
use_royal_jelly(struct obj *obj)
use_royal_jelly(struct obj **optr)
{
int oldcorpsenm;
unsigned was_timed;
struct obj *obj = *optr;
struct obj *eobj;

if (obj->quan > 1L)
Expand Down Expand Up @@ -3844,6 +3845,7 @@ use_royal_jelly(struct obj *obj)
/* not useup() because we've already done freeinv() */
setnotworn(obj);
obfree(obj, (struct obj *) 0);
*optr = 0;
return ECMD_TIME;
}

Expand Down Expand Up @@ -4389,9 +4391,10 @@ doapply(void)
break;
case CREAM_PIE:
res = use_cream_pie(obj);
obj = (struct obj *) 0;
break;
case LUMP_OF_ROYAL_JELLY:
res = use_royal_jelly(obj);
res = use_royal_jelly(&obj);
break;
case BULLWHIP:
res = use_whip(obj);
Expand Down Expand Up @@ -4539,6 +4542,8 @@ doapply(void)
pline("Sorry, I don't know how to use that.");
return ECMD_FAIL;
}
/* This assumes that anything that potentially destroyed obj has kept track
* of it and set obj to null before this point. */
if (obj && obj->oartifact) {
res |= arti_speak(obj); /* sets ECMD_TIME bit if artifact speaks */
}
Expand Down

0 comments on commit fb42cb0

Please sign in to comment.