Fix object-destruct UAF and compiler_tests string-table corruption (#1288)

* object: fix dealloc_object() leaving a dangling obj_list_destruct head

destruct_object() (simulate.cc) unconditionally pushes the object onto
the front of the global obj_list_destruct queue -- the list of objects
destructed but not yet swept by remove_destructed_objects(). But
dealloc_object() never unlinked the object from that queue when its
ref count dropped to 0, so if the object's last reference (or a stray
reference reclaim_objects() finds) drops before the next sweep runs,
obj_list_destruct is left pointing at freed memory -- the very next
destruct_object() call anywhere then writes through it
(obj_list_destruct->prev_all = ob), a heap-use-after-free confirmed
with AddressSanitizer.

In DEBUG builds, obj_list_destruct happens to share the object's
next_all/prev_all storage with the separate (correctly-maintained)
obj_list_dangling leak-hunting list, so the existing dangling-unlink
logic already keeps the underlying chain structure correct as a side
effect; only obj_list_destruct's own head *variable* needed updating,
added as three lines inside the existing #ifdef DEBUG block. Non-DEBUG
builds have no such side effect, so a full (head + general neighbor
relink) unlink was added there instead -- needed for the same bug to
be fixed in a plain build, and for the mid-chain case reclaim_objects()
can trigger independent of any sweep.

Two regression tests in test_lpc.cc reproduce both shapes (head and
mid-chain) via destruct_object()+free_object(); both crash under ASan
on the unfixed binary at exactly the reported call site
(simulate.cc:1555, destruct_object) and pass cleanly with the fix.
Verified on RelWithDebInfo and Debug+ASan/UBSan: full LPC testsuite
(608 files) passes on both, twice each; all GTest suites pass.

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

* compiler_tests: fix cross-test shared-string table corruption

Running the compiler_tests GTest binary in full reliably crashed with
"stralloc.c: free_string called on non-shared string: /test" during
the first real driver boot's compile of single/simul_efun.lpc. Root
cause (two independent, compounding bugs, both fixed):

1. TokenizeSession() (the lightweight, no-full-boot tokenizer test
   harness) calls start_new_file(), which arms lexer_utils.cc's
   file-local `main_filename` pin (a make_shared_string() ref) on the
   first call of the whole process -- but TokenizeSession never called
   the matching end_new_file() to disarm it, so main_filename stayed
   permanently pinned to that first session's filename ("/test") for
   the rest of the process, and stopped tracking any later top-level
   compile's own filename as intended.

2. init_strings() (base/internal/stralloc.cc) has no guard against
   being called twice: the real driver boot's vm_init() calls it
   unconditionally, silently reallocating the shared-string hash table
   and orphaning everything interned so far -- including the leaked
   "/test" pin from (1) -- without freeing or migrating the old table.
   The orphaned block is never corrupted or freed early (confirmed via
   a live gdb backtrace: perfectly valid refs/size at the crash site),
   it simply becomes unreachable from the fresh table. The first
   free_string() against it (the first real end_new_file() call of the
   process, hit when single/simul_efun.lpc's `inherit "std/json"`
   can't yet resolve and epilog()'s abort path runs) then fails its
   "is this still in the table" check and aborts.

Fixed both: TokenizeSession() now calls end_new_file() symmetrically
with start_new_file(), and init_strings() is idempotent (skips with a
debug_message() if already initialized) as defense in depth against
the same class of bug from any other leaked pin -- directly
implementing the "whichever initializes first wins" invariant the
surrounding code comment already documented but only half-enforced.

Verified via the minimal reproducing filter found during
investigation (--gtest_filter="Preprocessor.PassthroughPlainCode:
CompileEntry.CompileFileFdSuccess", deterministic 100% before the
fix) and the full suite (195 tests), both passing cleanly across
repeated runs on RelWithDebInfo and Debug+ASan/UBSan.

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

---------

Co-authored-by: Claude <noreply@anthropic.com>

src/base/internal/stralloc.cc

