fix github issue #596 - wishing exploit

for helm of opposite alignment.

Discovered and described by vultur-cadens.

The #adjust command can be used to split an object stack and if the
shop price of the two halves are different, the new stack will have
its obj->o_id modified to make the prices the same.  That could be
used to tip off the player as to what the low bits of the next o_id
will be.  Since no time passes, no intervening activity such as
random creation of a new monster can take place, so the player could
wish for something that depends on o_id with some degree of control.
Matters mainly for helms of opposite alignment intended to be used
by neutral characters since the player isn't supposed to be able to
control that.  (Other items like T-shirt slogan text and candy bar
wrapper text had a similar issue but controlling those wouldn't have
had any tangible difference on play.)

The issue writeup suggested allowing the player to specify a helm's
alignment during a wish.  That would defeat the purpose of having
o_id affect the helm's behavior in an arbitrary but repeatable way
so is rejected.

I implemented this fix before seeing a followup comment that suggests
using a more sophisticated decision than 'obj->o_id % N' for the
arbitrary effect.  This just increments context.ident for the next
obj->o_id or mon->m_id by 1 or 2 instead of always by 1 and should
be adequate.  It also has the side-effect that two consecutive wishes
for helm of opposite alignment won't necessary give one for each of
the two possible 'polarities', even with no intervening activity by
monsters, reinforcing the lack of player control.

Minor bonus fix:  it moves the incrementing check for wrap-to-0 into
a single place instead of replicating that half a dozen times.  Ones
that should have been there for shop billing and for objects loaded
from bones files were missing.

Fixes #596

doc/fixes37.0

@@ -624,6 +624,12 @@ data tracking for #overview was mis-using u.urooms[] and after being in a
 	levels might flag unvisisted rooms as having been visited
 special damage attacks by the Riders and by fatal-illness inflictors such as
 	Demogorgon did no damage against other monsters, only against the hero
+using obj->o_id to control 'random' behavior of a helm of opposite alignment
+	could potentially be controlled by player when wishing for such
+obj->o_id might be set to invalid value 0 when a partly used up stack had a
+	dummy copy added to a shop's bill or when a bones file was loaded
+	(in theory that could happen on any system but in practice it could
+	only happen on a configuration that uses 16-bit ints)
 
 
 Fixes to 3.7.0-x Problems that Were Exposed Via git Repository

include/extern.h

@@ -1329,6 +1329,7 @@ extern struct obj *mkobj(int, boolean);
 extern int rndmonnum(void);
 extern boolean bogon_is_pname(char);
 extern struct obj *splitobj(struct obj *, long);
+extern unsigned next_ident(void);
 extern struct obj *unsplitobj(struct obj *);
 extern void clear_splitobjs(void);
 extern void replace_object(struct obj *, struct obj *);

src/makemon.c

