fix odd messages caused by buffer re-use

Reported and diagnosed by entrez:
"The <mon> yanks <two-handed weapon> from your corpses!"

It became unwielded and that triggered a perm_invent update and
such updates reformat entire inventory, so if that contains a dozen
or more items it will use all the obuf[] static buffers as least
once.  In this case, the bullwhip code had plural of "hand" in one
of those buffers and by the time it delivered the message which
used that, the value had been clobbered.

As the diagnosis mentioned, it can be tricky to reproduce since
either &obuf[0] or &obuf[PREFIX] might be used and if the value
being clobbered didn't overlap, the effect wasn't noticeable.

Instead of fixing the bullwhip message, this changes inventory
display so that it should no longer churn through all the buffers.

It also adds a fixes entry for #K3401, which was already fixed for
3.7 but I hadn't been able to reproduce it for 3.6.x (which I now
blame on the PREFIX trickiness).

diff --git a/doc/fixes37.0 b/doc/fixes37.0
index 78518f30317c5f7ab7df766b72afda4f60c67183..6c84ddbfca9569a31caaf3db1cc0de51b0b5b45f 100644 (file)
--- a/doc/fixes37.0
+++ b/doc/fixes37.0
@@ -607,6 +607,16 @@ yet another fix for display problems during restore:  if game is saved while
        instead it would access steed pointer before that has been set up
 resistances gained from worn or wielded items also protect hero's inventory
 non-metallic gloves protect worn rings from shock
+message "Oops!  food rations out of your grasp!" occurred due to perm_invent
+       in mid-operation overwriting all of xname's/doname's obufs; fixed by
+       having hold_another_object() defer perm_invent update til done with
+       all its args (so fixed as a side-effect of "spurious add and remove
+       perm_invent updates" above, prior to being reported as #K3401)
+similar "The ogre lord yanks Cleaver from your corpses!" due to caching the
+       result from makeplural(body_part(HAND)) then having a perm_invent
+       update clobber that; fixed by having inventory display release the
+       obuf used for each item so that the same one will be reused for the
+       next item, to avoid churning through the whole pool of obufs
 
 
 Fixes to 3.7.0-x Problems that Were Exposed Via git Repository

diff --git a/include/extern.h b/include/extern.h
index 27ce86eafcd3ce955860a5aab9d08e886a452d2f..5ea0b0eb6fa7f36de6b6484b62417a55ade533f0 100644 (file)
--- a/include/extern.h
+++ b/include/extern.h
@@ -1762,6 +1762,7 @@ extern void objects_globals_init(void);
 
 /* ### objnam.c ### */
 
+extern void maybereleaseobuf(char *);
 extern char *obj_typename(int);
 extern char *simple_typename(int);
 extern char *safe_typename(int);

diff --git a/src/invent.c b/src/invent.c
index 14b222fe908fa2d614b540c1cf904267f8a08203..afd5efe09137c5364f0222127c382e0486f19f36 100644 (file)
--- a/src/invent.c
+++ b/src/invent.c
@@ -2535,7 +2535,7 @@ display_pickinv(
 {
     static const char not_carrying_anything[] = "Not carrying anything";
     struct obj *otmp, wizid_fakeobj;
-    char ilet, ret;
+    char ilet, ret, *formattedobj;
     const char *invlet = flags.inv_order;
     int n, classcount;
     winid win;                        /* windows being used */
@@ -2697,9 +2697,14 @@ display_pickinv(
                 any.a_char = ilet;
             tmpglyph = obj_to_glyph(otmp, rn2_on_display_rng);
             map_glyphinfo(0, 0, tmpglyph, 0U, &tmpglyphinfo);
+            formattedobj = doname(otmp);
             add_menu(win, &tmpglyphinfo, &any, ilet,
                      wizid ? def_oc_syms[(int) otmp->oclass].sym : 0,
-                     ATR_NONE, doname(otmp), MENU_ITEMFLAGS_NONE);
+                     ATR_NONE, formattedobj, MENU_ITEMFLAGS_NONE);
+            /* doname() uses a static pool of obuf[] output buffers and
+               we don't want inventory display to overwrite all of them,
+               so when we've used one we release it for re-use */
+            maybereleaseobuf(formattedobj);
             gotsomething = TRUE;
         }
     }

diff --git a/src/objnam.c b/src/objnam.c
index d8ea61c2b58a80c32ce9584ec7cf58800473c721..cd9274c8b78763872e1f0c74fe15cc8f21660599 100644 (file)
--- a/src/objnam.c
+++ b/src/objnam.c
@@ -128,6 +128,44 @@ releaseobuf(char *bufp)
         obufidx = (obufidx - 1 + NUMOBUF) % NUMOBUF;
 }
 
