fix: stack corruption and use-after-free in string/object concatenation

Three related defects in the F_ADD / F_ADD_EQ object branches of
eval_instruction(), the site of the garbage-pointer segfault reported
in #1295 ("str" + ob in a heart_beat, fault inside
EXTEND_SVALUE_STRING of the object's name):

- str += ob popped one stack slot too many: the T_OBJECT branch did
  sp-- where every sibling branch (string/number/real rhs) leaves sp on
  the consumed RHS slot, so the trailing rvalue store
  (assign_svalue_no_free(sp, lval)) wrote one slot below the operands,
  clobbering a live local or temporary -- and F_VOID_ADD_EQ then popped
  a further slot. Any mudlib str += ob corrupted the eval stack;
  misinterpreting a clobbered slot later is a likely source of the
  reported crash. Reproduced deterministically: after s += ob, sibling
  int locals held fragments of unrelated strings.

- "str" + ob and str += ob captured ob->obname and then dropped the
  stack's object reference with free_object() BEFORE appending the
  name. free_object() deallocates a destructed object's last reference
  immediately -- freeing obname with it -- so the append read freed
  memory. (Reachable once destruct2() has released the destruct-list
  reference, i.e. minutes after destruction, with a container holding
  the last reference.) Append first, drop the reference after.

- ob + "str" copied the unbounded obname into a fixed 1024-byte stack
  buffer with sprintf; use snprintf.

Regression test efuns/string_add_object.lpc covers += stack balance
(guard locals detect the clobber and fail on the unfixed driver), the
append-before-free ordering behaviorally, and destructed-object
concatenation semantics.

Fixes #1295

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV55ZRoue2DepeM1q8UNXG

src/vm/internal/base/interpret.cc

@@ -2691,7 +2691,7 @@ void eval_instruction(char* p) {
               case T_OBJECT: {
                 char buff[1024];
                 object_t* ob = (sp - 1)->u.ob;
-                sprintf(buff, "/%s", ob->obname);
+                snprintf(buff, sizeof(buff), "/%s", ob->obname);
                 SVALUE_STRING_ADD_LEFT(buff, "f_add: 3");
                 free_object(&ob, "f_add: 3");
                 break;
@@ -2735,10 +2735,12 @@ void eval_instruction(char* p) {
           case T_OBJECT:
             switch ((sp - 1)->type) {
               case T_STRING: {
-                const char* fname = sp->u.ob->obname;
+                /* Extend before dropping the ref: if the stack holds the last
+                   reference to a destructed object, free_object() deallocates
+                   it immediately, freeing obname with it. */
+                EXTEND_SVALUE_STRING(sp - 1, "/", "f_add: str ob");
+                EXTEND_SVALUE_STRING(sp - 1, sp->u.ob->obname, "f_add: str ob");
                 free_object(&(sp--)->u.ob, "f_add: str+ob");
-                EXTEND_SVALUE_STRING(sp, "/", "f_add: str ob");
-                EXTEND_SVALUE_STRING(sp, fname, "f_add: str ob");
                 break;
               }
               default:
@@ -2793,10 +2795,13 @@ void eval_instruction(char* p) {
               sprintf(buff, "%" LPC_FLOAT_FMTSTR_P, sp->u.real);
               EXTEND_SVALUE_STRING(lval, buff, "f_add_eq: 2");
             } else if (sp->type == T_OBJECT) {
-              const char* fname = sp->u.ob->obname;
-              free_object(&(sp--)->u.ob, "f_add_eq: 2");
-              EXTEND_SVALUE_STRING(lval, "/", "f_add: str ob");
-              EXTEND_SVALUE_STRING(lval, fname, "f_add_eq: 2");
+              /* Extend before dropping the ref (obname dies with the object's
+                 last reference), and leave sp on the consumed RHS slot like
+                 the other branches: the extra sp-- here unbalanced the stack,
+                 making the trailing rvalue store below clobber a live slot. */
+              EXTEND_SVALUE_STRING(lval, "/", "f_add_eq: 2");
+              EXTEND_SVALUE_STRING(lval, sp->u.ob->obname, "f_add_eq: 2");
+              free_object(&sp->u.ob, "f_add_eq: 2");
             } else {
               bad_argument(sp, T_OBJECT | T_STRING | T_NUMBER | T_REAL, 2, instruction);
             }

testsuite/single/tests/efuns/string_add_object.lpc

@@ -0,0 +1,63 @@
+// Regression test for https://github.com/fluffos/fluffos/issues/1295
+// (string/object concatenation in F_ADD / F_ADD_EQ / F_VOID_ADD_EQ).
+//
+// Two defects lived in these opcodes:
+//
+// 1. "str" + ob and str += ob captured ob->obname, then dropped the stack's
+//    object reference with free_object() BEFORE appending the name. If the
+//    stack held the last reference to a destructed object (possible once
+//    destruct2() has released the destruct-list reference, minutes after
+//    destruction), the object -- and obname with it -- was deallocated and
+//    the append read freed memory. destruct2() cannot be forced from a
+//    synchronous testsuite run, so that half is pinned behaviorally: the
+//    append must happen before the reference is dropped.
+//
+// 2. str += ob additionally popped one stack slot too many (the object
+//    branch did sp-- where every sibling branch leaves sp on the consumed
+//    RHS slot), so the trailing rvalue store wrote one slot below the
+//    operands -- clobbering a live local / temporary. The guard variables
+//    below detect exactly that: on the broken driver they end up holding
+//    fragments of unrelated values.
+
+mapping m = ([]);
+
+// Returns the stashed object while removing the mapping's reference.
+object take() {
+  object r = m["k"];
+  map_delete(m, "k");
+  return r;
+}
+
+void do_tests() {
+  object o = new(__FILE__);
+  string expected, s, t;
+  mixed u;
+  int guard1 = 111;
+  int guard2 = 222;
+
+  // Baseline: "str" + live object appends "/" + obname.
+  expected = "x" + o;
+  ASSERT(stringp(expected));
+  ASSERT_EQ("x/" + file_name(o)[1..], expected);
+
+  // str += ob (statement => F_VOID_ADD_EQ): must not unbalance the stack.
+  s = "x";
+  s += o;
+  ASSERT_EQ(expected, s);
+
+  // str += ob as an rvalue (F_ADD_EQ): result lands in the right slot and
+  // the guards/live locals survive.
+  t = "x";
+  u = (t += o);
+  ASSERT_EQ(expected, t);
+  ASSERT_EQ(expected, u);
+  ASSERT_EQ(111, guard1);
+  ASSERT_EQ(222, guard2);
+
+  // A destructed object read back out of a container is fetched as 0, so
+  // concatenation sees a number, not a stale object.
+  m["k"] = o;
+  destruct(o);
+  ASSERT_EQ("x0", "x" + take());
+  ASSERT_EQ(0, m["k"]);
+}