@@ -837,9 +837,7 @@ clone_mon(struct monst *mon,
     m2->mextra = (struct mextra *) 0;
     m2->nmon = fmon;
     fmon = m2;
-    m2->m_id = g.context.ident++;
-    if (!m2->m_id)
-        m2->m_id = g.context.ident++; /* ident overflowed */
+    m2->m_id = next_ident();
     m2->mx = mm.x;
     m2->my = mm.y;
 
@@ -1205,9 +1203,7 @@ makemon(register struct permonst *ptr,
         mtmp->msleeping = 1;
     mtmp->nmon = fmon;
     fmon = mtmp;
-    mtmp->m_id = g.context.ident++;
-    if (!mtmp->m_id)
-        mtmp->m_id = g.context.ident++; /* ident overflowed */
+    mtmp->m_id = next_ident();
     set_mon_data(mtmp, ptr); /* mtmp->data = ptr; */
     if (ptr->msound == MS_LEADER && quest_info(MS_LEADER) == mndx)
         g.quest_status.leader_m_id = mtmp->m_id;

src/mkobj.c

@@ -358,9 +358,7 @@ copy_oextra(struct obj *obj2, struct obj *obj1)
         OMONST(obj2)->mextra = (struct mextra *) 0;
         OMONST(obj2)->nmon = (struct monst *) 0;
 #if 0
-        OMONST(obj2)->m_id = g.context.ident++;
-        if (OMONST(obj2)->m_id) /* ident overflowed */
-            OMONST(obj2)->m_id = g.context.ident++;
+        OMONST(obj2)->m_id = next_ident();
 #endif
         if (OMONST(obj1)->mextra)
             copy_mextra(OMONST(obj2), OMONST(obj1));
@@ -421,6 +419,34 @@ splitobj(struct obj *obj, long num)
     return otmp;
 }
 
+/* return the value of context.ident and then increment it to be ready for
+   its next use; used to be simple += 1 so that every value from 1 to N got
+   used but now has a random increase that skips half of potential values */
+unsigned
+next_ident(void)
+{
+    unsigned res = g.context.ident;
+
+    /* +rnd(2): originally just +1; changed to rnd() to avoid potential
+       exploit of player using #adjust to split an object stack in a manner
+       that makes most recent ident%2 known; since #adjust takes no time,
+       no intervening activity like random creation of a new monster will
+       take place before next user command; with former +1, o_id%2 of the
+       next object to be created was knowable and player could make a wish
+       under controlled circumstances for an item that is affected by the
+       low bits of its obj->o_id [particularly helm of opposite alignment] */
+    g.context.ident += rnd(2); /* ready for next new object or monster */
+
+    /* if ident has wrapped to 0, force it to be non-zero; if/when it
+       ever wraps past 0 (unlikely, but possible on a configuration which
+       uses 16-bit 'int'), just live with that and hope no o_id conflicts
+       between objects or m_id conflicts between monsters arise */
+    if (!g.context.ident)
+        g.context.ident = rnd(2);
+
+    return res;
+}
+
 /* when splitting a stack that has o_id-based shop prices, pick an
    o_id value for the new stack that will maintain the same price */
 static unsigned
@@ -436,8 +462,9 @@ nextoid(struct obj *oldobj, struct obj *newobj)
             ++oid;
         newdif = oid_price_adjustment(newobj, oid);
     } while (newdif != olddif && --trylimit >= 0);
-    g.context.ident = oid + 1; /* ready for next new object */
-    return oid;
+    g.context.ident = oid; /* update 'last ident used' */
+    (void) next_ident(); /* increment context.ident for next use */
+    return oid; /* caller will use this ident */
 }
 
 /* try to find the stack obj was split from, then merge them back together;
@@ -756,9 +783,7 @@ mksobj(int otyp, boolean init, boolean artif)
     otmp = newobj();
     *otmp = cg.zeroobj;
     otmp->age = g.moves;
-    otmp->o_id = g.context.ident++;
-    if (!otmp->o_id)
-        otmp->o_id = g.context.ident++; /* ident overflowed */
+    otmp->o_id = next_ident();
     otmp->quan = 1L;
     otmp->oclass = let;
     otmp->otyp = otyp;

src/restore.c

@@ -263,7 +263,8 @@ restobjchn(NHFILE* nhfp, boolean frozen)
             otmp2->nobj = otmp;
 
         if (ghostly) {
-            unsigned nid = g.context.ident++;
+            unsigned nid = next_ident();
+
             add_id_mapping(otmp->o_id, nid);
             otmp->o_id = nid;
         }
@@ -404,7 +405,8 @@ restmonchn(NHFILE* nhfp)
             mtmp2->nmon = mtmp;
 
         if (ghostly) {
-            unsigned nid = g.context.ident++;
+            unsigned nid = next_ident();
+
             add_id_mapping(mtmp->m_id, nid);
             mtmp->m_id = nid;
         }

src/shk.c

@@ -2786,7 +2786,7 @@ sub_one_frombill(register struct obj* obj, register struct monst* shkp)
             otmp = newobj();
             *otmp = *obj;
             otmp->oextra = (struct oextra *) 0;
-            bp->bo_id = otmp->o_id = g.context.ident++;
+            bp->bo_id = otmp->o_id = next_ident(); /* g.context.ident++ */
             otmp->where = OBJ_FREE;
             otmp->quan = (bp->bquan -= obj->quan);
             otmp->owt = 0; /* superfluous */