stale lock picking context

Lock context wasn't being cleared if it was for a container and that
container got destroyed.  Case discovered was forcelock() ->
breakchestlock() -> delobj() (sometimes the container is destroyed
rather than just breaking its lock) followed by #wizmakemap (replace
current level) and maybe_reset_pick() trying to check whether
xlock.box was being carried.  But being interrupted, destroying the
container or dropping it down a hole to ship it to another level, then
attempting to resume picking the lock would also find a stale pointer.

diff --git a/doc/fixes36.2 b/doc/fixes36.2
index c5c37abeaa057517c243371c5d6c2eb6d1c74918..1c5b9edfac64f113b946df91ac5445f1ceef9188 100644 (file)
--- a/doc/fixes36.2
+++ b/doc/fixes36.2
@@ -1,4 +1,4 @@
-$NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.241 $ $NHDT-Date: 1548937318 2019/01/31 12:21:58 $
+$NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.242 $ $NHDT-Date: 1548978603 2019/01/31 23:50:03 $
 
 This fixes36.2 file is here to capture information about updates in the 3.6.x
 lineage following the release of 3.6.1 in April 2018. Please note, however,
@@ -357,6 +357,8 @@ smudging of an engraving has been relocated to after a succesful move
        subject to the smudging
 monster with multiple items in inventory could trigger 'dealloc_obj with nobj'
        panic when turned into a statue if separate mon->minvent items merged
+lock picking context could end up with stale container pointer if container
+       being forced/unlocked/locked got destroyed or sent to another level
 
 
 Fixes to Post-3.6.1 Problems that Were Exposed Via git Repository

diff --git a/src/cmd.c b/src/cmd.c
index 75319f5e560d881a64e42c42536ad47956697982..5b77eb1a423b91b4a750a91ca182b6118840b116 100644 (file)
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 cmd.c   $NHDT-Date: 1547512504 2019/01/15 00:35:04 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.328 $ */
+/* NetHack 3.6 cmd.c   $NHDT-Date: 1548978603 2019/01/31 23:50:03 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.330 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /*-Copyright (c) Robert Patrick Rankin, 2013. */
 /* NetHack may be freely redistributed.  See license for details. */
@@ -823,7 +823,7 @@ wiz_makemap(VOID_ARGS)
             unplacebc();
         }
         /* reset lock picking unless it's for a carried container */
-        maybe_reset_pick();
+        maybe_reset_pick((struct obj *) 0);
         /* reset interrupted digging if it was taking place on this level */
         if (on_level(&context.digging.level, &u.uz))
             (void) memset((genericptr_t) &context.digging, 0,

diff --git a/src/do.c b/src/do.c
index 7467356c7e42b444952f5a434260d487833f2b94..20aadf7515c59cfc3b2ad739447ea606b38d4d38 100644 (file)
--- a/src/do.c
+++ b/src/do.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 do.c    $NHDT-Date: 1547680082 2019/01/16 23:08:02 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.186 $ */
+/* NetHack 3.6 do.c    $NHDT-Date: 1548978604 2019/01/31 23:50:04 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.189 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /*-Copyright (c) Derek S. Ray, 2015. */
 /* NetHack may be freely redistributed.  See license for details. */
@@ -1322,7 +1322,7 @@ boolean at_stairs, falling, portal;
        for lock-picking, container may be carried, in which case we
        keep context; if on the floor, it's about to be saved+freed and
        maybe_reset_pick() needs to do its carried() check before that */
-    maybe_reset_pick();
+    maybe_reset_pick((struct obj *) 0);
     reset_trapset(); /* even if to-be-armed trap obj is accompanying hero */
     iflags.travelcc.x = iflags.travelcc.y = 0; /* travel destination cache */
     context.polearm.hitmon = (struct monst *) 0; /* polearm target */

diff --git a/src/lock.c b/src/lock.c
index 0f4c4dbff3515a489863eca1da833ba99403e72e..3e23373519c5ac0675a6ed1e14ba153a55ef2c9d 100644 (file)
--- a/src/lock.c
+++ b/src/lock.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 lock.c  $NHDT-Date: 1547086531 2019/01/10 02:15:31 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.83 $ */
+/* NetHack 3.6 lock.c  $NHDT-Date: 1548978605 2019/01/31 23:50:05 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.84 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /*-Copyright (c) Robert Patrick Rankin, 2011. */
 /* NetHack may be freely redistributed.  See license for details. */
@@ -253,10 +253,14 @@ forcelock(VOID_ARGS)
         return 1; /* still busy */
 
     You("succeed in forcing the lock.");
+    exercise(xlock.picktyp ? A_DEX : A_STR, TRUE);
+    /* breakchestlock() might destroy xlock.box; if so, xlock context will
+       be cleared (delobj -> obfree -> maybe_reset_pick); but it might not,
+       so explicitly clear that manually */
     breakchestlock(xlock.box, (boolean) (!xlock.picktyp && !rn2(3)));
+    reset_pick(); /* lock-picking context is no longer valid */
 
-    exercise((xlock.picktyp) ? A_DEX : A_STR, TRUE);
-    return ((xlock.usedtime = 0));
+    return 0;
 }
 
 void
