Skip to content

Commit

Permalink
fix(swingset): liveslots: disable metering of GC-sensitive calls
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@warner
warner committed Aug 12, 2021
1 parent 6929460 commit 6d60f0a
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 25 deletions.
56 changes: 42 additions & 14 deletions packages/SwingSet/src/kernel/liveSlots.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
*
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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`);
Expand Down Expand Up @@ -659,12 +672,15 @@ function build(
return undefined;
}
return (...args) => {
// the m.serialize should be metered, so getters don't provide an
// escape from metering
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;
};
},
Expand Down Expand Up @@ -708,6 +724,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
Expand Down Expand Up @@ -747,6 +764,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) {
Expand All @@ -757,6 +775,7 @@ function build(
}

function thenHandler(p, promiseID, rejected) {
// this runs metered
insistVatType('promise', promiseID);
return value => {
knownResolutions.set(p, harden([rejected, value]));
Expand All @@ -778,7 +797,7 @@ function build(
pRec.resolve(value);
}
}
retirePromiseID(promiseID);
meterControl.runWithoutMetering(() => retirePromiseID(promiseID));
};
}

Expand All @@ -802,6 +821,7 @@ function build(
X`unknown promiseID '${promiseID}'`,
);
const pRec = importedPromisesByPromiseID.get(promiseID);
meterControl.assertNotMetered();
const val = m.unserialize(data);
if (rejected) {
pRec.reject(val);
Expand All @@ -824,6 +844,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);
}
}
Expand Down Expand Up @@ -860,6 +881,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();
Expand Down Expand Up @@ -903,6 +925,7 @@ function build(
// TODO: when we add notifyForward, guard against cycles

function exitVat(completion) {
// this is allowed to be metered: only unserialization must be unmetered
const args = m.serialize(harden(completion));
args.slots.map(retainExportedVref);
syscall.exit(false, args);
Expand Down Expand Up @@ -1030,8 +1053,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
Expand All @@ -1048,7 +1084,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}`),
);
Expand All @@ -1060,15 +1096,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);

Expand Down
32 changes: 21 additions & 11 deletions packages/SwingSet/test/test-marshal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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'],
});
Expand All @@ -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'],
});
Expand All @@ -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'],
});
Expand All @@ -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}',
Expand All @@ -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,
);

Expand All @@ -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'],
});
Expand Down

0 comments on commit 6d60f0a

Please sign in to comment.