fix(swingset): liveslots: disable metering of GC-sensitive calls

We're trying to hedge against XS not necessarily performing
"organic" (non-forced) GC at exactly the same across all members (validators)
of a consensus machine, especially when some members have reloaded a vat from
snapshot (i.e. restarted) recently and others have not. We rely upon
finalizer callbacks only running at explicitly-deterministic times, but we
must still guard against objects becoming collected spontaneously, which will
cause a WeakRef to become "dead" (`wr.deref() === undefined`) at a random
point in the middle of a turn. Any code which calls `wr.deref`, or is
conditionally executed/skipped according to the results, is "GC-sensitive".
This includes `convertSlotToVal`, and therefore `m.unserialize`.

We cannot allow metering results to diverge between validators, because:

* 1: it might make the difference between the crank completing successfully,
and the vat being terminated for a per-crank metering fault
* 2: it will change the large-scale Meter value, which is reported to
userspace
* 3: it might cause the runPolicy to finish the block earlier on one
validator than on others

all of which would cause a consensus failure.

To prevent this, we run most of the "inbound" side of liveslots without
metering. This includes the first turn of all `dispatch.*` methods, which
runs entirely within liveslots:

* `dispatch.deliver` performs argument deserialization in the first turn,
  then executes user code in the second and subsequent turns
* `dispatch.notify` does the same
* the GC deliveries (`dispatch.dropExport`, etc) only use one turn

We also disable metering when deserializing the return value from
a (synchronous) device call, and when retiring a promise ID (which touches
`slotToVal`).

Finally, we disable metering for all turns of the post-crank GC `finish()`
call. This excludes all invocations of the finalizer callbacks, as well as
all the `processDeadSet` code which is highly sensitive to the results.

closes #3458

packages/SwingSet/src/kernel/liveSlots.js

@@ -31,7 +31,8 @@ const DEFAULT_VIRTUAL_OBJECT_CACHE_SIZE = 3; // XXX ridiculously small value to
  * @param {boolean} enableVatstore
  * @param {*} vatPowers
  * @param {*} vatParameters
- * @param {*} gcTools { WeakRef, FinalizationRegistry, waitUntilQuiescent, gcAndFinalize }
+ * @param {*} gcTools { WeakRef, FinalizationRegistry, waitUntilQuiescent, gcAndFinalize,
+ *                      meterControl }
  * @param {Console} console
  * @returns {*} { vatGlobals, inescapableGlobalProperties, dispatch, setBuildRootObject }
  *
