From 8b58027282e4ba01edd2cfd4838f7f3eb738b661 Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Thu, 6 Apr 2023 18:03:56 -0700 Subject: [PATCH] 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 | 31 ++++++++--- .../src/virtualReferences.js | 52 +++++++++++++------ .../test/storeGC/test-weak-key.js | 11 ++-- .../test-virtualObjectManager.js | 37 ++++++++----- 4 files changed, 90 insertions(+), 41 deletions(-) diff --git a/packages/swingset-liveslots/src/liveslots.js b/packages/swingset-liveslots/src/liveslots.js index 4f44595e99d0..a4140d4fb4ce 100644 --- a/packages/swingset-liveslots/src/liveslots.js +++ b/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)); } diff --git a/packages/swingset-liveslots/src/virtualReferences.js b/packages/swingset-liveslots/src/virtualReferences.js index 1474397c0e0b..141f77b1b19f 100644 --- a/packages/swingset-liveslots/src/virtualReferences.js +++ b/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,9 +47,28 @@ 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 @@ -58,23 +76,22 @@ export function makeVirtualReferenceManager( * 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) { @@ -673,7 +690,8 @@ export function makeVirtualReferenceManager( isPresenceReachable, isVrefRecognizable, setExportStatus, - possibleVirtualObjectDeath, + isVirtualObjectReachable, + deleteVirtualObject, ceaseRecognition, setDeleteCollectionEntry, testHooks, diff --git a/packages/swingset-liveslots/test/storeGC/test-weak-key.js b/packages/swingset-liveslots/test/storeGC/test-weak-key.js index c53dcd22785e..e8f6ddd320cc 100644 --- a/packages/swingset-liveslots/test/storeGC/test-weak-key.js +++ b/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 diff --git a/packages/swingset-liveslots/test/virtual-objects/test-virtualObjectManager.js b/packages/swingset-liveslots/test/virtual-objects/test-virtualObjectManager.js index 74cd48b8cdb6..00e8eab7aaaf 100644 --- a/packages/swingset-liveslots/test/virtual-objects/test-virtualObjectManager.js +++ b/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, []);