From 1a58ed8de929070ca574f61a335f0cccbc4f12e5 Mon Sep 17 00:00:00 2001 From: Pasi Kallinen Date: Mon, 6 May 2024 17:41:39 +0300 Subject: [PATCH] Rework object deletion Make object deletion work similarly to monster deletion: it's marked for deletion (by setting the where-field to OBJ_DELETED and moved to specific deleted-objects chain), but they're actually freed at the beginning of turn. This may need some more tweaking, especially in places that iterate over object chains, but fuzzing did not find any obvious problems. Fix a case of accessing freed memory: a monster breathed at hero, destroying some items. The code stored the next item in the chain (a cloak), but a ring of levitation was destroyed, causing hero to plop down into lava, destroying the cloak. The item destruction code then tried to access the destroyed cloak object. Make the code check the object where-field - which will be different if the object was marked for deletion. Also removed an extra loop going through the whole object chain looking for the items to destroy. --- include/decl.h | 2 ++ include/extern.h | 1 + include/obj.h | 3 ++- src/allmain.c | 2 ++ src/cmd.c | 1 + src/decl.c | 1 + src/mkobj.c | 34 +++++++++++++++++++++++++++++++--- src/save.c | 1 + src/zap.c | 24 +++++++++++++++--------- 9 files changed, 56 insertions(+), 13 deletions(-) diff --git a/include/decl.h b/include/decl.h index 27b9e384c..69eeddb7f 100644 --- a/include/decl.h +++ b/include/decl.h @@ -733,6 +733,8 @@ struct instance_globals_n { struct instance_globals_o { + struct obj *objs_deleted; + /* dbridge.c */ struct entity occupants[ENTITIES]; diff --git a/include/extern.h b/include/extern.h index b1113340b..4a36834d0 100644 --- a/include/extern.h +++ b/include/extern.h @@ -1635,6 +1635,7 @@ extern void container_weight(struct obj *) NONNULLARG1; extern void dealloc_obj(struct obj *) NONNULLARG1; extern void obj_ice_effects(coordxy, coordxy, boolean); extern long peek_at_iced_corpse_age(struct obj *) NONNULLARG1; +extern void dobjsfree(void); extern int hornoplenty(struct obj *, boolean, struct obj *); extern void obj_sanity_check(void); extern struct obj *obj_nexto(struct obj *); diff --git a/include/obj.h b/include/obj.h index c747dc9d8..08d11dcf8 100644 --- a/include/obj.h +++ b/include/obj.h @@ -81,7 +81,8 @@ struct obj { #define OBJ_BURIED 6 /* object buried */ #define OBJ_ONBILL 7 /* object on shk bill */ #define OBJ_LUAFREE 8 /* object has been dealloc'd, but is ref'd by lua */ -#define NOBJ_STATES 9 +#define OBJ_DELETED 9 /* object is marked for deletion by dobjsfree() */ +#define NOBJ_STATES 10 xint16 timed; /* # of fuses (timers) attached to this obj */ Bitfield(cursed, 1); /* uncursed when neither cursed nor blessed */ diff --git a/src/allmain.c b/src/allmain.c index 3ce39ad38..eb003a4f1 100644 --- a/src/allmain.c +++ b/src/allmain.c @@ -174,6 +174,8 @@ moveloop_core(void) do_positionbar(); #endif + dobjsfree(); + if (gc.context.bypasses) clear_bypasses(); diff --git a/src/cmd.c b/src/cmd.c index bba7805b7..cc9372106 100644 --- a/src/cmd.c +++ b/src/cmd.c @@ -1008,6 +1008,7 @@ makemap_prepost(boolean pre, boolean wiztower) set_uinwater(0); /* u.uinwater = 0 */ u.uundetected = 0; /* not hidden, even if means are available */ dmonsfree(); /* purge dead monsters from 'fmon' */ + dobjsfree(); /* discard current level; "saving" is used to release dynamic data */ zero_nhfile(&tmpnhfp); /* also sets fd to -1 as desired */ diff --git a/src/decl.c b/src/decl.c index d3d0ea8c2..e4f8289e0 100644 --- a/src/decl.c +++ b/src/decl.c @@ -642,6 +642,7 @@ static const struct instance_globals_n g_init_n = { }; static const struct instance_globals_o g_init_o = { + NULL, /* objs_deleted */ /* dbridge.c */ { { 0 } }, /* occupants */ /* decl.c */ diff --git a/src/mkobj.c b/src/mkobj.c index 5416c7d7f..044bdbbd2 100644 --- a/src/mkobj.c +++ b/src/mkobj.c @@ -13,6 +13,7 @@ staticfn void mksobj_init(struct obj *, boolean); staticfn int item_on_ice(struct obj *); staticfn void shrinking_glob_gone(struct obj *); staticfn void obj_timer_checks(struct obj *, coordxy, coordxy, int); +staticfn void dealloc_obj_real(struct obj *); staticfn struct obj *save_mtraits(struct obj *, struct monst *); staticfn void objlist_sanity(struct obj *, int, const char *); staticfn void shop_obj_sanity(struct obj *, const char *); @@ -2478,6 +2479,7 @@ obj_extract_self(struct obj *obj) switch (obj->where) { case OBJ_FREE: case OBJ_LUAFREE: + case OBJ_DELETED: break; case OBJ_FLOOR: remove_object(obj); @@ -2656,7 +2658,7 @@ container_weight(struct obj *object) } /* - * Deallocate the object. _All_ objects should be run through here for + * Mark object to be deallocated. _All_ objects should be run through here for * them to be deallocated. */ void @@ -2695,13 +2697,23 @@ dealloc_obj(struct obj *obj) if (obj == gk.kickedobj) gk.kickedobj = 0; - if (obj->oextra) - dealloc_oextra(obj); if (obj->lua_ref_cnt) { /* obj is referenced from a lua script, let lua gc free it */ obj->where = OBJ_LUAFREE; return; } + /* mark object as deleted, put it into queue to be freed */ + obj->where = OBJ_DELETED; + obj->nobj = go.objs_deleted; + go.objs_deleted = obj; +} + +/* actually deallocate the object */ +staticfn void +dealloc_obj_real(struct obj *obj) +{ + if (obj->oextra) + dealloc_oextra(obj); /* clear out of date information contained in the about-to-become stale memory so that potential used-after-freed bugs (should never @@ -2712,6 +2724,22 @@ dealloc_obj(struct obj *obj) free((genericptr_t) obj); } +/* free all the objects marked for deletion */ +void +dobjsfree(void) +{ + struct obj *otmp = go.objs_deleted; + + while (go.objs_deleted) { + otmp = go.objs_deleted->nobj; + if (go.objs_deleted->where != OBJ_DELETED) + panic("dobjsfree: obj where is not OBJ_DELETED"); + obj_extract_self(go.objs_deleted); + dealloc_obj_real(go.objs_deleted); + go.objs_deleted = otmp; + } +} + /* create an object from a horn of plenty; mirrors bagotricks(makemon.c) */ int hornoplenty( diff --git a/src/save.c b/src/save.c index 71fc6c409..d747db1f3 100644 --- a/src/save.c +++ b/src/save.c @@ -1185,6 +1185,7 @@ freedynamicdata(void) /* move-specific data */ dmonsfree(); /* release dead monsters */ + dobjsfree(); alloc_itermonarr(0U); /* a request of 0 releases existing allocation */ /* level-specific data */ diff --git a/src/zap.c b/src/zap.c index e122a68eb..6e4bd5536 100644 --- a/src/zap.c +++ b/src/zap.c @@ -5849,6 +5849,7 @@ destroy_items( int limit; /* max amount of item stacks destroyed, based on damage */ struct { unsigned oid; + struct obj *otmp; boolean deferred; } items_to_destroy[MAX_ITEMS_DESTROYED]; int elig_stacks = 0; /* number of destroyable objects found so far */ @@ -5856,11 +5857,13 @@ destroy_items( /* this is a struct obj** because we might destroy the first item in it */ struct obj **objchn = u_carry ? &gi.invent : &mon->minvent; int dmg_out = 0; /* damage caused by items getting destroyed */ + xint8 where = NOBJ_STATES; /* initialize items_to_destroy */ for (i = 0; i < MAX_ITEMS_DESTROYED; ++i) { /* 0 should not be a valid o_id for anything */ items_to_destroy[i].oid = 0; + items_to_destroy[i].otmp = (struct obj *) 0; items_to_destroy[i].deferred = FALSE; } @@ -5923,6 +5926,11 @@ destroy_items( continue; } items_to_destroy[i].oid = obj->o_id; + items_to_destroy[i].otmp = obj; + if (where == NOBJ_STATES) + where = obj->where; + else if (where != obj->where) + impossible("destroy_item: items in multiple chains"); /* if loss of this item might dump us onto a trap, hold off until later because potential recursive destroy_items() will @@ -5948,15 +5956,13 @@ destroy_items( /* if we saved some items for later (most likely just a worn ring of levitation) and they're still in inventory, handle them on the second iteration of the loop */ - struct obj *nobj; - for (obj = *objchn; obj; obj = nobj) { - nobj = obj->nobj; - for (i = 0; i < elig_stacks; ++i) { - if (obj->o_id == items_to_destroy[i].oid - && (items_to_destroy[i].deferred == (defer == 1))) { - dmg_out += maybe_destroy_item(mon, obj, dmgtyp); - break; - } + for (i = 0; i < elig_stacks; ++i) { + obj = items_to_destroy[i].otmp; + if (obj && obj->o_id == items_to_destroy[i].oid + && obj->where == where + && (items_to_destroy[i].deferred == (defer == 1))) { + dmg_out += maybe_destroy_item(mon, obj, dmgtyp); + items_to_destroy[i].otmp = (struct obj *) 0; } } }