fix liveslots double-free of virtual object/collection

When A and B are any combination of virtual objects or collections,
and A holds a reference to B, and no other vdata/export references
exist to either, and both A and B's Representatives are dropped, and
both drops are noticed in the same BOYD, and A's vref is
lexicographically earlier than B's, then liveslots would attempt to
free B twice.

The cause was using `vrm.possibleVirtualObjectDeath()`, which combines
a remaining-pillar check with the deletion code. Deleting a virtual
object or collection necessarily examines the contents (to decrement
their refcounts), and will add vrefs to possiblyDeadSet in the
process. For our example, deleting A caused B to get added back to
possiblyDeadSet, which caused a second deletion attempt as
scanForDeadObjects looped back around to pick up second-level dead
objects, even though B was deleted during the first pass, a moment
after A. If B's vref sorted earlier than A, possibleVirtualObjectDeath
would have not deleted B on the first pass, leaving it for the second,
and only trying to delete it once.

In earlier versions of the code, this double free was silently
ignored. But since the introduction of dataCache and schemaCache, this
causes an assertion to fail, causing a crash.

The fix is to filter possiblyDeadSet through a new
`vrm.isVirtualObjectReachable()` predicate to generate deadSet,
and *then* delete the objects with a new `vrm.deleteVirtualObject`.

closes #7353

packages/swingset-liveslots/src/liveslots.js

