From f7c1cea9b70c7b48c2b1fa4fb03f547879dff5f8 Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Fri, 16 Dec 2022 15:27:30 +0000 Subject: [PATCH] bad(Swingset): Releasing a constrained Zalgo on syscall replay --- .../src/kernel/vat-loader/async-helper.js | 46 +++++++++++++++++++ .../src/kernel/vat-loader/manager-helper.js | 32 ++++++++++--- .../kernel/vat-loader/manager-nodeworker.js | 2 +- .../vat-loader/manager-subprocess-node.js | 2 +- .../vat-loader/manager-subprocess-xsnap.js | 6 +-- .../src/kernel/vat-loader/transcript.js | 28 ++++++----- 6 files changed, 94 insertions(+), 22 deletions(-) create mode 100644 packages/SwingSet/src/kernel/vat-loader/async-helper.js diff --git a/packages/SwingSet/src/kernel/vat-loader/async-helper.js b/packages/SwingSet/src/kernel/vat-loader/async-helper.js new file mode 100644 index 00000000000..709899c4019 --- /dev/null +++ b/packages/SwingSet/src/kernel/vat-loader/async-helper.js @@ -0,0 +1,46 @@ +/** + * @template T + * @typedef {T extends PromiseLike ? never : T} SyncOnly + */ + +/** + * @template {boolean} AllowAsync + * @template T + * @typedef {true extends AllowAsync ? (Promise | T) : T} MaybePromiseResult + */ + +/** + * WARNING: Do not use, this is releasing Zalgo + * + * @template T + * @template R + * @param {T} v + * @param {(v: Awaited) => R} handle + * @returns {T extends PromiseLike ? Promise> : R} + */ +export function maybeAsync(v, handle) { + if (typeof v === 'object' && v !== null && 'then' in v) { + return Promise.resolve(v).then(handle); + } else { + return handle(v); + } +} + +/** + * Somewhat tames Zalgo + * + * @template {(...args: any[]) => any} T + * @param {T} fn + * @returns {(...args: Parameters) => SyncOnly>} + */ +export function ensureSync(fn) { + // eslint-disable-next-line func-names + return function (...args) { + const result = Reflect.apply(fn, this, args); + if (typeof result === 'object' && result !== null && 'then' in result) { + throw new Error('Unexpected async result'); + } else { + return result; + } + }; +} diff --git a/packages/SwingSet/src/kernel/vat-loader/manager-helper.js b/packages/SwingSet/src/kernel/vat-loader/manager-helper.js index 94d8255f52b..cf7a7c6dd9d 100644 --- a/packages/SwingSet/src/kernel/vat-loader/manager-helper.js +++ b/packages/SwingSet/src/kernel/vat-loader/manager-helper.js @@ -3,6 +3,7 @@ import { assert } from '@agoric/assert'; import '../../types-ambient.js'; import { insistVatDeliveryResult } from '../../lib/message.js'; import { makeTranscriptManager } from './transcript.js'; +import { ensureSync, maybeAsync } from './async-helper.js'; // We use vat-centric terminology here, so "inbound" means "into a vat", // always from the kernel. Conversely "outbound" means "out of a vat", into @@ -47,9 +48,10 @@ import { makeTranscriptManager } from './transcript.js'; /** * + * @template {boolean} AsyncSyscall * @typedef { { getManager: (shutdown: () => Promise, * makeSnapshot?: (ss: SnapStore) => Promise) => VatManager, - * syscallFromWorker: (vso: VatSyscallObject) => VatSyscallResult, + * syscallFromWorker: (vso: VatSyscallObject) => import('./async-helper.js').MaybePromiseResult, * setDeliverToWorker: (dtw: unknown) => void, * } } ManagerKit * @@ -91,7 +93,7 @@ import { makeTranscriptManager } from './transcript.js'; * * The returned syscallFromWorker function should be called when the worker * wants to make a syscall. It accepts a VatSyscallObject and will return a - * (synchronous) VatSyscallResult, never throwing. For remote workers, this + * (synchronous if possible) VatSyscallResult. For remote workers, this * should be called from a handler that receives a syscall message from the * child process. * @@ -106,7 +108,7 @@ import { makeTranscriptManager } from './transcript.js'; * @param {WorkerAsyncKind} workerAsyncKind * @param {import('./transcript.js').CompareSyscalls} [compareSyscalls] * @param {boolean} [useTranscript] - * @returns {ManagerKit} + * @returns {ManagerKit<'async-blocking' extends WorkerAsyncKind ? true : false>} */ function makeManagerKit( @@ -120,13 +122,21 @@ function makeManagerKit( ) { assert(kernelSlog); const vatKeeper = kernelKeeper.provideVatKeeper(vatID); - /** @type {ReturnType | undefined} */ + /** @typedef {'async-blocking' extends WorkerAsyncKind ? true : false} AsyncMode */ + + /** @type {ReturnType> | undefined} */ let transcriptManager; if (useTranscript) { + const asyncCompliantCompareSyscall = + workerAsyncKind === 'async-blocking' || !compareSyscalls + ? /** @type {import('./transcript.js').CompareSyscalls | undefined} */ ( + compareSyscalls + ) + : ensureSync(compareSyscalls); transcriptManager = makeTranscriptManager( vatKeeper, vatID, - compareSyscalls, + asyncCompliantCompareSyscall, ); } @@ -261,7 +271,17 @@ function makeManagerKit( // but if the puppy deviates one inch from previous twitches, explode kernelSlog.syscall(vatID, undefined, vso); - return transcriptManager.simulateSyscall(vso) || doSyscallFromWorker(vso); + // We trust simulateSyscall to only return a promise if compareSyscalls + // itself returns a promise, and we propagate that mode here. + // Since compareSyscalls can be user provided, we ensure it doesn't + // return a promise if the worker doesn't support it (workerAsyncKind + // is not 'async-blocking') + return /** @type {import('./async-helper.js').MaybePromiseResult} */ ( + maybeAsync( + transcriptManager.simulateSyscall(vso), + vres => vres || doSyscallFromWorker(vso), + ) + ); } else { const vres = doSyscallFromWorker(vso); if (transcriptManager) { diff --git a/packages/SwingSet/src/kernel/vat-loader/manager-nodeworker.js b/packages/SwingSet/src/kernel/vat-loader/manager-nodeworker.js index d8963c90030..e07212ad978 100644 --- a/packages/SwingSet/src/kernel/vat-loader/manager-nodeworker.js +++ b/packages/SwingSet/src/kernel/vat-loader/manager-nodeworker.js @@ -85,7 +85,7 @@ export function makeNodeWorkerVatManagerFactory(tools) { } else if (type === 'syscall') { parentLog(`syscall`, args); const [vatSyscallObject] = args; - mk.syscallFromWorker(vatSyscallObject); + void mk.syscallFromWorker(vatSyscallObject); } else if (type === 'testLog') { testLog(...args); } else if (type === 'deliverDone') { diff --git a/packages/SwingSet/src/kernel/vat-loader/manager-subprocess-node.js b/packages/SwingSet/src/kernel/vat-loader/manager-subprocess-node.js index bf7e1c52029..e6d22803c11 100644 --- a/packages/SwingSet/src/kernel/vat-loader/manager-subprocess-node.js +++ b/packages/SwingSet/src/kernel/vat-loader/manager-subprocess-node.js @@ -82,7 +82,7 @@ export function makeNodeSubprocessFactory(tools) { } else if (type === 'syscall') { parentLog(`syscall`, args); const [vatSyscallObject] = args; - mk.syscallFromWorker(vatSyscallObject); + void mk.syscallFromWorker(vatSyscallObject); } else if (type === 'testLog') { testLog(...args); } else if (type === 'deliverDone') { diff --git a/packages/SwingSet/src/kernel/vat-loader/manager-subprocess-xsnap.js b/packages/SwingSet/src/kernel/vat-loader/manager-subprocess-xsnap.js index 3c956abc9f3..c78e43d275a 100644 --- a/packages/SwingSet/src/kernel/vat-loader/manager-subprocess-xsnap.js +++ b/packages/SwingSet/src/kernel/vat-loader/manager-subprocess-xsnap.js @@ -76,8 +76,8 @@ export function makeXsSubprocessFactory({ useTranscript, ); - /** @type { (item: Tagged) => unknown } */ - function handleUpstream([type, ...args]) { + /** @type { (item: Tagged) => Promise } */ + async function handleUpstream([type, ...args]) { parentLog(vatID, `handleUpstream`, type, args.length); switch (type) { case 'syscall': { @@ -107,7 +107,7 @@ export function makeXsSubprocessFactory({ /** @type { (msg: Uint8Array) => Promise } */ async function handleCommand(msg) { // parentLog('handleCommand', { length: msg.byteLength }); - const tagged = handleUpstream(JSON.parse(decoder.decode(msg))); + const tagged = await handleUpstream(JSON.parse(decoder.decode(msg))); return encoder.encode(JSON.stringify(tagged)); } diff --git a/packages/SwingSet/src/kernel/vat-loader/transcript.js b/packages/SwingSet/src/kernel/vat-loader/transcript.js index 54a1f848f38..1920c611fd2 100644 --- a/packages/SwingSet/src/kernel/vat-loader/transcript.js +++ b/packages/SwingSet/src/kernel/vat-loader/transcript.js @@ -1,6 +1,7 @@ // @ts-check import djson from '../../lib/djson.js'; +import { maybeAsync } from './async-helper.js'; // Indicate that a syscall is missing from the transcript but is safe to // perform during replay @@ -12,12 +13,13 @@ export const extraSyscall = Symbol('extra transcript syscall'); /** @typedef {typeof missingSyscall | typeof extraSyscall | Error | undefined} CompareSyscallsResult */ /** + * @template {boolean} [MaybeAsync=boolean] * @typedef {( * vatId: any, * originalSyscall: VatSyscallObject, * newSyscall: VatSyscallObject, * originalResponse?: VatSyscallResult, - * ) => CompareSyscallsResult + * ) => import('./async-helper.js').MaybePromiseResult * } CompareSyscalls */ @@ -98,9 +100,10 @@ export function requireIdenticalExceptStableVCSyscalls( } /** + * @template {boolean} AsyncCompare * @param {*} vatKeeper * @param {*} vatID - * @param {CompareSyscalls} compareSyscalls + * @param {CompareSyscalls} compareSyscalls */ export function makeTranscriptManager( vatKeeper, @@ -179,19 +182,22 @@ export function makeTranscriptManager( return failReplay(); } - // eslint-disable-next-line no-use-before-define - return handleCompareResult( - compareSyscalls( - vatID, - playbackSyscalls[0].d, - newSyscall, - playbackSyscalls[0].response, - ), + return /** @type {import('./async-helper.js').MaybePromiseResult} */ ( + maybeAsync( + compareSyscalls( + vatID, + playbackSyscalls[0].d, + newSyscall, + playbackSyscalls[0].response, + ), + // eslint-disable-next-line no-use-before-define + handleCompareResult, + ) ); } /** * @param {CompareSyscallsResult} compareError - * @returns {VatSyscallResult | undefined} + * @returns {import('./async-helper.js').MaybePromiseResult} */ function handleCompareResult(compareError) { if (compareError === missingSyscall) {