fix #H7686 - destroy_item()'s inventory traversal

Inventory traversal can be disrupted when items being traversed are
able to change inventory.  I've lost track of how many times this
sort of thing has been discovered.

Report claimed that boiled potion of polymorph caused transformation
which resulted in dropped weapon and dropped or destroyed worn armor.
That was evidently a guess; potionbreathe() for that potion only
abuses constitution.  The traceback showed 'you_were()' was involved.
Boiled potion of unholy water triggers human-to-beast transformation
of hero inflicted with lycanthropy, yielding similar situation.

I didn't notice anything unusual when reproducing this but inventory
was definitely vulnerable.  My 'one line' fixes entries are steadily
getting to be more verbose; I may have to go back to 'fix bug'.  :-}

diff --git a/doc/fixes36.2 b/doc/fixes36.2
index f87856d44340cda353132b2a5cd0f5c6e6eae514..7f0673f39347df4be2a55e598512a9b407c6b336 100644 (file)
--- a/doc/fixes36.2
+++ b/doc/fixes36.2
@@ -243,6 +243,14 @@ level change after being interruped locking or unlocking a container might
 if a restore attempt failed and a new game was started instead, it would use
        stale context from old game if restoration got far enough to load that
 if hero survives turning into slime (life-saving), survive as a green slime
+hero hit by something that causes inventory items to be destroyed with loss of
+       any of those causing other inventory items to be dropped or destroyed,
+       inventory traversal became unreliable (known sequence: potions hit by
+       fire then breahing vapor from boiled unholy water triggering were
+       transformation to beast form; possible sequence: ring of levitation
+       blasted by lightning and dropping hero onto fire trap); [3.6.1 fixed a
+       similar problem with more obvious symptom, an "object lost" panic when
+       the unholy water was wielded; the fix for that wasn't general enough]
 
 
 Fixes to Post-3.6.1 Problems that Were Exposed Via git Repository

diff --git a/src/zap.c b/src/zap.c
index 5a5a67c34e0802fa9c71654e9329417cf3d25813..8f7adf33878ecce433c0f7767e929d98630a2156 100644 (file)
--- a/src/zap.c
+++ b/src/zap.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 zap.c   $NHDT-Date: 1543744276 2018/12/02 09:51:16 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.299 $ */
+/* NetHack 3.6 zap.c   $NHDT-Date: 1544146046 2018/12/07 01:27:26 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.300 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /*-Copyright (c) Robert Patrick Rankin, 2013. */
 /* NetHack may be freely redistributed.  See license for details. */
@@ -4719,15 +4719,32 @@ void
 destroy_item(osym, dmgtyp)
 register int osym, dmgtyp;
 {
-    register struct obj *obj, *obj2;
+    register struct obj *obj;
     int dmg, xresist, skip;
     long i, cnt, quan;
     int dindx;
     const char *mult;
     boolean physical_damage;
 
-    for (obj = invent; obj; obj = obj2) {
-        obj2 = obj->nobj;
+    /*
+     * Sometimes destroying an item can change inventory aside from the
+     * item itself (cited case was a potion of polymorph; when destroyed,
+     * potion_breathe() caused hero to transform and that resulted in
+     * destruction of some worn armor).  Unlike other uses of the object
+     * bybass mechanism, destroy_item() can be called multiple times for
+     * same event.  So we have to explicitly clear it before each use and
+     * hope no other section of code expects it to retain previous value.
+     *
+     * FIXME?  Destruction of a ring of levitation could drop hero onto
+     * a fire trap which could destroy other items and we'll get called
+     * recursively.  This should still work, but items beyond the ring
+     * which survive the fire will be marked as already processed by the
+     * inner call, so will always survive the remainder of the outer call
+     * instead of being subjected to original chance of destruction.
+     */
+    bypass_objlist(invent, FALSE); /* clear bypass bit for invent */
+
+    while ((obj = nxt_unbypassed_obj(invent)) != 0) {
         physical_damage = FALSE;
         if (obj->oclass != osym)
             continue; /* test only objs of type osym */
@@ -4820,6 +4837,7 @@ register int osym, dmgtyp;
             skip++;
             break;
         }
+
         if (!skip) {
             if (obj->in_use)
                 --quan; /* one will be used up elsewhere */
@@ -4852,9 +4870,9 @@ register int osym, dmgtyp;
             for (i = 0; i < cnt; i++)
                 useup(obj);
             if (dmg) {
-                if (xresist)
+                if (xresist) {
                     You("aren't hurt!");
-                else {
+                } else {
                     const char *how = destroy_strings[dindx][2];
                     boolean one = (cnt == 1L);
 
@@ -4877,7 +4895,7 @@ destroy_mitem(mtmp, osym, dmgtyp)
 struct monst *mtmp;
 int osym, dmgtyp;
 {
-    struct obj *obj, *obj2;
+    struct obj *obj;
     int skip, tmp = 0;
     long i, cnt, quan;
     int dindx;
@@ -4889,8 +4907,11 @@ int osym, dmgtyp;
     }
 
     vis = canseemon(mtmp);
-    for (obj = mtmp->minvent; obj; obj = obj2) {
-        obj2 = obj->nobj;
+
+    /* see destroy_item(); object destruction could disrupt inventory list */
+    bypass_objlist(mtmp->minvent, FALSE); /* clear bypass bit for invent */
+
+    while ((obj = nxt_unbypassed_obj(mtmp->minvent)) != 0) {
         if (obj->oclass != osym)
             continue; /* test only objs of type osym */
         skip = 0;