@@ -52,7 +53,7 @@ function build(
   gcTools,
   console,
 ) {
-  const { WeakRef, FinalizationRegistry } = gcTools;
+  const { WeakRef, FinalizationRegistry, meterControl } = gcTools;
   const enableLSDebug = false;
   function lsdebug(...args) {
     if (enableLSDebug) {
@@ -408,6 +409,7 @@ function build(
       // controlled by the `console` option given to makeLiveSlots.
       console.info('Logging sent error stack', err),
   });
+  const unmeteredUnserialize = meterControl.unmetered(m.unserialize);
 
   function getSlotForVal(val) {
     return valToSlot.get(val);
@@ -461,6 +463,9 @@ function build(
       valToSlot.set(val, slot);
       slotToVal.set(slot, new WeakRef(val));
       if (type === 'object') {
+        // Set.delete() metering seems unaffected by presence/absence, but it
+        // doesn't matter anyway because deadSet.add only happens when
+        // finializers run, which happens deterministically
         deadSet.delete(slot);
         droppedRegistry.register(val, slot, val);
       }
@@ -489,7 +494,14 @@ function build(
     }
   }
 
+  // The meter usage of convertSlotToVal is strongly affected by GC, because
+  // it only creates a new Presence if one does not already exist. Userspace
+  // moves from REACHABLE to UNREACHABLE, but the JS engine then moves to
+  // COLLECTED (and maybe FINALIZED) on its own, and we must not allow the
+  // latter changes to affect metering. So every call to convertSlotToVal (or
+  // m.unserialize) must be wrapped by unmetered().
   function convertSlotToVal(slot, iface = undefined) {
+    meterControl.assertNotMetered();
     const { type, allocatedByVat, virtual } = parseVatSlot(slot);
     const wr = slotToVal.get(slot);
     let val = wr && wr.deref();
@@ -552,6 +564,7 @@ function build(
       for (const slot of slots) {
         const { type } = parseVatSlot(slot);
         if (type === 'promise') {
+          // this can run metered because it's supposed to always be present
           const wr = slotToVal.get(slot);
           const p = wr && wr.deref();
           assert(p, X`should have a value for ${slot} but didn't`);
@@ -567,6 +580,7 @@ function build(
 
     function collect(promiseID, rejected, value) {
       doneResolutions.add(promiseID);
+      meterControl.assertIsMetered(); // else userspace getters could escape
       const valueSer = m.serialize(value);
       valueSer.slots.map(retainExportedVref);
       resolutions.push([promiseID, rejected, valueSer]);
@@ -599,6 +613,7 @@ function build(
       }
     }
 
+    meterControl.assertIsMetered(); // else userspace getters could escape
     const serArgs = m.serialize(harden(args));
     serArgs.slots.map(retainExportedVref);
     const resultVPID = allocatePromiseID();
@@ -659,12 +674,14 @@ function build(
           return undefined;
         }
         return (...args) => {
+          meterControl.assertIsMetered(); // userspace getters shouldn't escape
           const serArgs = m.serialize(harden(args));
           serArgs.slots.map(retainExportedVref);
           forbidPromises(serArgs);
           const ret = syscall.callNow(slot, prop, serArgs);
           insistCapData(ret);
-          const retval = m.unserialize(ret);
+          // but the unserialize must be unmetered, to prevent divergence
+          const retval = unmeteredUnserialize(ret);
           return retval;
         };
       },
@@ -708,6 +725,7 @@ function build(
       method = Symbol.asyncIterator;
     }
 
+    meterControl.assertNotMetered();
     const args = m.unserialize(argsdata);
 
     // If the method is missing, or is not a Function, or the method throws a
@@ -747,6 +765,7 @@ function build(
   function retirePromiseID(promiseID) {
     lsdebug(`Retiring ${forVatID}:${promiseID}`);
     importedPromisesByPromiseID.delete(promiseID);
+    meterControl.assertNotMetered();
     const wr = slotToVal.get(promiseID);
     const p = wr && wr.deref();
     if (p) {
@@ -757,6 +776,7 @@ function build(
   }
 
   function thenHandler(p, promiseID, rejected) {
+    // this runs metered
     insistVatType('promise', promiseID);
     return value => {
       knownResolutions.set(p, harden([rejected, value]));
@@ -778,7 +798,7 @@ function build(
           pRec.resolve(value);
         }
       }
-      retirePromiseID(promiseID);
+      meterControl.runWithoutMetering(() => retirePromiseID(promiseID));
     };
   }
 
@@ -802,6 +822,7 @@ function build(
       X`unknown promiseID '${promiseID}'`,
     );
     const pRec = importedPromisesByPromiseID.get(promiseID);
+    meterControl.assertNotMetered();
     const val = m.unserialize(data);
     if (rejected) {
       pRec.reject(val);
@@ -824,6 +845,7 @@ function build(
     const imports = finishCollectingPromiseImports();
     for (const slot of imports) {
       if (slotToVal.get(slot)) {
+        // we'll only subscribe to new promises, which is within consensus
         syscall.subscribe(slot);
       }
     }
@@ -860,6 +882,7 @@ function build(
     } else {
       // Remotable
       // console.log(`-- liveslots acting on retireExports ${vref}`);
+      meterControl.assertNotMetered();
       const wr = slotToVal.get(vref);
       if (wr) {
         const val = wr.deref();
@@ -903,12 +926,14 @@ function build(
   // TODO: when we add notifyForward, guard against cycles
 
   function exitVat(completion) {
+    meterControl.assertIsMetered(); // else userspace getters could escape
     const args = m.serialize(harden(completion));
     args.slots.map(retainExportedVref);
     syscall.exit(false, args);
   }
 
   function exitVatWithFailure(reason) {
+    meterControl.assertIsMetered(); // else userspace getters could escape
     const args = m.serialize(harden(reason));
     args.slots.map(retainExportedVref);
     syscall.exit(true, args);
@@ -1030,8 +1055,21 @@ function build(
     }
   }
 
+  // the first turn of each dispatch is spent in liveslots, and is not
+  // metered
+  const unmeteredDispatch = meterControl.unmetered(dispatchToUserspace);
+
   const { waitUntilQuiescent, gcAndFinalize } = gcTools;
 
+  async function finish() {
+    await gcAndFinalize();
+    const doMore = processDeadSet();
+    if (doMore) {
+      return finish();
+    }
+    return undefined;
+  }
+
   /**
    * This low-level liveslots code is responsible for deciding when userspace
    * is done with a crank. Userspace code can use Promises, so it can add as
@@ -1048,7 +1086,7 @@ function build(
     // *not* directly wait for the userspace function to complete, nor for
     // any promise it returns to fire.
     Promise.resolve(delivery)
-      .then(dispatchToUserspace)
+      .then(unmeteredDispatch)
       .catch(err =>
         console.log(`liveslots error ${err} during delivery ${delivery}`),
       );
@@ -1060,15 +1098,7 @@ function build(
 
     // Now that userspace is idle, we can drive GC until we think we've
     // stopped.
-    async function finish() {
-      await gcAndFinalize();
-      const doMore = processDeadSet();
-      if (doMore) {
-        return finish();
-      }
-      return undefined;
-    }
-    return finish();
+    return meterControl.runWithoutMeteringAsync(finish);
   }
   harden(dispatch);
 

packages/SwingSet/test/test-marshal.js

@@ -17,6 +17,12 @@ const gcTools = harden({
   meterControl: makeDummyMeterControl(),
 });
 
+function makeUnmeteredMarshaller(syscall) {
+  const { m } = makeMarshaller(syscall, gcTools);
+  const unmeteredUnserialize = gcTools.meterControl.unmetered(m.unserialize);
+  return { m, unmeteredUnserialize };
+}
+
 async function prep() {
   const config = {};
   const controller = await buildVatController(config);
@@ -51,8 +57,8 @@ test('serialize exports', t => {
 
 test('deserialize imports', async t => {
   await prep();
-  const { m } = makeMarshaller(undefined, gcTools);
-  const a = m.unserialize({
+  const { unmeteredUnserialize } = makeUnmeteredMarshaller(undefined);
+  const a = unmeteredUnserialize({
     body: '{"@qclass":"slot","index":0}',
     slots: ['o-1'],
   });
@@ -61,25 +67,25 @@ test('deserialize imports', async t => {
   t.truthy(Object.isFrozen(a));
 
   // m now remembers the proxy
-  const b = m.unserialize({
+  const b = unmeteredUnserialize({
     body: '{"@qclass":"slot","index":0}',
     slots: ['o-1'],
   });
   t.is(a, b);
 
   // the slotid is what matters, not the index
-  const c = m.unserialize({
+  const c = unmeteredUnserialize({
     body: '{"@qclass":"slot","index":2}',
     slots: ['x', 'x', 'o-1'],
   });
   t.is(a, c);
 });
 
 test('deserialize exports', t => {
-  const { m } = makeMarshaller(undefined, gcTools);
+  const { m, unmeteredUnserialize } = makeUnmeteredMarshaller(undefined);
   const o1 = Far('o1', {});
   m.serialize(o1); // allocates slot=1
-  const a = m.unserialize({
+  const a = unmeteredUnserialize({
     body: '{"@qclass":"slot","index":0}',
     slots: ['o+1'],
   });
@@ -88,8 +94,8 @@ test('deserialize exports', t => {
 
 test('serialize imports', async t => {
   await prep();
-  const { m } = makeMarshaller(undefined, gcTools);
-  const a = m.unserialize({
+  const { m, unmeteredUnserialize } = makeUnmeteredMarshaller(undefined);
+  const a = unmeteredUnserialize({
     body: '{"@qclass":"slot","index":0}',
     slots: ['o-1'],
   });
@@ -107,7 +113,7 @@ test('serialize promise', async t => {
     },
   };
 
-  const { m } = makeMarshaller(syscall, gcTools);
+  const { m, unmeteredUnserialize } = makeUnmeteredMarshaller(syscall);
   const { promise, resolve } = makePromiseKit();
   t.deepEqual(m.serialize(promise), {
     body: '{"@qclass":"slot","index":0}',
@@ -121,7 +127,10 @@ test('serialize promise', async t => {
 
   // inbound should recognize it and return the promise
   t.deepEqual(
-    m.unserialize({ body: '{"@qclass":"slot","index":0}', slots: ['p+5'] }),
+    unmeteredUnserialize({
+      body: '{"@qclass":"slot","index":0}',
+      slots: ['p+5'],
+    }),
     promise,
   );
 
@@ -144,7 +153,8 @@ test('unserialize promise', async t => {
   };
 
   const { m } = makeMarshaller(syscall, gcTools);
-  const p = m.unserialize({
+  const unserialize = gcTools.meterControl.unmetered(m.unserialize);
+  const p = unserialize({
     body: '{"@qclass":"slot","index":0}',
     slots: ['p-1'],
   });