@@ -95,6 +95,21 @@ static block_t* alloc_new_shared_string(const char* /*string*/, int /*h*/, const
 void init_strings() {
   int x, y;
 
+  // Idempotent: a real driver process calls this exactly once (vm_init()),
+  // but a second call anywhere -- reachable in practice from a test binary
+  // that hosts both a lightweight, no-full-boot compiler harness and a
+  // real driver boot in the same process -- would otherwise silently
+  // reallocate base_table, orphaning every shared string interned so far
+  // (still live, valid memory, just unreachable from the fresh table) with
+  // no free/migration. The first free_string() of one of those orphaned
+  // strings then fails its "is this still in the table" sanity check and
+  // aborts the process. Skip rather than orphan: whichever call happens
+  // first wins, matching the invariant the driver already relies on.
+  if (base_table) {
+    debug_message("init_strings: called again, ignoring (already initialized).\n");
+    return;
+  }
+
   /* ensure that htable size is a power of 2 */
   y = CONFIG_INT(__SHARED_STRING_HASH_TABLE_SIZE__);
   /* Cap the round-up at 2^30: a larger configured value would shift past

src/tests/test_compiler.cc

@@ -268,6 +268,19 @@ static std::vector<Token> TokenizeSession(bool keep_macros, const std::string& s
   // segfaulted CompileEntry tests running after any tokenizer test.
   lpc_lex_scanner_destroyed(scanner);
   yylex_destroy(scanner);
+  // Symmetric with start_new_file() above: that call arms lexer_utils.cc's
+  // file-local main_filename exactly once per top-level compile (a
+  // make_shared_string() ref), and only end_new_file() ever disarms it.
+  // Skipping this call here (as this function did before) leaves
+  // main_filename permanently pinned to THIS session's filename for the
+  // rest of the process -- harmless-looking on its own, but a real bug
+  // when a later CompileEntry test's ensure_compile_env() then
+  // re-initializes the shared-string table (see the g_test_env_inited
+  // comment above): the pin still points at a live, valid block, but one
+  // that's no longer reachable from the fresh table, so the eventual
+  // free_string() of a REAL compile's own main_filename trips "free_string
+  // called on non-shared string" against this session's stale pin.
+  end_new_file();
   free_string(const_cast<char*>(current_file));
   current_file = nullptr;
   for (int i = 0; i < NUMAREAS; i++) {

src/tests/test_lpc.cc

@@ -5,6 +5,28 @@
 
 #include "compiler/internal/compiler.h"
 
+namespace {
+// Runs `fn` (arbitrary LPC-triggering driver code -- load_object_from_source,
+// destruct_object, free_object, ... can all run create()/__INIT/applies)
+// under a proper recovery point, matching the pattern DriverTest's other
+// tests use inline. Without this, an error() thrown with no established
+// error_context hits the driver's fatal() fallback and aborts the whole
+// test binary instead of failing just one check.
+template <typename F>
+void RunGuarded(F&& fn) {
+  error_context_t econ{};
+  save_context(&econ);
+  try {
+    fn();
+  } catch (...) {
+    restore_context(&econ);
+    ADD_FAILURE() << "unexpected error() during test";
+    return;
+  }
+  pop_context(&econ);
+}
+}  // namespace
+
 // Test fixture class
 class DriverTest : public ::testing::Test {
  public:
@@ -227,3 +249,73 @@ TEST_F(DriverTest, ExplodeReversibleAllDelimiters) {
   v = explode_string("a", 1, "a", 1, false);
   EXPECT_EQ(v->size, 0);
 }
+
+// Regression test for a heap-use-after-free in dealloc_object()
+// (src/vm/internal/base/object.cc): destruct_object() pushes the object
+// onto the global obj_list_destruct queue (simulate.cc); on the unfixed
+// binary, dealloc_object() never unlinked the object from that queue when
+// its ref count hit 0, so a same-call-sequence destruct+free of object A
+// left obj_list_destruct pointing at freed memory. The very next
+// destruct_object() call anywhere -- here, on an unrelated object B --
+// then wrote through that dangling head pointer. Fails under ASan on the
+// unfixed binary; on a plain build it would silently corrupt whatever
+// memory A's address gets reused for.
+TEST_F(DriverTest, DestructThenImmediateFreeDoesNotDangleObjListDestruct) {
+  object_t* a = nullptr;
+  object_t* b = nullptr;
+  RunGuarded([&] { a = load_object_from_source("void bump() {}\n", "lifecycle_head_a", 0); });
+  RunGuarded([&] { b = load_object_from_source("void bump() {}\n", "lifecycle_head_b", 0); });
+  ASSERT_NE(a, nullptr);
+  ASSERT_NE(b, nullptr);
+
+  RunGuarded([&] {
+    destruct_object(a);
+    free_object(&a, "DestructThenImmediateFreeDoesNotDangleObjListDestruct");
+  });
+  RunGuarded([&] {
+    destruct_object(b);
+    free_object(&b, "DestructThenImmediateFreeDoesNotDangleObjListDestruct");
+  });
+}
+
+// Regression test for the general (not just head) case of the same bug:
+// destructing A, B, C in order chains obj_list_destruct as C -> B -> A.
+// Freeing the MIDDLE object (B) directly -- exactly what reclaim_objects()
+// does when it finds a stray reference to an already-destructed object --
+// must correctly relink C's neighbor pointer to skip the freed B, or a
+// later destruct_object() call (which touches the current head's
+// neighbor pointers) dereferences freed memory.
+TEST_F(DriverTest, MidChainFreeKeepsObjListDestructWalkable) {
+  object_t* a = nullptr;
+  object_t* b = nullptr;
+  object_t* c = nullptr;
+  object_t* d = nullptr;
+  RunGuarded([&] { a = load_object_from_source("void bump() {}\n", "lifecycle_mid_a", 0); });
+  RunGuarded([&] { b = load_object_from_source("void bump() {}\n", "lifecycle_mid_b", 0); });
+  RunGuarded([&] { c = load_object_from_source("void bump() {}\n", "lifecycle_mid_c", 0); });
+  ASSERT_NE(a, nullptr);
+  ASSERT_NE(b, nullptr);
+  ASSERT_NE(c, nullptr);
+
+  RunGuarded([&] { destruct_object(a); });
+  RunGuarded([&] { destruct_object(b); });
+  RunGuarded([&] { destruct_object(c); });
+
+  // Free the middle object directly, simulating reclaim_objects() dropping
+  // a stray reference to a destructed object mid-queue.
+  RunGuarded([&] { free_object(&b, "MidChainFreeKeepsObjListDestructWalkable"); });
+
+  // Destructing (and freeing) a 4th object exercises the current
+  // obj_list_destruct head's neighbor pointers -- on the unfixed binary
+  // this is where the stale link left by the mid-chain free above would
+  // be dereferenced.
+  RunGuarded([&] { d = load_object_from_source("void bump() {}\n", "lifecycle_mid_d", 0); });
+  ASSERT_NE(d, nullptr);
+  RunGuarded([&] {
+    destruct_object(d);
+    free_object(&d, "MidChainFreeKeepsObjListDestructWalkable");
+  });
+
+  RunGuarded([&] { free_object(&c, "MidChainFreeKeepsObjListDestructWalkable"); });
+  RunGuarded([&] { free_object(&a, "MidChainFreeKeepsObjListDestructWalkable"); });
+}

src/vm/internal/base/object.cc

@@ -1966,6 +1966,19 @@ void dealloc_object(object_t* ob, const char* from) {
     SETOBNAME(ob, nullptr);
   }
 #ifdef DEBUG
+  // obj_list_destruct (destruct_object()'s "not yet swept by
+  // remove_destructed_objects()" queue, simulate.cc) is NOT gated by
+  // DEBUG, unlike obj_list_dangling below -- but in a DEBUG build the two
+  // lists happen to share this object's next_all/prev_all storage
+  // (destruct_object() pushes onto both, one right after the other, so
+  // the two chains are always structurally identical until something is
+  // unlinked). The neighbor fixup the obj_list_dangling unlink below
+  // performs therefore already keeps the underlying chain correct for
+  // obj_list_destruct's own forward walk too; only its separate head
+  // *variable* needs updating here.
+  if (obj_list_destruct == ob) {
+    obj_list_destruct = ob->next_all;
+  }
   prev_all = ob->prev_all;
   if (prev_all) {
     prev_all->next_all = ob->next_all;
@@ -1981,6 +1994,27 @@ void dealloc_object(object_t* ob, const char* from) {
   ob->next_all = 0;
   ob->prev_all = 0;
   tot_dangling_object--;
+#else
+  // No obj_list_dangling bookkeeping exists in this build to perform the
+  // equivalent neighbor fixup as a side effect (see the DEBUG branch
+  // above), so unlink from obj_list_destruct explicitly here. Otherwise a
+  // later destruct_object() call anywhere in the driver can dereference
+  // this object's now-freed address via a stale obj_list_destruct head or
+  // a neighbor's stale next_all/prev_all -- a real, ASan-confirmed
+  // heap-use-after-free (reachable via reclaim_objects() freeing a stray
+  // reference to a destructed object still queued mid-chain, or simply an
+  // object whose only reference drops immediately after destruct()).
+  if (obj_list_destruct == ob) {
+    obj_list_destruct = ob->next_all;
+    if (obj_list_destruct) {
+      obj_list_destruct->prev_all = nullptr;
+    }
+  } else if (ob->prev_all) {
+    ob->prev_all->next_all = ob->next_all;
+    if (ob->next_all) {
+      ob->next_all->prev_all = ob->prev_all;
+    }
+  }
 #endif
   tot_alloc_object--;
   FREE((char*)ob);