@@ -264,15 +268,28 @@ reset_pick()
 {
     xlock.usedtime = xlock.chance = xlock.picktyp = 0;
     xlock.magic_key = FALSE;
-    xlock.door = 0;
-    xlock.box = 0;
+    xlock.door = (struct rm *) 0;
+    xlock.box = (struct obj *) 0;
 }
 
-/* level change; don't reset if hero is carrying xlock.box with him/her */
+/* level change or object deletion; context may no longer be valid */
 void
-maybe_reset_pick()
+maybe_reset_pick(container)
+struct obj *container; /* passed from obfree() */
 {
-    if (!xlock.box || !carried(xlock.box))
+    /*
+     * If a specific container, only clear context if it is for that
+     * particular container (which is being deleted).  Other stuff on
+     * the current dungeon level remains valid.
+     * However if 'container' is Null, clear context if not carrying
+     * xlock.box (which might be Null if context is for a door).
+     * Used for changing levels, where a floor container or a door is
+     * being left behind and won't be valid on the new level but a
+     * carried container will still be.  There might not be any context,
+     * in which case redundantly clearing it is harmless.
+     */
+    if (container ? (container == xlock.box)
+                  : (!xlock.box || !carried(xlock.box)))
         reset_pick();
 }
 

diff --git a/src/mkobj.c b/src/mkobj.c
index 0c6b960cae28b4272e402b7f1e583897cff9ebbe..195de0b4a90d4f9a3cab77c55df84cd20f86e948 100644 (file)
--- a/src/mkobj.c
+++ b/src/mkobj.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 mkobj.c $NHDT-Date: 1547086532 2019/01/10 02:15:32 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.141 $ */
+/* NetHack 3.6 mkobj.c $NHDT-Date: 1548978605 2019/01/31 23:50:05 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.142 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /*-Copyright (c) Derek S. Ray, 2015. */
 /* NetHack may be freely redistributed.  See license for details. */
@@ -2076,6 +2076,10 @@ struct obj *obj;
     if (obj->where != OBJ_FREE)
         panic("add_to_migration: obj not free");
 
+    /* lock picking context becomes stale if it's for this object */
+    if (Is_container(obj))
+        maybe_reset_pick(obj);
+
     obj->where = OBJ_MIGRATING;
     obj->nobj = migrating_objs;
     migrating_objs = obj;

diff --git a/src/shk.c b/src/shk.c
index 1549245502fdbc4f1cfa3b6c6c28912bde970459..09892e70911e1b679691077af977314623662838 100644 (file)
--- a/src/shk.c
+++ b/src/shk.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 shk.c   $NHDT-Date: 1547849604 2019/01/18 22:13:24 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.153 $ */
+/* NetHack 3.6 shk.c   $NHDT-Date: 1548978606 2019/01/31 23:50:06 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.154 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /*-Copyright (c) Robert Patrick Rankin, 2012. */
 /* NetHack may be freely redistributed.  See license for details. */
@@ -912,6 +912,8 @@ register struct obj *obj, *merge;
         book_disappears(obj);
     if (Has_contents(obj))
         delete_contents(obj);
+    if (Is_container(obj))
+        maybe_reset_pick(obj);
 
     shkp = 0;
     if (obj->unpaid) {