Do not hold a reference into the type memo across constructing a type (Covered by dco/Brian_Egge.md)

The previous commit rewrote unique_refc_map::get to return by value, and in
doing so held a reference into the map across mk(), the call that constructs
the type being interned. Constructing a type can intern the types it is made
of -- FixedArray and Array normalise their element type on the way in, for
one -- which re-enters the same map (the mutex is recursive for exactly this)
and can insert, and an insertion can rehash, and a rehash moves the entry the
reference pointed at. The write through it afterwards is a write to freed
memory. CI caught it: hobbes-test died with "double free or corruption" in
Compiler/ccInManyThreads on the clang-16 build, and only there, as
use-after-free does.

The original code looked the key up again after mk() for this reason, and
that is restored in a form that also keeps the first object if mk() interned
this very key on the way: look up, construct, look up again, keep whichever
entry is there.

While here, two of Copilot's review comments on morganstanley#552: the compaction in
net.C and prepl.C ran after the work and so was skipped when the work threw
-- and the failing input is the one that interned nothing but garbage. Both
sites now use a scope guard (CompactMTypeMemoryAtExit, in lang/type.H)
declared at the top of the request, so it runs on every way out: success,
the decode throwing, or the error write itself throwing on a dead peer. And
the reuse test in test/Matching.C now proves reuse rather than just its
correctness, by counting the ".regex." functions defined in a fresh compiler
as the same and a look-alike regex are compiled.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016rGT4C394qeh2DBQhqy3Tb

Author: brianegge

include/hobbes/lang/type.H