@@ -257,18 +257,37 @@ function build(
       // eslint-disable-next-line no-await-in-loop, @jessie.js/no-nested-await
       await gcTools.gcAndFinalize();
 
-      // `deadSet` is the subset of those vrefs which lack an in-memory
-      // manifestation *right now* (i.e. the non-resurrected ones), for which
-      // we must check the remaining pillars.
+      // possiblyDeadSet contains a baseref for everything (Presences,
+      // Remotables, Representatives) that might have lost a
+      // pillar. The object might still be supported by other pillars,
+      // and the lost pillar might have been reinstantiated by the
+      // time we get here. The first step is to filter this down to a
+      // list of definitely dead baserefs.
+
       const deadSet = new Set();
+
       for (const baseRef of possiblyDeadSet) {
         // eslint-disable-next-line no-use-before-define
-        if (!slotToVal.has(baseRef)) {
-          deadSet.add(baseRef);
+        if (slotToVal.has(baseRef)) {
+          continue; // RAM pillar remains
+        }
+        const { virtual, durable, type } = parseVatSlot(baseRef);
+        assert(type === 'object', `unprepared to track ${type}`);
+        if (virtual || durable) {
+          // eslint-disable-next-line no-use-before-define
+          if (vrm.isVirtualObjectReachable(baseRef)) {
+            continue; // vdata or export pillar remains
+          }
         }
+        deadSet.add(baseRef);
       }
       possiblyDeadSet.clear();
 
+      // deadSet now contains objects which are certainly dead
+
+      // possiblyRetiredSet holds (a subset of??) baserefs which have
+      // lost a recognizer recently. TODO recheck this
+
       for (const vref of possiblyRetiredSet) {
         // eslint-disable-next-line no-use-before-define
         if (!getValForSlot(vref) && !deadSet.has(vref)) {
@@ -291,7 +310,7 @@ function build(
         if (virtual || durable) {
           // Representative: send nothing, but perform refcount checking
           // eslint-disable-next-line no-use-before-define
-          const [gcAgain, retirees] = vrm.possibleVirtualObjectDeath(baseRef);
+          const [gcAgain, retirees] = vrm.deleteVirtualObject(baseRef);
           if (retirees) {
             retirees.map(retiree => exportsToRetire.add(retiree));
           }

packages/swingset-liveslots/src/virtualReferences.js

@@ -38,8 +38,7 @@ export function makeVirtualReferenceManager(
   );
 
   /**
-   * Check if a virtual object is truly dead - i.e., unreachable - and truly
-   * delete it if so.
+   * Check if a virtual object is unreachable via virtual-data references.
    *
    * A virtual object is kept alive if it or any of its facets are reachable by
    * any of three legs:
@@ -48,33 +47,51 @@ export function makeVirtualReferenceManager(
    *  - virtual references (if so, it will have a refcount > 0)
    *  - being exported (if so, its export flag will be set)
    *
-   * This function is called after a leg has been reported missing, and only
-   * if the memory (Representative) leg is currently missing, to see if the
-   * other two legs are now gone also.
+   * This function is called after a leg has been reported missing,
+   * and reports back on whether the virtual-reference and
+   * export-status legs remain. The caller (liveslots
+   * scanForDeadObjects) will combine this with information about the
+   * RAM leg to decide whether the object is completely unreachable,
+   * and thus should be deleted.
+   *
+   * @param {string} baseRef  The virtual object cohort might be dead
+   *
+   * @returns {boolean} True if the object remains referenced, false if unreachable
+   */
+  function isVirtualObjectReachable(baseRef) {
+    const refCount = getRefCount(baseRef);
+    const [reachable, _retirees] = getExportStatus(baseRef);
+    return !!(reachable || refCount);
+  }
+
+  /**
+   * Delete a virtual object
+   *
+   * Once the caller determines that all three legs are gone, they
+   * call us to delete the object.
    *
    * Deletion consists of removing the vatstore entries that describe its state
    * and track its refcount status.  In addition, when a virtual object is
    * deleted, we delete any weak collection entries for which it was a key. If
    * it had been exported, we also inform the kernel that the vref has been
    * retired, so other vats can delete their weak collection entries too.
    *
-   * @param {string} baseRef  The virtual object cohort that's plausibly dead
+   * @param {string} baseRef  The virtual object cohort that's certainly dead
    *
    * @returns {[boolean, string[]]} A pair of a flag that's true if this
    *    possibly created a new GC opportunity and an array of vrefs that should
    *    now be regarded as unrecognizable
    */
-  function possibleVirtualObjectDeath(baseRef) {
+  function deleteVirtualObject(baseRef) {
     const refCount = getRefCount(baseRef);
     const [reachable, retirees] = getExportStatus(baseRef);
-    if (!reachable && refCount === 0) {
-      let doMoreGC = deleteStoredRepresentation(baseRef);
-      syscall.vatstoreDelete(`vom.rc.${baseRef}`);
-      syscall.vatstoreDelete(`vom.es.${baseRef}`);
-      doMoreGC = ceaseRecognition(baseRef) || doMoreGC;
-      return [doMoreGC, retirees];
-    }
-    return [false, []];
+    assert(!reachable);
+    assert(!refCount);
+    let doMoreGC = deleteStoredRepresentation(baseRef);
+    syscall.vatstoreDelete(`vom.rc.${baseRef}`);
+    syscall.vatstoreDelete(`vom.es.${baseRef}`);
+    doMoreGC = ceaseRecognition(baseRef) || doMoreGC;
+    return [doMoreGC, retirees];
   }
 
   /**
@@ -574,7 +591,7 @@ export function makeVirtualReferenceManager(
       // themselves.
       const kindInfo = kindInfoTable.get(`${p.id}`);
       // This function can be called either from `dispatch.retireImports` or
-      // from `possibleVirtualObjectDeath`.  In the latter case the vref is
+      // from `deleteVirtualObject`.  In the latter case the vref is
       // actually a baseRef and so needs to be expanded to cease recognition of
       // all the facets.
       if (kindInfo) {
@@ -685,7 +702,8 @@ export function makeVirtualReferenceManager(
     isPresenceReachable,
     isVrefRecognizable,
     setExportStatus,
-    possibleVirtualObjectDeath,
+    isVirtualObjectReachable,
+    deleteVirtualObject,
     ceaseRecognition,
     setDeleteCollectionEntry,
     getRetentionStats,

packages/swingset-liveslots/test/storeGC/test-weak-key.js

@@ -69,13 +69,14 @@ test.serial('verify store weak key GC', async t => {
 
   // the full sequence is:
   // * finalizer(held) pushes vref onto possiblyDeadSet
-  // * BOYD calls vrm.possibleVirtualObjectDeath
+  // * BOYD calls vrm.isVirtualObjectReachable
   // * that checks vdata refcount and export status (vstore reads)
   // * concludes no pillars are remaining, initiates deletion
-  // * pVOD uses deleteStoredRepresentation() to delete vobj data
-  //   * 'held' is empty, so has no vobj data to delete
-  // * vom.(rc.es).${baseRef} keys deleted (refcount/export-status trackers)
-  // * ceaseRecognition() is called, then pVOD returns
+  // * BOYD calls vrm.deleteVirtualObject
+  //   * dVO uses deleteStoredRepresentation() to delete vobj data
+  //     * 'held' is empty, so has no vobj data to delete
+  //   * vom.(rc.es).${baseRef} keys deleted (refcount/export-status trackers)
+  //   * ceaseRecognition() is called, then dVO returns
   // * cR removes from all voAwareWeakMap/Sets (none)
   // * cR walks vom.ir.${vref}|XX to find weak-store recognizers
   //   * this finds both our WeakSet and our WeakMap

packages/swingset-liveslots/test/virtual-objects/test-virtualObjectManager.js

@@ -562,7 +562,7 @@ test('durable kind IDs can be reanimated', t => {
     log,
   });
   const { makeKindHandle, defineDurableKind, flushStateCache } = vom;
-  const { possibleVirtualObjectDeath } = vrm;
+  const { isVirtualObjectReachable } = vrm;
   const { makeScalarBigMapStore } = cm;
   const { deleteEntry } = fakeStuff;
 
@@ -591,10 +591,11 @@ test('durable kind IDs can be reanimated', t => {
   t.is(log.shift(), 'set vc.1.|entryCount 1');
   t.deepEqual(log, []);
 
-  // Forget its existence
+  // Forget its Representative
   kindHandle = null;
   deleteEntry(khid);
-  possibleVirtualObjectDeath(khid);
+  t.deepEqual(log, []);
+  t.truthy(isVirtualObjectReachable(khid));
   t.is(log.shift(), `get vom.rc.${khid} => 1`);
   t.is(log.shift(), `get vom.es.${khid} => undefined`);
   t.deepEqual(log, []);
@@ -631,7 +632,8 @@ test('virtual object gc', t => {
   const log = [];
   const { vom, vrm, fakeStuff } = makeFakeVirtualStuff({ log });
   const { defineKind, flushStateCache } = vom;
-  const { setExportStatus, possibleVirtualObjectDeath } = vrm;
+  const { setExportStatus, deleteVirtualObject, isVirtualObjectReachable } =
+    vrm;
   const { deleteEntry, dumpStore } = fakeStuff;
 
   const makeThing = defineKind('thing', initThing, thingBehavior);
@@ -689,9 +691,12 @@ test('virtual object gc', t => {
   ]);
 
   // This is what the finalizer would do if the local reference was dropped and GC'd
-  function pretendGC(vref) {
+  function pretendGC(vref, shouldDelete) {
     deleteEntry(vref);
-    possibleVirtualObjectDeath(vref);
+    t.is(!!isVirtualObjectReachable(vref), !shouldDelete);
+    if (shouldDelete) {
+      deleteVirtualObject(vref);
+    }
   }
 
   // case 1: export, drop local ref, drop export
@@ -701,7 +706,7 @@ test('virtual object gc', t => {
   t.is(log.shift(), `set vom.es.${tbase}/1 r`);
   t.deepEqual(log, []);
   // drop local ref -- should not delete because exported
-  pretendGC(`${tbase}/1`);
+  pretendGC(`${tbase}/1`, false);
   t.is(log.shift(), `get vom.rc.${tbase}/1 => undefined`);
   t.is(log.shift(), `get vom.es.${tbase}/1 => r`);
   t.deepEqual(log, []);
@@ -728,7 +733,9 @@ test('virtual object gc', t => {
   t.is(log.shift(), `set vom.es.${tbase}/1 s`);
   t.is(log.shift(), `get vom.rc.${tbase}/1 => undefined`);
   t.deepEqual(log, []);
-  pretendGC(`${tbase}/1`);
+  pretendGC(`${tbase}/1`, true);
+  t.is(log.shift(), `get vom.rc.${tbase}/1 => undefined`);
+  t.is(log.shift(), `get vom.es.${tbase}/1 => s`);
   t.is(log.shift(), `get vom.rc.${tbase}/1 => undefined`);
   t.is(log.shift(), `get vom.es.${tbase}/1 => s`);
   t.is(log.shift(), `get vom.${tbase}/1 => ${thingVal(0, 'thing #1', 0)}`);
@@ -788,7 +795,9 @@ test('virtual object gc', t => {
   ]);
 
   // drop local ref -- should delete
-  pretendGC(`${tbase}/2`);
+  pretendGC(`${tbase}/2`, true);
+  t.is(log.shift(), `get vom.rc.${tbase}/2 => undefined`);
+  t.is(log.shift(), `get vom.es.${tbase}/2 => s`);
   t.is(log.shift(), `get vom.rc.${tbase}/2 => undefined`);
   t.is(log.shift(), `get vom.es.${tbase}/2 => s`);
   t.is(log.shift(), `get vom.${tbase}/2 => ${thingVal(0, 'thing #2', 0)}`);
@@ -816,7 +825,9 @@ test('virtual object gc', t => {
 
   // case 3: drop local ref with no prior export
   // drop local ref -- should delete
-  pretendGC(`${tbase}/3`);
+  pretendGC(`${tbase}/3`, true);
+  t.is(log.shift(), `get vom.rc.${tbase}/3 => undefined`);
+  t.is(log.shift(), `get vom.es.${tbase}/3 => undefined`);
   t.is(log.shift(), `get vom.rc.${tbase}/3 => undefined`);
   t.is(log.shift(), `get vom.es.${tbase}/3 => undefined`);
   t.is(log.shift(), `get vom.${tbase}/3 => ${thingVal(0, 'thing #3', 0)}`);
@@ -867,7 +878,7 @@ test('virtual object gc', t => {
   t.is(log.shift(), `set vom.es.${tbase}/4 r`);
   t.deepEqual(log, []);
   // drop local ref -- should not delete because ref'd virtually AND exported
-  pretendGC(`${tbase}/4`);
+  pretendGC(`${tbase}/4`, false);
   t.is(log.shift(), `get vom.rc.${tbase}/4 => 1`);
   t.is(log.shift(), `get vom.es.${tbase}/4 => r`);
   t.deepEqual(log, []);
@@ -907,7 +918,7 @@ test('virtual object gc', t => {
     ['vom.vkind.11', '{"kindID":"11","tag":"ref"}'],
   ]);
   // drop local ref -- should not delete because ref'd virtually AND exported
-  pretendGC(`${tbase}/5`);
+  pretendGC(`${tbase}/5`, false);
   t.is(log.shift(), `get vom.rc.${tbase}/5 => 1`);
   t.is(log.shift(), `get vom.es.${tbase}/5 => r`);
   t.deepEqual(log, []);
@@ -943,7 +954,7 @@ test('virtual object gc', t => {
     ['vom.vkind.11', '{"kindID":"11","tag":"ref"}'],
   ]);
   // drop local ref -- should not delete because ref'd virtually
-  pretendGC(`${tbase}/6`);
+  pretendGC(`${tbase}/6`, false);
   t.is(log.shift(), `get vom.rc.${tbase}/6 => 1`);
   t.is(log.shift(), `get vom.es.${tbase}/6 => undefined`);
   t.deepEqual(log, []);