Skip to content

Commit

Permalink
fix(xsnap): do not leak through vat termination race
Browse files Browse the repository at this point in the history
  • Loading branch information
mhofman committed Jun 22, 2022
1 parent 547d722 commit 61bccdf
Show file tree
Hide file tree
Showing 4 changed files with 318 additions and 18 deletions.
1 change: 1 addition & 0 deletions packages/xsnap/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"@endo/eventual-send": "^0.15.3",
"@endo/init": "^0.5.41",
"@endo/netstring": "^0.3.11",
"@endo/promise-kit": "^0.2.41",
"@endo/stream": "^0.3.10",
"@endo/stream-node": "^0.2.11",
"glob": "^7.1.6",
Expand Down
36 changes: 18 additions & 18 deletions packages/xsnap/src/xsnap.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

import { makeNetstringReader, makeNetstringWriter } from '@endo/netstring';
import { makeNodeReader, makeNodeWriter } from '@endo/stream-node';
import { racePromises } from '@endo/promise-kit';
import { ErrorCode, ErrorSignal, ErrorMessage, METER_TYPE } from '../api.js';
import { defer } from './defer.js';

Expand All @@ -30,6 +31,8 @@ const decoder = new TextDecoder();

const { freeze } = Object;

const noop = freeze(() => {});

/**
* @param {Uint8Array} arg
* @returns {Uint8Array}
Expand Down Expand Up @@ -211,8 +214,8 @@ export function xsnap(options) {
await messagesToXsnap.next(encoder.encode(`e${code}`));
return runToIdle();
});
baton = result.then(() => {}).catch(() => {});
return Promise.race([vatCancelled, result]);
baton = result.then(noop, noop);
return racePromises([vatCancelled, result]);
}

/**
Expand All @@ -224,8 +227,8 @@ export function xsnap(options) {
await messagesToXsnap.next(encoder.encode(`s${fileName}`));
await runToIdle();
});
baton = result.catch(() => {});
return Promise.race([vatCancelled, result]);
baton = result.then(noop, noop);
return racePromises([vatCancelled, result]);
}

/**
Expand All @@ -237,8 +240,8 @@ export function xsnap(options) {
await messagesToXsnap.next(encoder.encode(`m${fileName}`));
await runToIdle();
});
baton = result.catch(() => {});
return Promise.race([vatCancelled, result]);
baton = result.then(noop, noop);
return racePromises([vatCancelled, result]);
}

/**
Expand All @@ -249,8 +252,8 @@ export function xsnap(options) {
await messagesToXsnap.next(encoder.encode(`R`));
await runToIdle();
});
baton = result.catch(() => {});
return Promise.race([vatCancelled, result]);
baton = result.then(noop, noop);
return racePromises([vatCancelled, result]);
}

/**
Expand All @@ -265,11 +268,8 @@ export function xsnap(options) {
await messagesToXsnap.next(request);
return runToIdle();
});
baton = result.then(
() => {},
() => {},
);
return Promise.race([vatCancelled, result]);
baton = result.then(noop, noop);
return racePromises([vatCancelled, result]);
}

/**
Expand All @@ -290,8 +290,8 @@ export function xsnap(options) {
await messagesToXsnap.next(encoder.encode(`w${file}`));
await runToIdle();
});
baton = result.catch(() => {});
return Promise.race([vatExit.promise, baton]);
baton = result.then(noop, noop);
return racePromises([vatExit.promise, baton]);
}

/**
Expand All @@ -302,7 +302,7 @@ export function xsnap(options) {
await messagesToXsnap.return(undefined);
throw new Error(`${name} closed`);
});
baton.catch(() => {}); // Suppress Node.js unhandled exception warning.
baton.catch(noop); // Suppress Node.js unhandled exception warning.
return vatExit.promise;
}

Expand All @@ -312,9 +312,9 @@ export function xsnap(options) {
async function terminate() {
xsnapProcess.kill();
baton = Promise.reject(new Error(`${name} terminated`));
baton.catch(() => {}); // Suppress Node.js unhandled exception warning.
baton.catch(noop); // Suppress Node.js unhandled exception warning.
// Mute the vatExit exception: it is expected.
return vatExit.promise.catch(() => {});
return vatExit.promise.catch(noop);
}

return freeze({
Expand Down
277 changes: 277 additions & 0 deletions patches/@endo+promise-kit+0.2.41.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,277 @@
diff --git a/node_modules/@endo/promise-kit/index.js b/node_modules/@endo/promise-kit/index.js
index 1c37090..77dbc83 100644
--- a/node_modules/@endo/promise-kit/index.js
+++ b/node_modules/@endo/promise-kit/index.js
@@ -3,17 +3,26 @@

/// <reference types="ses"/>

+import { memoRace } from './memo-race.js';
+
/** @type {PromiseConstructor} */
const BestPipelinablePromise = globalThis.HandledPromise || Promise;