@@ -1260,6 +1260,20 @@ inline MonoTypePtr fnresult(const MonoTypePtr& fty) {
 
 void compactMTypeMemory();
 
+// compact the type memo when the enclosing scope ends, however it ends: for
+// code that decodes types it may not keep -- a request handler, say -- so that
+// the types a rejected input interned are released on the failure path as
+// well as on success
+class CompactMTypeMemoryAtExit {
+public:
+  explicit CompactMTypeMemoryAtExit(bool armed = true) : armed(armed) { }
+  ~CompactMTypeMemoryAtExit() { if (this->armed) { compactMTypeMemory(); } }
+  CompactMTypeMemoryAtExit(const CompactMTypeMemoryAtExit&) = delete;
+  CompactMTypeMemoryAtExit& operator=(const CompactMTypeMemoryAtExit&) = delete;
+private:
+  bool armed;
+};
+
 // shorthand for making file ref types
 MonoTypePtr fileRefTy(const MonoTypePtr&, const MonoTypePtr&);
 MonoTypePtr fileRefTy(const MonoTypePtr&);

include/hobbes/util/ptr.H

@@ -26,11 +26,23 @@ template <typename T, typename ... Args>
     std::shared_ptr<T> get(const std::function<T*(Args...)>& mk, const Args&... args) {
       std::lock_guard<std::recursive_mutex> lock(mutex);
       auto k = std::tuple<Args...>(args...);
-      auto& r = this->values[k];
-      if (!r) {
-        r = std::shared_ptr<T>(mk(args...));
+
+      auto found = this->values.find(k);
+      if (found != this->values.end() && found->second) {
+        return found->second;
+      }
+
+      // no reference into the map is held across mk(): constructing a type can
+      // intern the types it is made of, re-entering this map (the mutex is
+      // recursive for that reason), and an insertion there can rehash it. So
+      // construct first, then look the key up again, and keep whichever entry
+      // is there -- mk() may have interned this very key on the way.
+      std::shared_ptr<T> made(mk(args...));
+      std::shared_ptr<T>& slot = this->values[k];
+      if (!slot) {
+        slot = made;
       }
-      return r;
+      return slot;
     }
 
     size_t compact() {

lib/hobbes/ipc/net.C

@@ -234,6 +234,11 @@ void evaluateNetREPLRequest(int c, void *d) {
     case 0:
       // prepare a lexical expression with input and output types given
       try {
+        // the type descriptions read below are interned in the process-wide
+        // type memo whether or not they are accepted; release what this
+        // request does not end up keeping, on every way out of here
+        CompactMTypeMemoryAtExit compactAfter;
+
         exprid eid = 0;
         fdread(c, &eid);
 
@@ -259,11 +264,12 @@ void evaluateNetREPLRequest(int c, void *d) {
         fdwrite(c, uint8_t(0));
         fdwrite(c, std::string(ex.what()));
       }
-      compactMTypeMemory();
       break;
     case 1:
       // prepare a serialized expression, also return its type
       try {
+        CompactMTypeMemoryAtExit compactAfter;
+
         exprid eid = 0;
         fdread(c, &eid);
         RawData exprd;
@@ -287,7 +293,6 @@ void evaluateNetREPLRequest(int c, void *d) {
         fdwrite(c, uint8_t(0));
         fdwrite(c, std::string(ex.what()));
       }
-      compactMTypeMemory();
       break;
     case 2:
       // invoke a prepared expression

lib/hobbes/ipc/prepl.C

@@ -315,6 +315,16 @@ void runMachineREPLStep(cc* c) {
     int cmd = 0;
     fdread(STDIN_FILENO, &cmd);
 
+    // the meta commands below decode types and expressions sent by the peer,
+    // and every type that builds is interned in the process-wide type memo,
+    // which holds on to it until asked not to. Ask once this step is over,
+    // however it ends: what the command needed is held by the compiler and
+    // stays, what it only passed through -- including everything a rejected
+    // input interned before it was rejected -- goes. Invoking a compiled
+    // function (the default case) decodes nothing and is the hot path, so it
+    // is left alone.
+    CompactMTypeMemoryAtExit compactAfter(cmd < CMD_COUNT);
+
     switch (cmd) {
     case CMD_REFINE_VNAME: {
       // legacy method to refine the type of a variable given an initial type "guess"
@@ -508,16 +518,6 @@ void runMachineREPLStep(cc* c) {
       resetMemoryPool();
       break;
     }
-
-    // the meta commands above decode types and expressions sent by the peer,
-    // and every type that builds is interned in the process-wide type memo,
-    // which holds on to it until asked not to. Ask after each one: what the
-    // command needed is held by the compiler and stays, what it only passed
-    // through goes. Invoking a compiled function (the default case) decodes
-    // nothing and is the hot path, so it is left alone.
-    if (cmd < CMD_COUNT) {
-      compactMTypeMemory();
-    }
   } catch (std::exception& ex) {
     std::string exn = ex.what();
     dbglog("*** " + exn);

test/Matching.C

@@ -522,13 +522,37 @@ TEST(Matching, hugeRegexDFACompilesWithoutQuadraticBlowup) {
 // first time rather than defining another. Reuse has to be by the regex
 // itself, not by how it prints: 'a|b' (either) and 'a\|b' (three literals)
 // print alike but match differently, and must not share a matcher.
+// a compiled matcher is a function defined in the compiler under a ".regex."
+// name, so counting those in its type environment counts the matchers it holds
+// (dumpTypeEnv hides dot-prefixed names, so go to the environment directly)
+static size_t regexMatchersDefinedIn(cc &x) {
+  size_t n = 0;
+  for (const auto &s : x.typeEnv()->boundVariables()) {
+    if (s.rfind(".regex.", 0) == 0) {
+      ++n;
+    }
+  }
+  return n;
+}
+
 TEST(Matching, regexMatchersAreReusedButOnlyForTheSameRegex) {
-  auto either1 = c().compileFn<int(const std::string &)>(
+  // a fresh compiler, so that the count of matchers is this test's to reason about
+  cc lc;
+  const size_t before = regexMatchersDefinedIn(lc);
+
+  auto either1 = lc.compileFn<int(const std::string &)>(
       "s", "match s with | 'a|b' -> 1 | _ -> 0");
-  auto either2 = c().compileFn<int(const std::string &)>(
+  EXPECT_EQ(regexMatchersDefinedIn(lc), before + 1);
+
+  // the same regex again: no new matcher
+  auto either2 = lc.compileFn<int(const std::string &)>(
       "s", "match s with | 'a|b' -> 2 | _ -> 0");
-  auto literal = c().compileFn<int(const std::string &)>(
+  EXPECT_EQ(regexMatchersDefinedIn(lc), before + 1);
+
+  // a regex that merely prints the same: a new one
+  auto literal = lc.compileFn<int(const std::string &)>(
       "s", "match s with | 'a\\|b' -> 3 | _ -> 0");
+  EXPECT_EQ(regexMatchersDefinedIn(lc), before + 2);
 
   EXPECT_EQ(either1("a"), 1);
   EXPECT_EQ(either1("b"), 1);