+/* used by display_pickinv (invent.c, main whole-inventory routine) to
+   release each successive doname() result in order to try to avoid
+   clobbering all the obufs when 'perm_invent' is enabled and updated
+   while one or more obufs have been allocated but not released yet */
+void
+maybereleaseobuf(char *obuffer)
+{
+    releaseobuf(obuffer);
+
+    /*
+     * An example from 3.6.x where all obufs got clobbered was when a
+     * monster used a bullwhip to disarm the hero of a two-handed weapon:
+     * "The ogre lord yanks Cleaver from your corpses!"
+     |
+     | hand = body_part(HAND);
+     | if (use_plural)      // switches 'hand' from static buffer to an obuf
+     |   hand = makeplural(hand);
+      ...
+     | release_worn_item(); // triggers full inventory update for perm_invent
+      ...
+     | pline(..., hand);    // the obuf[] for "hands" was clobbered with the
+     |                      //+ partial formatting of an item from invent
+     *
+     * Another example was from writing a scroll without room in invent to
+     * hold it after being split from a stack of blank scrolls:
+     * "Oops!  food rations out of your grasp!"
+     * hold_another_object() was passed 'the(aobjnam(newscroll, "slip"))'
+     * as an argument and that should have yielded
+     * "Oops!  The scroll of <foo> slips out of your grasp!"
+     * but attempting to add the item to inventory triggered update for
+     * perm_invent and the result from 'the(...)' was clobbered by partial
+     * formatting of some inventory item.  [It happened in a shop and the
+     * shk claimed ownership of the new scroll, but that wasn't relevant.]
+     * That got fixed earlier, by delaying update_inventory() during
+     * hold_another_object() rather than by avoiding using all the obufs.
+     */
+}
+
 char *
 obj_typename(int otyp)
 {
@@ -434,6 +472,7 @@ xname_flags(
     unsigned cxn_flags) /* bitmask of CXN_xxx values */
 {
     register char *buf;
+    char *obufp;
     register int typ = obj->otyp;
     register struct objclass *ocl = &objects[typ];
     int nn = ocl->oc_name_known, omndx = obj->corpsenm;
@@ -574,10 +613,18 @@ xname_flags(
             } else {
                 Strcpy(buf, f->fname);
                 if (pluralize) {
-                    /* ick; already pluralized fruit names
-                       are allowed--we want to try to avoid
-                       adding a redundant plural suffix */
-                    Strcpy(buf, makeplural(makesingular(buf)));
+                    /* ick: already pluralized fruit names are allowed--we
+                       want to try to avoid adding a redundant plural suffix;
+                       double ick: makesingular() and makeplural() each use
+                       and return an obuf but we don't want any particular
+                       xname() call to consume more than one of those
+                       [note: makeXXX() will be fully evaluated and done with
+                       'buf' before strcpy() touches its output buffer] */
+                    Strcpy(buf, obufp = makesingular(buf));
+                    releaseobuf(obufp);
+                    Strcpy(buf, obufp = makeplural(buf));
+                    releaseobuf(obufp);
+
                     pluralize = FALSE;
                 }
             }
@@ -729,8 +776,11 @@ xname_flags(
         Sprintf(buf, "glorkum %d %d %d", obj->oclass, typ, obj->spe);
         break;
     }
-    if (pluralize)
-        Strcpy(buf, makeplural(buf));
+    if (pluralize) {
+        /* (see fruit name handling in case FOOD_CLASS above) */
+        Strcpy(buf, obufp = makeplural(buf));
+        releaseobuf(obufp);
+    }
 
     /* maybe give some extra information which isn't shown during play */
     if (g.program_state.gameover) {
@@ -1486,11 +1536,15 @@ corpse_xname(
         }
     }
 
-    /* it's safe to overwrite our nambuf after an() has copied
-       its old value into another buffer */
-    if (any_prefix)
-        Strcpy(nambuf, an(nambuf));
+    /* it's safe to overwrite our nambuf[] after an() has copied its
+       old value into another buffer; and once _that_ has been copied,
+       the obuf[] returned by an() can be made available for re-use */
+    if (any_prefix) {
+        char *obufp;
 
+        Strcpy(nambuf, obufp = an(nambuf));
+        releaseobuf(obufp);
+    }
     return nambuf;
 }
 
@@ -1949,10 +2003,12 @@ Ysimple_name2(struct obj* obj)
 char *
 simpleonames(struct obj* obj)
 {
-    char *simpleoname = minimal_xname(obj);
+    char *obufp, *simpleoname = minimal_xname(obj);
 
-    if (obj->quan != 1L)
-        simpleoname = makeplural(simpleoname);
+    if (obj->quan != 1L) {
+        simpleoname = makeplural(obufp = simpleoname);
+        releaseobuf(obufp);
+    }
     return simpleoname;
 }
 
@@ -1960,7 +2016,7 @@ simpleonames(struct obj* obj)
 char *
 ansimpleoname(struct obj* obj)
 {
-    char *simpleoname = simpleonames(obj);
+    char *obufp, *simpleoname = simpleonames(obj);
     int otyp = obj->otyp;
 
     /* prefix with "the" if a unique item, or a fake one imitating same,
@@ -1969,12 +2025,16 @@ ansimpleoname(struct obj* obj)
     if (otyp == FAKE_AMULET_OF_YENDOR)
         otyp = AMULET_OF_YENDOR;
     if (objects[otyp].oc_unique
-        && !strcmp(simpleoname, OBJ_NAME(objects[otyp])))
-        return the(simpleoname);
-
-    /* simpleoname is singular if quan==1, plural otherwise */
-    if (obj->quan == 1L)
-        simpleoname = an(simpleoname);
+        && !strcmp(simpleoname, OBJ_NAME(objects[otyp]))) {
+        /* the() will allocate another obuf[]; we want to avoid using two */
+        Strcpy(simpleoname, obufp = the(simpleoname));
+        releaseobuf(obufp);
+    } else if (obj->quan == 1L) {
+        /* simpleoname[] is singular if quan==1, plural otherwise;
+           an() will allocate another obuf[]; we want to avoid using two */
+        Strcpy(simpleoname, obufp = an(simpleoname));
+        releaseobuf(obufp);
+    }
     return simpleoname;
 }
 
@@ -1982,9 +2042,12 @@ ansimpleoname(struct obj* obj)
 char *
 thesimpleoname(struct obj* obj)
 {
-    char *simpleoname = simpleonames(obj);
+    char *obufp, *simpleoname = simpleonames(obj);
 
-    return the(simpleoname);
+    /* the() will allocate another obuf[]; we want to avoid using two */
+    Strcpy(simpleoname, obufp = the(simpleoname));
+    releaseobuf(obufp);
+    return simpleoname;
 }
 
 /* artifact's name without any object type or known/dknown/&c feedback */