/**
* @template T
- * @typedef {Object} PromiseKit A reified Promise
+ * @typedef {object} PromiseKit A reified Promise
* @property {(value: ERef<T>) => void} resolve
* @property {(reason: any) => void} reject
* @property {Promise<T>} promise
*/

+/**
+ * @template T
+ * @callback PromiseExecutor The promise executor
+ * @param {(value: ERef<T>) => void} resolve
+ * @param {(reason: any) => void} reject
+ */
+
/**
* PromiseRecord is deprecated in favor of PromiseKit.
*
@@ -31,9 +40,49 @@ const BestPipelinablePromise = globalThis.HandledPromise || Promise;
*/

/**
- * Needed to prevent type errors where functions are detected to be undefined.
+ * releasingExecutorKit() builds resolve/reject functions which drop references
+ * to the resolve/reject functions gathered from an executor to be used with a
+ * promise constructor.
+ *
+ * @template T
+ * @returns {Pick<PromiseKit<T>, 'resolve' | 'reject'> & { executor: PromiseExecutor<T>}}
*/
-const NOOP_INITIALIZER = harden(() => {});
+const releasingExecutorKit = () => {
+ /** @type {null | undefined | ((value: ERef<T>) => void)} */
+ let internalResolve;
+ /** @type {null | undefined | ((reason: unknown) => void)} */
+ let internalReject;
+
+ /** @param {ERef<T>} value */
+ const resolve = value => {
+ if (internalResolve) {
+ internalResolve(value);
+ internalResolve = null;
+ internalReject = null;
+ } else {
+ assert(internalResolve === null);
+ }
+ };
+
+ /** @param {unknown} reason */
+ const reject = reason => {
+ if (internalReject) {
+ internalReject(reason);
+ internalResolve = null;
+ internalReject = null;
+ } else {
+ assert(internalReject === null);
+ }
+ };
+
+ const executor = (res, rej) => {
+ assert(internalResolve === undefined && internalReject === undefined);
+ internalResolve = res;
+ internalReject = rej;
+ };
+
+ return { resolve, reject, executor };
+};

/**
* makePromiseKit() builds a Promise object, and returns a record
@@ -44,16 +93,9 @@ const NOOP_INITIALIZER = harden(() => {});
* @returns {PromiseKit<T>}
*/
export function makePromiseKit() {
- /** @type {(value: ERef<T>) => void} */
- let resolve = NOOP_INITIALIZER;
- /** @type {(reason: unknown) => void} */
- let reject = NOOP_INITIALIZER;
+ const { resolve, reject, executor } = releasingExecutorKit();

- /** @type {Promise<T>} */
- const promise = new BestPipelinablePromise((res, rej) => {
- resolve = res;
- reject = rej;
- });
+ const promise = new BestPipelinablePromise(executor);

return harden({ promise, resolve, reject });
}
@@ -69,3 +111,23 @@ export function isPromise(maybePromise) {
return Promise.resolve(maybePromise) === maybePromise;
}
harden(isPromise);
+
+// NB: Another implementation for Promise.race would be to use the releasing executor,
+// However while it would no longer leak the raced promise objects themselves, it would
+// still leak reactions on the non-resolved promises contending for the race.
+
+/**
+ * Creates a Promise that is resolved or rejected when any of the provided Promises are resolved
+ * or rejected.
+ *
+ * Unlike `Promise.race` it cleans up after itself so a non-resolved value doesn't hold onto
+ * the result promise.
+ *
+ * @template {readonly unknown[] | []} T
+ * @param {T} values An array of Promises.
+ * @returns {Promise<Awaited<T[number]>>} A new Promise.
+ */
+export function racePromises(values) {
+ return harden(memoRace(values));
+}
+harden(racePromises);
diff --git a/node_modules/@endo/promise-kit/memo-race.js b/node_modules/@endo/promise-kit/memo-race.js
new file mode 100644
index 0000000..c03e9e0
--- /dev/null
+++ b/node_modules/@endo/promise-kit/memo-race.js
@@ -0,0 +1,144 @@
+// @ts-check
+/*
+Initial version authored by Brian Kim:
+https://github.com/nodejs/node/issues/17469#issuecomment-685216777
+
+This is free and unencumbered software released into the public domain.
+
+Anyone is free to copy, modify, publish, use, compile, sell, or
+distribute this software, either in source code form or as a compiled
+binary, for any purpose, commercial or non-commercial, and by any
+means.
+
+In jurisdictions that recognize copyright laws, the author or authors
+of this software dedicate any and all copyright interest in the
+software to the public domain. We make this dedication for the benefit
+of the public at large and to the detriment of our heirs and
+successors. We intend this dedication to be an overt act of
+relinquishment in perpetuity of all present and future rights to this
+software under copyright law.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
+IN NO EVENT SHALL THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+OTHER DEALINGS IN THE SOFTWARE.
+
+For more information, please refer to <http://unlicense.org/>
+*/
+
+const isObject = value => Object(value) === value;
+
+/**
+ * @template [T=any]
+ * @typedef {object} Deferred
+ * @property {(value?: T | PromiseLike<T> ) => void} resolve
+ * @property {(err?: any ) => void} reject
+ */
+
+/**
+ * @typedef { never
+ * | {settled: false, deferreds: Set<Deferred>}
+ * | {settled: true, deferreds?: undefined}
+ * } PromiseMemoRecord
+ */
+
+// Keys are the values passed to race, values are a record of data containing a
+// set of deferreds and whether the value has settled.
+/** @type {WeakMap<object, PromiseMemoRecord>} */
+const knownPromises = new WeakMap();
+
+/**
+ * @param {PromiseMemoRecord | undefined} record
+ * @returns {Set<Deferred>}
+ */
+const markSettled = record => {
+ if (!record || record.settled) {
+ return new Set();
+ }
+
+ const { deferreds } = record;
+ Object.assign(record, {
+ deferreds: undefined,
+ settled: true,
+ });
+ Object.freeze(record);
+ return deferreds;
+};
+
+/**
+ *
+ * @param {any} value
+ * @returns {PromiseMemoRecord}
+ */
+const getMemoRecord = value => {
+ if (!isObject(value)) {
+ // If the contender is a primitive, attempting to use it as a key in the
+ // weakmap would throw an error. Luckily, it is safe to call
+ // `Promise.resolve(contender).then` on a primitive value multiple times
+ // because the promise fulfills immediately. So we fake a settled record.
+ return { settled: true };
+ }
+
+ let record = knownPromises.get(value);
+
+ if (!record) {
+ record = { deferreds: new Set(), settled: false };
+ knownPromises.set(value, record);
+ // This call to `then` happens once for the lifetime of the value.
+ Promise.resolve(value).then(
+ val => {
+ for (const { resolve } of markSettled(record)) {
+ resolve(val);
+ }
+ },
+ err => {
+ for (const { reject } of markSettled(record)) {
+ reject(err);
+ }
+ },
+ );
+ }
+ return record;
+};
+
+/**
+ * Creates a Promise that is resolved or rejected when any of the provided Promises are resolved
+ * or rejected.
+ *
+ * Unlike `Promise.race` it cleans up after itself so a non-resolved value doesn't hold onto
+ * the result promise.
+ *
+ * @template {readonly unknown[] | []} T
+ * @param {T} values An array of Promises.
+ * @returns {Promise<Awaited<T[number]>>} A new Promise.
+ */
+export const memoRace = values => {
+ let deferred;
+ const result = new Promise((resolve, reject) => {
+ deferred = { resolve, reject };
+ for (const value of values) {
+ const { settled, deferreds } = getMemoRecord(value);
+ if (settled) {
+ // If the contender is settled (including primitives), it is safe
+ // to call `Promise.resolve(value).then` on it.
+ Promise.resolve(value).then(resolve, reject);
+ } else {
+ deferreds.add(deferred);
+ }
+ }
+ });
+
+ // The finally callback executes when any value settles, preventing any of
+ // the unresolved values from retaining a reference to the resolved value.
+ return result.finally(() => {
+ for (const value of values) {
+ const { deferreds } = getMemoRecord(value);
+ if (deferreds) {
+ deferreds.delete(deferred);
+ }
+ }
+ });
+};
Loading

0 comments on commit 61bccdf

Please sign in to comment.