From 63fc3f75d6fed345e03898436e71933362641871 Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Tue, 15 Mar 2022 18:00:52 +0000 Subject: [PATCH 01/11] feat: handle Promise symbols added by node async_hooks --- .github/workflows/ci.yml | 55 +++++ packages/init/node-async_hooks-patch.js | 3 + packages/init/node-async_hooks-symbols.d.ts | 11 + packages/init/node-async_hooks.js | 226 ++++++++++++++++++++ packages/init/pre.js | 2 + packages/init/test/test-async_hooks.js | 79 +++++++ packages/ses/src/whitelist.js | 7 + 7 files changed, 383 insertions(+) create mode 100644 packages/init/node-async_hooks-patch.js create mode 100644 packages/init/node-async_hooks-symbols.d.ts create mode 100644 packages/init/node-async_hooks.js create mode 100644 packages/init/test/test-async_hooks.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index afdd321d79..20d4fb7910 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -108,6 +108,61 @@ jobs: - name: Run yarn test run: yarn test + test-async-hooks: + name: test-async-hooks + + runs-on: ${{ matrix.platform }} + strategy: + fail-fast: false + matrix: + node-version: + - '14.2' # last version before promise fast-path without destroy + - '14.17' # last version before unconditional promise fast-path + - '14.18' # first version after unconditional promise fast-path + - '16.1' # last version before some significant promise hooks changes + - '16.5' # last version before unconditional promise fast-path + - '16.6' # first version after unconditional promise fast-path + platform: + - ubuntu-latest + +# begin macro + + steps: + + - name: Checkout + uses: actions/checkout@v2 + + - name: Use Node.js ${{ matrix.node-version }} + uses: actions/setup-node@v1 + with: + node-version: ${{ matrix.node-version }} + + - name: Echo node version + run: node --version + + - name: Get yarn cache directory path + id: yarn-cache-dir-path + run: echo "::set-output name=dir::$(yarn cache dir)" + + - name: Cache npm modules + uses: actions/cache@v1 + id: yarn-cache # use this to check for `cache-hit` (`steps.yarn-cache.outputs.cache-hit != 'true'`) + with: + path: ${{ steps.yarn-cache-dir-path.outputs.dir }} + key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }} + + - name: Install dependencies + run: yarn --frozen-lockfile + +# end macro + + - name: Run yarn build + run: yarn build + + - name: Run yarn test (@endo/init) + working-directory: packages/init + run: yarn test + cover: name: cover diff --git a/packages/init/node-async_hooks-patch.js b/packages/init/node-async_hooks-patch.js new file mode 100644 index 0000000000..6fe0ce4414 --- /dev/null +++ b/packages/init/node-async_hooks-patch.js @@ -0,0 +1,3 @@ +import { setup } from './node-async_hooks.js'; + +setup(true); diff --git a/packages/init/node-async_hooks-symbols.d.ts b/packages/init/node-async_hooks-symbols.d.ts new file mode 100644 index 0000000000..09fd7027de --- /dev/null +++ b/packages/init/node-async_hooks-symbols.d.ts @@ -0,0 +1,11 @@ +// @ts-check + +export {}; + +declare global { + interface SymbolConstructor { + readonly nodeAsyncHooksAsyncId: unique symbol; + readonly nodeAsyncHooksTriggerAsyncId: unique symbol; + readonly nodeAsyncHooksDestroyed: unique symbol; + } +} diff --git a/packages/init/node-async_hooks.js b/packages/init/node-async_hooks.js new file mode 100644 index 0000000000..b102c19000 --- /dev/null +++ b/packages/init/node-async_hooks.js @@ -0,0 +1,226 @@ +// @ts-check + +import { createHook, AsyncResource } from 'async_hooks'; + +/// + +const asyncHooksWellKnownNameFromDescription = { + async_id_symbol: 'nodeAsyncHooksAsyncId', + trigger_async_id_symbol: 'nodeAsyncHooksTriggerAsyncId', + destroyed: 'nodeAsyncHooksDestroyed', +}; + +const promiseAsyncHookFallbackStates = new WeakMap(); + +const setAsyncSymbol = (description, symbol) => { + const wellKnownName = asyncHooksWellKnownNameFromDescription[description]; + if (!wellKnownName) { + throw new Error('Unknown symbol'); + } else if (!Symbol[wellKnownName]) { + Symbol[wellKnownName] = symbol; + return true; + } else if (Symbol[wellKnownName] !== symbol) { + // console.warn( + // `Found duplicate ${description}:`, + // symbol, + // Symbol[wellKnownName], + // ); + return false; + } else { + return true; + } +}; + +// We can get the `async_id_symbol` and `trigger_async_id_symbol` through a +// simple instantiation of async_hook.AsyncResource, which causes little side +// effects. These are the 2 symbols that may be late bound, aka after the promise +// is returned to the program and would normally be frozen. +const findAsyncSymbolsFromAsyncResource = () => { + let found = 0; + Object.getOwnPropertySymbols(new AsyncResource('Bootstrap')).forEach(sym => { + const { description } = sym; + if (description in asyncHooksWellKnownNameFromDescription) { + if (setAsyncSymbol(description, sym)) { + found += 1; + } + } + }); + return found; +}; + +// To get the `destroyed` symbol installed on promises by async_hooks, +// the only option is to create and enable an AsyncHook. +// Different versions of Node handle this in various ways. +const findAsyncSymbolsFromPromiseCreateHook = () => { + const bootstrapData = []; + + { + const bootstrapHook = createHook({ + init(asyncId, type, triggerAsyncId, resource) { + if (type !== 'PROMISE') return; + // console.log('Bootstrap', asyncId, triggerAsyncId, resource); + bootstrapData.push({ asyncId, triggerAsyncId, resource }); + }, + destroy(_asyncId) { + // Needs to be present to trigger the addition of the destroyed symbol + }, + }); + + bootstrapHook.enable(); + // Use a never resolving promise to avoid triggering settlement hooks + const trigger = new Promise(() => {}); + bootstrapHook.disable(); + + // In some versions of Node, async_hooks don't give access to the resource + // itself, but to a "wrapper" which is basically hooks metadata for the promise + const promisesData = bootstrapData.filter( + ({ resource }) => Promise.resolve(resource) === resource, + ); + bootstrapData.length = 0; + const { length } = promisesData; + if (length > 1) { + // console.warn('Found multiple potential candidates'); + } + + const promiseData = promisesData.find( + ({ resource }) => resource === trigger, + ); + if (promiseData) { + bootstrapData.push(promiseData); + } else if (length) { + // console.warn('No candidates matched'); + } + } + + if (bootstrapData.length) { + // Normally all promise hooks are disabled in a subsequent microtask + // That means Node versions that modify promises at init will still + // trigger our proto hooks for promises created in this turn + // The following trick will disable the internal promise init hook + // However, only do this for destroy modifying versions, since some versions + // only modify promises if no destroy hook is requested, and do not correctly + // reset the internal init promise hook in those case. (e.g. v14.16.2) + const resetHook = createHook({}); + resetHook.enable(); + resetHook.disable(); + + const { asyncId, triggerAsyncId, resource } = bootstrapData.pop(); + const symbols = Object.getOwnPropertySymbols(resource); + // const { length } = symbols; + let found = 0; + // if (length !== 3) { + // console.error(`Found ${length} symbols on promise:`, ...symbols); + // } + symbols.forEach(symbol => { + const value = resource[symbol]; + let type; + if (value === asyncId) { + type = 'async_id_symbol'; + } else if (value === triggerAsyncId) { + type = 'trigger_async_id_symbol'; + } else if (typeof value === 'object' && 'destroyed' in value) { + type = 'destroyed'; + } else { + // console.error(`Unexpected symbol`, symbol); + return; + } + + if (setAsyncSymbol(type, symbol)) { + found += 1; + } + }); + return found; + } else { + // This node version is not mutating promises + return -2; + } +}; + +const getAsyncHookFallbackState = (promise, create) => { + let state = promiseAsyncHookFallbackStates.get(promise); + if (!state && create) { + state = { + [Symbol.nodeAsyncHooksAsyncId]: undefined, + [Symbol.nodeAsyncHooksTriggerAsyncId]: undefined, + }; + if (Symbol.nodeAsyncHooksDestroyed) { + state[Symbol.nodeAsyncHooksDestroyed] = undefined; + } + promiseAsyncHookFallbackStates.set(promise, state); + } + return state; +}; + +const setAsyncIdFallback = (promise, symbol, value) => { + const state = getAsyncHookFallbackState(promise, true); + + if (state[symbol]) { + if (state[symbol] !== value) { + // This can happen if a frozen promise created before hooks were enabled + // is used multiple times as a parent promise + // It's safe to ignore subsequent values + } + } else { + state[symbol] = value; + } +}; + +const getAsyncHookSymbolPromiseProtoDesc = (symbol, disallowGet) => ({ + set(value) { + if (Object.isExtensible(this)) { + Object.defineProperty(this, symbol, { + value, + writable: false, + configurable: false, + enumerable: false, + }); + } else { + // console.log('fallback set of async id', symbol, value, new Error().stack); + setAsyncIdFallback(this, symbol, value); + } + }, + get() { + if (disallowGet) { + return undefined; + } + const state = getAsyncHookFallbackState(this, false); + return state && state[symbol]; + }, + enumerable: false, + configurable: true, +}); + +export const setup = (withDestroy = true) => { + if (withDestroy) { + findAsyncSymbolsFromPromiseCreateHook(); + } else { + findAsyncSymbolsFromAsyncResource(); + } + + if (!Symbol.nodeAsyncHooksAsyncId || !Symbol.nodeAsyncHooksTriggerAsyncId) { + // console.log(`Async symbols not found, moving on`); + return; + } + + const PromiseProto = Promise.prototype; + Object.defineProperty( + PromiseProto, + Symbol.nodeAsyncHooksAsyncId, + getAsyncHookSymbolPromiseProtoDesc(Symbol.nodeAsyncHooksAsyncId), + ); + Object.defineProperty( + PromiseProto, + Symbol.nodeAsyncHooksTriggerAsyncId, + getAsyncHookSymbolPromiseProtoDesc(Symbol.nodeAsyncHooksTriggerAsyncId), + ); + + if (Symbol.nodeAsyncHooksDestroyed) { + Object.defineProperty( + PromiseProto, + Symbol.nodeAsyncHooksDestroyed, + getAsyncHookSymbolPromiseProtoDesc(Symbol.nodeAsyncHooksDestroyed, true), + ); + } else if (withDestroy) { + // console.warn(`Couldn't find destroyed symbol to setup trap`); + } +}; diff --git a/packages/init/pre.js b/packages/init/pre.js index 134a7a87a5..88af15e0f0 100644 --- a/packages/init/pre.js +++ b/packages/init/pre.js @@ -1,2 +1,4 @@ // Generic preamble for all shims. + +import './node-async_hooks-patch.js'; import '@endo/lockdown'; diff --git a/packages/init/test/test-async_hooks.js b/packages/init/test/test-async_hooks.js new file mode 100644 index 0000000000..9cf7ce64d6 --- /dev/null +++ b/packages/init/test/test-async_hooks.js @@ -0,0 +1,79 @@ +/* global globalThis, $262 */ + +import '../index.js'; +import test from 'ava'; +import { createHook } from 'async_hooks'; +import { setTimeout } from 'timers'; + +const gcP = (async () => { + let gc = globalThis.gc || (typeof $262 !== 'undefined' ? $262.gc : null); + if (!gc) { + gc = () => { + Array.from({ length: 2 ** 24 }, () => Math.random()); + }; + } + return gc; +})(); + +test('async_hooks Promise patch', async t => { + const hasSymbols = + Symbol.nodeAsyncHooksAsyncId && Symbol.nodeAsyncHooksTriggerAsyncId; + let resolve; + const q = (() => { + const p1 = new Promise(r => (resolve = r)); + t.deepEqual( + Reflect.ownKeys(p1), + [], + `Promise instances don't start with any own keys`, + ); + harden(p1); + + // The `.then()` fulfillment triggers the "before" hook for `p2`, + // which enforces that `p2` is a tracked promise by installing async id symbols + const p2 = Promise.resolve().then(() => {}); + t.deepEqual( + Reflect.ownKeys(p2), + [], + `Promise instances don't start with any own keys`, + ); + harden(p2); + + const testHooks = createHook({ + init() {}, + before() {}, + // after() {}, + destroy() {}, + }); + testHooks.enable(); + + // Create a promise with symbols attached + const p3 = Promise.resolve(); + if (hasSymbols) { + t.truthy(Reflect.ownKeys(p3)); + } + + return Promise.resolve().then(() => { + resolve(8); + // ret is a tracked promise created from parent `p1` + // async_hooks will attempt to get the asyncId from `p1` + // which was created and frozen before the symbols were installed + const ret = p1.then(() => {}); + // Trigger attempting to get asyncId of `p1` again, which in current + // node versions will fail and generate a new one because of an own check + p1.then(() => {}); + + if (hasSymbols) { + t.truthy(Reflect.ownKeys(ret)); + } + + // testHooks.disable(); + + return ret; + }); + })(); + + return q + .then(() => new Promise(r => setTimeout(r, 0, gcP))) + .then(gc => gc()) + .then(() => new Promise(r => setTimeout(r))); +}); diff --git a/packages/ses/src/whitelist.js b/packages/ses/src/whitelist.js index 1e97daa71c..11f713aa06 100644 --- a/packages/ses/src/whitelist.js +++ b/packages/ses/src/whitelist.js @@ -504,6 +504,9 @@ export const whitelist = { keyFor: fn, match: 'symbol', matchAll: 'symbol', + nodeAsyncHooksAsyncId: 'symbol', + nodeAsyncHooksTriggerAsyncId: 'symbol', + nodeAsyncHooksDestroyed: 'symbol', prototype: '%SymbolPrototype%', replace: 'symbol', search: 'symbol', @@ -1292,6 +1295,10 @@ export const whitelist = { finally: fn, then: fn, '@@toStringTag': 'string', + // Non-standard, used in node to prevent async_hooks from breaking + '@@nodeAsyncHooksAsyncId': accessor, + '@@nodeAsyncHooksTriggerAsyncId': accessor, + '@@nodeAsyncHooksDestroyed': accessor, }, '%InertAsyncFunction%': { From 228b6bac61b7fcfbe06457622419a3f78cbefb7b Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Tue, 15 Mar 2022 20:08:37 +0000 Subject: [PATCH 02/11] fix CI --- .github/workflows/ci.yml | 6 +++--- packages/init/node-async_hooks.js | 2 -- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 20d4fb7910..f0135d4274 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -151,11 +151,11 @@ jobs: path: ${{ steps.yarn-cache-dir-path.outputs.dir }} key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }} - - name: Install dependencies - run: yarn --frozen-lockfile - # end macro + - name: Install dependencies + run: yarn --frozen-lockfile --ignore-engines + - name: Run yarn build run: yarn build diff --git a/packages/init/node-async_hooks.js b/packages/init/node-async_hooks.js index b102c19000..1c053c29ba 100644 --- a/packages/init/node-async_hooks.js +++ b/packages/init/node-async_hooks.js @@ -1,5 +1,3 @@ -// @ts-check - import { createHook, AsyncResource } from 'async_hooks'; /// From 427b3ba83eb55cfc39f08bf119fe34e18d4cea6b Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Tue, 15 Mar 2022 20:09:52 +0000 Subject: [PATCH 03/11] Upgrade boolean args to prop bags --- packages/init/node-async_hooks-patch.js | 2 +- packages/init/node-async_hooks.js | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/init/node-async_hooks-patch.js b/packages/init/node-async_hooks-patch.js index 6fe0ce4414..cfa3e6d83b 100644 --- a/packages/init/node-async_hooks-patch.js +++ b/packages/init/node-async_hooks-patch.js @@ -1,3 +1,3 @@ import { setup } from './node-async_hooks.js'; -setup(true); +setup({ withDestroy: true }); diff --git a/packages/init/node-async_hooks.js b/packages/init/node-async_hooks.js index 1c053c29ba..fee38aa65d 100644 --- a/packages/init/node-async_hooks.js +++ b/packages/init/node-async_hooks.js @@ -134,7 +134,7 @@ const findAsyncSymbolsFromPromiseCreateHook = () => { } }; -const getAsyncHookFallbackState = (promise, create) => { +const getAsyncHookFallbackState = (promise, { create = false } = {}) => { let state = promiseAsyncHookFallbackStates.get(promise); if (!state && create) { state = { @@ -150,7 +150,7 @@ const getAsyncHookFallbackState = (promise, create) => { }; const setAsyncIdFallback = (promise, symbol, value) => { - const state = getAsyncHookFallbackState(promise, true); + const state = getAsyncHookFallbackState(promise, { create: true }); if (state[symbol]) { if (state[symbol] !== value) { @@ -163,7 +163,10 @@ const setAsyncIdFallback = (promise, symbol, value) => { } }; -const getAsyncHookSymbolPromiseProtoDesc = (symbol, disallowGet) => ({ +const getAsyncHookSymbolPromiseProtoDesc = ( + symbol, + { disallowGet = false } = {}, +) => ({ set(value) { if (Object.isExtensible(this)) { Object.defineProperty(this, symbol, { @@ -181,14 +184,14 @@ const getAsyncHookSymbolPromiseProtoDesc = (symbol, disallowGet) => ({ if (disallowGet) { return undefined; } - const state = getAsyncHookFallbackState(this, false); + const state = getAsyncHookFallbackState(this, { create: false }); return state && state[symbol]; }, enumerable: false, configurable: true, }); -export const setup = (withDestroy = true) => { +export const setup = ({ withDestroy = true } = {}) => { if (withDestroy) { findAsyncSymbolsFromPromiseCreateHook(); } else { @@ -216,7 +219,9 @@ export const setup = (withDestroy = true) => { Object.defineProperty( PromiseProto, Symbol.nodeAsyncHooksDestroyed, - getAsyncHookSymbolPromiseProtoDesc(Symbol.nodeAsyncHooksDestroyed, true), + getAsyncHookSymbolPromiseProtoDesc(Symbol.nodeAsyncHooksDestroyed, { + disallowGet: true, + }), ); } else if (withDestroy) { // console.warn(`Couldn't find destroyed symbol to setup trap`); From 3eb7f3bec0115a6d89fc3d7f9e72254015e5972e Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Tue, 15 Mar 2022 20:17:52 +0000 Subject: [PATCH 04/11] Avoid console calls in async_hooks code --- packages/init/node-async_hooks.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/init/node-async_hooks.js b/packages/init/node-async_hooks.js index fee38aa65d..2b35e65ea3 100644 --- a/packages/init/node-async_hooks.js +++ b/packages/init/node-async_hooks.js @@ -18,7 +18,7 @@ const setAsyncSymbol = (description, symbol) => { Symbol[wellKnownName] = symbol; return true; } else if (Symbol[wellKnownName] !== symbol) { - // console.warn( + // process._rawDebug( // `Found duplicate ${description}:`, // symbol, // Symbol[wellKnownName], @@ -56,7 +56,7 @@ const findAsyncSymbolsFromPromiseCreateHook = () => { const bootstrapHook = createHook({ init(asyncId, type, triggerAsyncId, resource) { if (type !== 'PROMISE') return; - // console.log('Bootstrap', asyncId, triggerAsyncId, resource); + // process._rawDebug('Bootstrap', asyncId, triggerAsyncId, resource); bootstrapData.push({ asyncId, triggerAsyncId, resource }); }, destroy(_asyncId) { @@ -77,7 +77,7 @@ const findAsyncSymbolsFromPromiseCreateHook = () => { bootstrapData.length = 0; const { length } = promisesData; if (length > 1) { - // console.warn('Found multiple potential candidates'); + // process._rawDebug('Found multiple potential candidates'); } const promiseData = promisesData.find( @@ -86,7 +86,7 @@ const findAsyncSymbolsFromPromiseCreateHook = () => { if (promiseData) { bootstrapData.push(promiseData); } else if (length) { - // console.warn('No candidates matched'); + // process._rawDebug('No candidates matched'); } } @@ -107,7 +107,7 @@ const findAsyncSymbolsFromPromiseCreateHook = () => { // const { length } = symbols; let found = 0; // if (length !== 3) { - // console.error(`Found ${length} symbols on promise:`, ...symbols); + // process._rawDebug(`Found ${length} symbols on promise:`, ...symbols); // } symbols.forEach(symbol => { const value = resource[symbol]; @@ -119,7 +119,7 @@ const findAsyncSymbolsFromPromiseCreateHook = () => { } else if (typeof value === 'object' && 'destroyed' in value) { type = 'destroyed'; } else { - // console.error(`Unexpected symbol`, symbol); + // process._rawDebug(`Unexpected symbol`, symbol); return; } @@ -176,7 +176,7 @@ const getAsyncHookSymbolPromiseProtoDesc = ( enumerable: false, }); } else { - // console.log('fallback set of async id', symbol, value, new Error().stack); + // process._rawDebug('fallback set of async id', symbol, value, new Error().stack); setAsyncIdFallback(this, symbol, value); } }, @@ -199,7 +199,7 @@ export const setup = ({ withDestroy = true } = {}) => { } if (!Symbol.nodeAsyncHooksAsyncId || !Symbol.nodeAsyncHooksTriggerAsyncId) { - // console.log(`Async symbols not found, moving on`); + // process._rawDebug(`Async symbols not found, moving on`); return; } @@ -224,6 +224,6 @@ export const setup = ({ withDestroy = true } = {}) => { }), ); } else if (withDestroy) { - // console.warn(`Couldn't find destroyed symbol to setup trap`); + // process._rawDebug(`Couldn't find destroyed symbol to setup trap`); } }; From 668c82662794bea40ee3585cf14b62c5d1fad68e Mon Sep 17 00:00:00 2001 From: Mathieu Hofman <86499+mhofman@users.noreply.github.com> Date: Tue, 15 Mar 2022 19:05:12 -0700 Subject: [PATCH 05/11] Apply suggestions from code review Co-authored-by: Richard Gibson --- packages/init/node-async_hooks.js | 6 +++--- packages/init/test/test-async_hooks.js | 8 ++------ 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/init/node-async_hooks.js b/packages/init/node-async_hooks.js index 2b35e65ea3..c7e04a2dc6 100644 --- a/packages/init/node-async_hooks.js +++ b/packages/init/node-async_hooks.js @@ -53,11 +53,12 @@ const findAsyncSymbolsFromPromiseCreateHook = () => { const bootstrapData = []; { + const bootstrapHookData = []; const bootstrapHook = createHook({ init(asyncId, type, triggerAsyncId, resource) { if (type !== 'PROMISE') return; // process._rawDebug('Bootstrap', asyncId, triggerAsyncId, resource); - bootstrapData.push({ asyncId, triggerAsyncId, resource }); + bootstrapHookData.push({ asyncId, triggerAsyncId, resource }); }, destroy(_asyncId) { // Needs to be present to trigger the addition of the destroyed symbol @@ -71,10 +72,9 @@ const findAsyncSymbolsFromPromiseCreateHook = () => { // In some versions of Node, async_hooks don't give access to the resource // itself, but to a "wrapper" which is basically hooks metadata for the promise - const promisesData = bootstrapData.filter( + const promisesData = bootstrapHookData.filter( ({ resource }) => Promise.resolve(resource) === resource, ); - bootstrapData.length = 0; const { length } = promisesData; if (length > 1) { // process._rawDebug('Found multiple potential candidates'); diff --git a/packages/init/test/test-async_hooks.js b/packages/init/test/test-async_hooks.js index 9cf7ce64d6..fa76f403ed 100644 --- a/packages/init/test/test-async_hooks.js +++ b/packages/init/test/test-async_hooks.js @@ -48,9 +48,7 @@ test('async_hooks Promise patch', async t => { // Create a promise with symbols attached const p3 = Promise.resolve(); - if (hasSymbols) { - t.truthy(Reflect.ownKeys(p3)); - } + t.is(Reflect.ownKeys(p3).length > 0, hasSymbols); return Promise.resolve().then(() => { resolve(8); @@ -62,9 +60,7 @@ test('async_hooks Promise patch', async t => { // node versions will fail and generate a new one because of an own check p1.then(() => {}); - if (hasSymbols) { - t.truthy(Reflect.ownKeys(ret)); - } + t.is(Reflect.ownKeys(ret).length > 0, hasSymbols); // testHooks.disable(); From cee6c5f8fb33cf389a7898eff01b27aa83b74dba Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Wed, 16 Mar 2022 02:16:19 +0000 Subject: [PATCH 06/11] Split async hook bootstrap logic --- packages/init/node-async_hooks.js | 76 +++++++++++++++---------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/packages/init/node-async_hooks.js b/packages/init/node-async_hooks.js index c7e04a2dc6..225788ed07 100644 --- a/packages/init/node-async_hooks.js +++ b/packages/init/node-async_hooks.js @@ -49,48 +49,37 @@ const findAsyncSymbolsFromAsyncResource = () => { // To get the `destroyed` symbol installed on promises by async_hooks, // the only option is to create and enable an AsyncHook. // Different versions of Node handle this in various ways. -const findAsyncSymbolsFromPromiseCreateHook = () => { - const bootstrapData = []; - - { - const bootstrapHookData = []; - const bootstrapHook = createHook({ - init(asyncId, type, triggerAsyncId, resource) { - if (type !== 'PROMISE') return; - // process._rawDebug('Bootstrap', asyncId, triggerAsyncId, resource); - bootstrapHookData.push({ asyncId, triggerAsyncId, resource }); - }, - destroy(_asyncId) { - // Needs to be present to trigger the addition of the destroyed symbol - }, - }); +const getPromiseFromCreateHook = () => { + const bootstrapHookData = []; + const bootstrapHook = createHook({ + init(asyncId, type, triggerAsyncId, resource) { + if (type !== 'PROMISE') return; + // process._rawDebug('Bootstrap', asyncId, triggerAsyncId, resource); + bootstrapHookData.push({ asyncId, triggerAsyncId, resource }); + }, + destroy(_asyncId) { + // Needs to be present to trigger the addition of the destroyed symbol + }, + }); - bootstrapHook.enable(); - // Use a never resolving promise to avoid triggering settlement hooks - const trigger = new Promise(() => {}); - bootstrapHook.disable(); + bootstrapHook.enable(); + // Use a never resolving promise to avoid triggering settlement hooks + const trigger = new Promise(() => {}); + bootstrapHook.disable(); - // In some versions of Node, async_hooks don't give access to the resource - // itself, but to a "wrapper" which is basically hooks metadata for the promise - const promisesData = bootstrapHookData.filter( - ({ resource }) => Promise.resolve(resource) === resource, - ); - const { length } = promisesData; - if (length > 1) { - // process._rawDebug('Found multiple potential candidates'); - } - - const promiseData = promisesData.find( - ({ resource }) => resource === trigger, - ); - if (promiseData) { - bootstrapData.push(promiseData); - } else if (length) { - // process._rawDebug('No candidates matched'); - } + // In some versions of Node, async_hooks don't give access to the resource + // itself, but to a "wrapper" which is basically hooks metadata for the promise + const promisesData = bootstrapHookData.filter( + ({ resource }) => Promise.resolve(resource) === resource, + ); + const { length } = promisesData; + if (length > 1) { + // process._rawDebug('Found multiple potential candidates'); } - if (bootstrapData.length) { + const promiseData = promisesData.find(({ resource }) => resource === trigger); + + if (promiseData) { // Normally all promise hooks are disabled in a subsequent microtask // That means Node versions that modify promises at init will still // trigger our proto hooks for promises created in this turn @@ -101,8 +90,17 @@ const findAsyncSymbolsFromPromiseCreateHook = () => { const resetHook = createHook({}); resetHook.enable(); resetHook.disable(); + } else if (length) { + // process._rawDebug('No candidates matched'); + } + return promiseData; +}; + +const findAsyncSymbolsFromPromiseCreateHook = () => { + const bootstrapPromise = getPromiseFromCreateHook(); - const { asyncId, triggerAsyncId, resource } = bootstrapData.pop(); + if (bootstrapPromise) { + const { asyncId, triggerAsyncId, resource } = bootstrapPromise; const symbols = Object.getOwnPropertySymbols(resource); // const { length } = symbols; let found = 0; From 1ae93f6334bd4ff04bb994d9beace7f9bfdc4a09 Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Wed, 16 Mar 2022 02:38:43 +0000 Subject: [PATCH 07/11] Do not register async hook symbols as well-known --- packages/init/node-async_hooks-symbols.d.ts | 11 ---- packages/init/node-async_hooks.js | 58 ++++++++++++--------- packages/init/test/test-async_hooks.js | 8 +-- packages/ses/src/whitelist.js | 9 ++-- 4 files changed, 40 insertions(+), 46 deletions(-) delete mode 100644 packages/init/node-async_hooks-symbols.d.ts diff --git a/packages/init/node-async_hooks-symbols.d.ts b/packages/init/node-async_hooks-symbols.d.ts deleted file mode 100644 index 09fd7027de..0000000000 --- a/packages/init/node-async_hooks-symbols.d.ts +++ /dev/null @@ -1,11 +0,0 @@ -// @ts-check - -export {}; - -declare global { - interface SymbolConstructor { - readonly nodeAsyncHooksAsyncId: unique symbol; - readonly nodeAsyncHooksTriggerAsyncId: unique symbol; - readonly nodeAsyncHooksDestroyed: unique symbol; - } -} diff --git a/packages/init/node-async_hooks.js b/packages/init/node-async_hooks.js index 225788ed07..5949c8945f 100644 --- a/packages/init/node-async_hooks.js +++ b/packages/init/node-async_hooks.js @@ -1,27 +1,30 @@ import { createHook, AsyncResource } from 'async_hooks'; -/// - -const asyncHooksWellKnownNameFromDescription = { - async_id_symbol: 'nodeAsyncHooksAsyncId', - trigger_async_id_symbol: 'nodeAsyncHooksTriggerAsyncId', - destroyed: 'nodeAsyncHooksDestroyed', +const asyncHooksSymbols = { + async_id_symbol: undefined, + trigger_async_id_symbol: undefined, + destroyed: undefined, }; const promiseAsyncHookFallbackStates = new WeakMap(); const setAsyncSymbol = (description, symbol) => { - const wellKnownName = asyncHooksWellKnownNameFromDescription[description]; - if (!wellKnownName) { + if (!(description in asyncHooksSymbols)) { throw new Error('Unknown symbol'); - } else if (!Symbol[wellKnownName]) { - Symbol[wellKnownName] = symbol; + } else if (!asyncHooksSymbols[description]) { + if (symbol.description !== description) { + // Throw an error since the whitelist mechanism relies on the description + throw new Error( + `Mismatched symbol found for ${description}: ${String(symbol)}`, + ); + } + asyncHooksSymbols[description] = symbol; return true; - } else if (Symbol[wellKnownName] !== symbol) { + } else if (asyncHooksSymbols[description] !== symbol) { // process._rawDebug( // `Found duplicate ${description}:`, // symbol, - // Symbol[wellKnownName], + // asyncHooksSymbols[description], // ); return false; } else { @@ -37,7 +40,7 @@ const findAsyncSymbolsFromAsyncResource = () => { let found = 0; Object.getOwnPropertySymbols(new AsyncResource('Bootstrap')).forEach(sym => { const { description } = sym; - if (description in asyncHooksWellKnownNameFromDescription) { + if (description in asyncHooksSymbols) { if (setAsyncSymbol(description, sym)) { found += 1; } @@ -136,11 +139,11 @@ const getAsyncHookFallbackState = (promise, { create = false } = {}) => { let state = promiseAsyncHookFallbackStates.get(promise); if (!state && create) { state = { - [Symbol.nodeAsyncHooksAsyncId]: undefined, - [Symbol.nodeAsyncHooksTriggerAsyncId]: undefined, + [asyncHooksSymbols.async_id_symbol]: undefined, + [asyncHooksSymbols.trigger_async_id_symbol]: undefined, }; - if (Symbol.nodeAsyncHooksDestroyed) { - state[Symbol.nodeAsyncHooksDestroyed] = undefined; + if (asyncHooksSymbols.destroyed) { + state[asyncHooksSymbols.destroyed] = undefined; } promiseAsyncHookFallbackStates.set(promise, state); } @@ -196,7 +199,10 @@ export const setup = ({ withDestroy = true } = {}) => { findAsyncSymbolsFromAsyncResource(); } - if (!Symbol.nodeAsyncHooksAsyncId || !Symbol.nodeAsyncHooksTriggerAsyncId) { + if ( + !asyncHooksSymbols.async_id_symbol || + !asyncHooksSymbols.trigger_async_id_symbol + ) { // process._rawDebug(`Async symbols not found, moving on`); return; } @@ -204,20 +210,22 @@ export const setup = ({ withDestroy = true } = {}) => { const PromiseProto = Promise.prototype; Object.defineProperty( PromiseProto, - Symbol.nodeAsyncHooksAsyncId, - getAsyncHookSymbolPromiseProtoDesc(Symbol.nodeAsyncHooksAsyncId), + asyncHooksSymbols.async_id_symbol, + getAsyncHookSymbolPromiseProtoDesc(asyncHooksSymbols.async_id_symbol), ); Object.defineProperty( PromiseProto, - Symbol.nodeAsyncHooksTriggerAsyncId, - getAsyncHookSymbolPromiseProtoDesc(Symbol.nodeAsyncHooksTriggerAsyncId), + asyncHooksSymbols.trigger_async_id_symbol, + getAsyncHookSymbolPromiseProtoDesc( + asyncHooksSymbols.trigger_async_id_symbol, + ), ); - if (Symbol.nodeAsyncHooksDestroyed) { + if (asyncHooksSymbols.destroyed) { Object.defineProperty( PromiseProto, - Symbol.nodeAsyncHooksDestroyed, - getAsyncHookSymbolPromiseProtoDesc(Symbol.nodeAsyncHooksDestroyed, { + asyncHooksSymbols.destroyed, + getAsyncHookSymbolPromiseProtoDesc(asyncHooksSymbols.destroyed, { disallowGet: true, }), ); diff --git a/packages/init/test/test-async_hooks.js b/packages/init/test/test-async_hooks.js index fa76f403ed..7941da6e33 100644 --- a/packages/init/test/test-async_hooks.js +++ b/packages/init/test/test-async_hooks.js @@ -16,8 +16,8 @@ const gcP = (async () => { })(); test('async_hooks Promise patch', async t => { - const hasSymbols = - Symbol.nodeAsyncHooksAsyncId && Symbol.nodeAsyncHooksTriggerAsyncId; + const hasAsyncSymbols = + Object.getOwnPropertySymbols(Promise.prototype).length > 1; let resolve; const q = (() => { const p1 = new Promise(r => (resolve = r)); @@ -48,7 +48,7 @@ test('async_hooks Promise patch', async t => { // Create a promise with symbols attached const p3 = Promise.resolve(); - t.is(Reflect.ownKeys(p3).length > 0, hasSymbols); + t.is(Reflect.ownKeys(p3).length > 0, hasAsyncSymbols); return Promise.resolve().then(() => { resolve(8); @@ -60,7 +60,7 @@ test('async_hooks Promise patch', async t => { // node versions will fail and generate a new one because of an own check p1.then(() => {}); - t.is(Reflect.ownKeys(ret).length > 0, hasSymbols); + t.is(Reflect.ownKeys(ret).length > 0, hasAsyncSymbols); // testHooks.disable(); diff --git a/packages/ses/src/whitelist.js b/packages/ses/src/whitelist.js index 11f713aa06..71377e4080 100644 --- a/packages/ses/src/whitelist.js +++ b/packages/ses/src/whitelist.js @@ -504,9 +504,6 @@ export const whitelist = { keyFor: fn, match: 'symbol', matchAll: 'symbol', - nodeAsyncHooksAsyncId: 'symbol', - nodeAsyncHooksTriggerAsyncId: 'symbol', - nodeAsyncHooksDestroyed: 'symbol', prototype: '%SymbolPrototype%', replace: 'symbol', search: 'symbol', @@ -1296,9 +1293,9 @@ export const whitelist = { then: fn, '@@toStringTag': 'string', // Non-standard, used in node to prevent async_hooks from breaking - '@@nodeAsyncHooksAsyncId': accessor, - '@@nodeAsyncHooksTriggerAsyncId': accessor, - '@@nodeAsyncHooksDestroyed': accessor, + 'UniqueSymbol(async_id_symbol)': accessor, + 'UniqueSymbol(trigger_async_id_symbol)': accessor, + 'UniqueSymbol(destroyed)': accessor, }, '%InertAsyncFunction%': { From 98d1c7923ce849dda2e9935b5a43bbb4fae83544 Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Thu, 17 Mar 2022 01:39:07 +0000 Subject: [PATCH 08/11] Add failing test for @endo/init compat with bundle --- packages/init/jsconfig.json | 18 ++++++++++++++++++ packages/init/package.json | 2 ++ packages/init/test/.eslintrc.json | 5 +++++ packages/init/test/fixture/ses-boot.js | 2 ++ packages/init/test/test-bundle.js | 25 +++++++++++++++++++++++++ 5 files changed, 52 insertions(+) create mode 100644 packages/init/jsconfig.json create mode 100644 packages/init/test/.eslintrc.json create mode 100644 packages/init/test/fixture/ses-boot.js create mode 100644 packages/init/test/test-bundle.js diff --git a/packages/init/jsconfig.json b/packages/init/jsconfig.json new file mode 100644 index 0000000000..b2a58dc298 --- /dev/null +++ b/packages/init/jsconfig.json @@ -0,0 +1,18 @@ +{ + "compilerOptions": { + "target": "esnext", + "module": "esnext", + "noEmit": true, + "downlevelIteration": true, + "strictNullChecks": true, + "moduleResolution": "node" + }, + "include": [ + "*.js", + "*.ts", + "src/**/*.js", + "src/**/*.ts", + "test/**/*.js", + "test/**/*.ts" + ] +} diff --git a/packages/init/package.json b/packages/init/package.json index e66bc248e8..c248ab86d1 100644 --- a/packages/init/package.json +++ b/packages/init/package.json @@ -21,6 +21,8 @@ "lint": "eslint '**/*.js'" }, "devDependencies": { + "@endo/compartment-mapper": "0.7.1", + "@endo/ses-ava": "0.2.21", "ava": "^3.12.1" }, "dependencies": { diff --git a/packages/init/test/.eslintrc.json b/packages/init/test/.eslintrc.json new file mode 100644 index 0000000000..5a0565b8cb --- /dev/null +++ b/packages/init/test/.eslintrc.json @@ -0,0 +1,5 @@ +{ + "rules": { + "import/no-extraneous-dependencies": ["error", { "devDependencies": true }] + } +} diff --git a/packages/init/test/fixture/ses-boot.js b/packages/init/test/fixture/ses-boot.js new file mode 100644 index 0000000000..9e9c13616b --- /dev/null +++ b/packages/init/test/fixture/ses-boot.js @@ -0,0 +1,2 @@ +// eslint-disable-next-line import/no-extraneous-dependencies +import '@endo/init'; diff --git a/packages/init/test/test-bundle.js b/packages/init/test/test-bundle.js new file mode 100644 index 0000000000..05d801a507 --- /dev/null +++ b/packages/init/test/test-bundle.js @@ -0,0 +1,25 @@ +// @ts-check + +import fs from 'fs'; +import url, { URL } from 'url'; +import crypto from 'crypto'; + +import '../debug.js'; +import { wrapTest } from '@endo/ses-ava'; +import rawTest from 'ava'; + +import { makeArchive } from '@endo/compartment-mapper'; +import { makeReadPowers } from '@endo/compartment-mapper/node-powers.js'; + +const test = wrapTest(rawTest); + +test.failing('Can be bundled', async t => { + const powers = makeReadPowers({ fs, url, crypto }); + + await t.notThrowsAsync(() => + makeArchive( + powers, + new URL('./fixture/ses-boot.js', import.meta.url).toString(), + ), + ); +}); From f431f1d749a17af1936003ee2a7ee84f7358e382 Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Thu, 17 Mar 2022 02:03:05 +0000 Subject: [PATCH 09/11] Use conditional exports to only perform async_hooks patch under node --- packages/init/debug-node.js | 6 ++++++ packages/init/index-node.js | 6 ++++++ packages/init/package.json | 25 ++++++++++++++++++++----- packages/init/pre-bundle-source-node.js | 7 +++++++ packages/init/pre-node.js | 4 ++++ packages/init/pre-remoting-node.js | 5 +++++ packages/init/pre.js | 1 - packages/init/test/test-async_hooks.js | 2 +- packages/init/test/test-bundle.js | 2 +- 9 files changed, 50 insertions(+), 8 deletions(-) create mode 100644 packages/init/debug-node.js create mode 100644 packages/init/index-node.js create mode 100644 packages/init/pre-bundle-source-node.js create mode 100644 packages/init/pre-node.js create mode 100644 packages/init/pre-remoting-node.js diff --git a/packages/init/debug-node.js b/packages/init/debug-node.js new file mode 100644 index 0000000000..b2a06d9665 --- /dev/null +++ b/packages/init/debug-node.js @@ -0,0 +1,6 @@ +// debug.js - call lockdown with default Agoric shims + +// Install our HandledPromise global. +import './pre-remoting-node.js'; + +export * from '@endo/lockdown/commit-debug.js'; diff --git a/packages/init/index-node.js b/packages/init/index-node.js new file mode 100644 index 0000000000..f13bc60f4a --- /dev/null +++ b/packages/init/index-node.js @@ -0,0 +1,6 @@ +// index.js - call lockdown with default Agoric shims + +// Install our HandledPromise global. +import './pre-remoting-node.js'; + +export * from '@endo/lockdown/commit.js'; diff --git a/packages/init/package.json b/packages/init/package.json index c248ab86d1..e2fbcc8682 100644 --- a/packages/init/package.json +++ b/packages/init/package.json @@ -5,11 +5,26 @@ "type": "module", "main": "index.js", "exports": { - ".": "./index.js", - "./debug.js": "./debug.js", - "./pre.js": "./pre.js", - "./pre-remoting.js": "./pre-remoting.js", - "./pre-bundle-source.js": "./pre-bundle-source.js", + ".": { + "node": "./index-node.js", + "default": "./index.js" + }, + "./debug.js": { + "node": "./debug-node.js", + "default": "./debug.js" + }, + "./pre.js": { + "node": "./pre-node.js", + "default": "./pre.js" + }, + "./pre-remoting.js": { + "node": "./pre-remoting-node.js", + "default": "./pre-remoting.js" + }, + "./pre-bundle-source.js": { + "node": "./pre-bundle-source-node.js", + "default": "./pre-bundle-source.js" + }, "./package.json": "./package.json" }, "scripts": { diff --git a/packages/init/pre-bundle-source-node.js b/packages/init/pre-bundle-source-node.js new file mode 100644 index 0000000000..166cb2f4a5 --- /dev/null +++ b/packages/init/pre-bundle-source-node.js @@ -0,0 +1,7 @@ +// pre-bundle-source.js - initialization to use @endo/bundle-source +// DEPRECATED: no longer necessary, imports of this module can be replaced with +// import '@endo/init'; +// or if further vetted shim initialization is needed: +// import '@endo/init/pre.js'; + +import './pre-node.js'; diff --git a/packages/init/pre-node.js b/packages/init/pre-node.js new file mode 100644 index 0000000000..88af15e0f0 --- /dev/null +++ b/packages/init/pre-node.js @@ -0,0 +1,4 @@ +// Generic preamble for all shims. + +import './node-async_hooks-patch.js'; +import '@endo/lockdown'; diff --git a/packages/init/pre-remoting-node.js b/packages/init/pre-remoting-node.js new file mode 100644 index 0000000000..0ea85e73f3 --- /dev/null +++ b/packages/init/pre-remoting-node.js @@ -0,0 +1,5 @@ +// pre-remoting.js - shims necessary to use @endo/far + +import './pre-node.js'; + +export * from '@endo/eventual-send/shim.js'; diff --git a/packages/init/pre.js b/packages/init/pre.js index 88af15e0f0..c02dfb1c9c 100644 --- a/packages/init/pre.js +++ b/packages/init/pre.js @@ -1,4 +1,3 @@ // Generic preamble for all shims. -import './node-async_hooks-patch.js'; import '@endo/lockdown'; diff --git a/packages/init/test/test-async_hooks.js b/packages/init/test/test-async_hooks.js index 7941da6e33..cdee9aa7b7 100644 --- a/packages/init/test/test-async_hooks.js +++ b/packages/init/test/test-async_hooks.js @@ -1,6 +1,6 @@ /* global globalThis, $262 */ -import '../index.js'; +import '@endo/init'; import test from 'ava'; import { createHook } from 'async_hooks'; import { setTimeout } from 'timers'; diff --git a/packages/init/test/test-bundle.js b/packages/init/test/test-bundle.js index 05d801a507..54c464a267 100644 --- a/packages/init/test/test-bundle.js +++ b/packages/init/test/test-bundle.js @@ -13,7 +13,7 @@ import { makeReadPowers } from '@endo/compartment-mapper/node-powers.js'; const test = wrapTest(rawTest); -test.failing('Can be bundled', async t => { +test('Can be bundled', async t => { const powers = makeReadPowers({ fs, url, crypto }); await t.notThrowsAsync(() => From 4b73064eb6a9db360ddf5b7f9d7359d54e894946 Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Thu, 17 Mar 2022 13:24:36 +0000 Subject: [PATCH 10/11] Use package self-reference to reduce clunk of conditonal exports --- packages/init/debug-node.js | 6 ------ packages/init/index-node.js | 6 ------ packages/init/package.json | 20 ++++---------------- packages/init/pre-bundle-source-node.js | 7 ------- packages/init/pre-bundle-source.js | 4 +++- packages/init/pre-remoting-node.js | 5 ----- packages/init/pre-remoting.js | 4 +++- packages/init/test/test-async_hooks.js | 2 ++ 8 files changed, 12 insertions(+), 42 deletions(-) delete mode 100644 packages/init/debug-node.js delete mode 100644 packages/init/index-node.js delete mode 100644 packages/init/pre-bundle-source-node.js delete mode 100644 packages/init/pre-remoting-node.js diff --git a/packages/init/debug-node.js b/packages/init/debug-node.js deleted file mode 100644 index b2a06d9665..0000000000 --- a/packages/init/debug-node.js +++ /dev/null @@ -1,6 +0,0 @@ -// debug.js - call lockdown with default Agoric shims - -// Install our HandledPromise global. -import './pre-remoting-node.js'; - -export * from '@endo/lockdown/commit-debug.js'; diff --git a/packages/init/index-node.js b/packages/init/index-node.js deleted file mode 100644 index f13bc60f4a..0000000000 --- a/packages/init/index-node.js +++ /dev/null @@ -1,6 +0,0 @@ -// index.js - call lockdown with default Agoric shims - -// Install our HandledPromise global. -import './pre-remoting-node.js'; - -export * from '@endo/lockdown/commit.js'; diff --git a/packages/init/package.json b/packages/init/package.json index e2fbcc8682..e3562a603e 100644 --- a/packages/init/package.json +++ b/packages/init/package.json @@ -5,26 +5,14 @@ "type": "module", "main": "index.js", "exports": { - ".": { - "node": "./index-node.js", - "default": "./index.js" - }, - "./debug.js": { - "node": "./debug-node.js", - "default": "./debug.js" - }, + ".": "./index.js", + "./debug.js": "./debug.js", "./pre.js": { "node": "./pre-node.js", "default": "./pre.js" }, - "./pre-remoting.js": { - "node": "./pre-remoting-node.js", - "default": "./pre-remoting.js" - }, - "./pre-bundle-source.js": { - "node": "./pre-bundle-source-node.js", - "default": "./pre-bundle-source.js" - }, + "./pre-remoting.js": "./pre-remoting.js", + "./pre-bundle-source.js": "./pre-bundle-source.js", "./package.json": "./package.json" }, "scripts": { diff --git a/packages/init/pre-bundle-source-node.js b/packages/init/pre-bundle-source-node.js deleted file mode 100644 index 166cb2f4a5..0000000000 --- a/packages/init/pre-bundle-source-node.js +++ /dev/null @@ -1,7 +0,0 @@ -// pre-bundle-source.js - initialization to use @endo/bundle-source -// DEPRECATED: no longer necessary, imports of this module can be replaced with -// import '@endo/init'; -// or if further vetted shim initialization is needed: -// import '@endo/init/pre.js'; - -import './pre-node.js'; diff --git a/packages/init/pre-bundle-source.js b/packages/init/pre-bundle-source.js index 400d329cd3..85f432bb0c 100644 --- a/packages/init/pre-bundle-source.js +++ b/packages/init/pre-bundle-source.js @@ -4,4 +4,6 @@ // or if further vetted shim initialization is needed: // import '@endo/init/pre.js'; -import './pre.js'; +// Use a package self-reference to go through the "exports" resolution +// eslint-disable-next-line import/no-extraneous-dependencies +import '@endo/init/pre.js'; diff --git a/packages/init/pre-remoting-node.js b/packages/init/pre-remoting-node.js deleted file mode 100644 index 0ea85e73f3..0000000000 --- a/packages/init/pre-remoting-node.js +++ /dev/null @@ -1,5 +0,0 @@ -// pre-remoting.js - shims necessary to use @endo/far - -import './pre-node.js'; - -export * from '@endo/eventual-send/shim.js'; diff --git a/packages/init/pre-remoting.js b/packages/init/pre-remoting.js index 5e1b21e230..52efb6426f 100644 --- a/packages/init/pre-remoting.js +++ b/packages/init/pre-remoting.js @@ -1,5 +1,7 @@ // pre-remoting.js - shims necessary to use @endo/far -import './pre.js'; +// Use a package self-reference to go through the "exports" resolution +// eslint-disable-next-line import/no-extraneous-dependencies +import '@endo/init/pre.js'; export * from '@endo/eventual-send/shim.js'; diff --git a/packages/init/test/test-async_hooks.js b/packages/init/test/test-async_hooks.js index cdee9aa7b7..2f16211ab0 100644 --- a/packages/init/test/test-async_hooks.js +++ b/packages/init/test/test-async_hooks.js @@ -1,5 +1,7 @@ /* global globalThis, $262 */ +// Use a package self-reference to go through the "exports" resolution +// eslint-disable-next-line import/no-extraneous-dependencies import '@endo/init'; import test from 'ava'; import { createHook } from 'async_hooks'; From df965cbb409a3996926e0b0b7cca05dd1df49ed6 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Fri, 18 Mar 2022 20:54:24 -0700 Subject: [PATCH 11/11] fix: add promiseTaming: "protect" --- packages/ses/index.d.ts | 1 + packages/ses/src/lockdown-shim.js | 7 ++ packages/ses/src/tame-promise.js | 70 +++++++++++++++++++ packages/ses/src/whitelist.js | 4 ++ .../test/test-tame-promise-min-override.js | 54 ++++++++++++++ packages/ses/test/test-tame-promise-unsafe.js | 57 +++++++++++++++ packages/ses/test/test-tame-promise.js | 54 ++++++++++++++ 7 files changed, 247 insertions(+) create mode 100644 packages/ses/src/tame-promise.js create mode 100644 packages/ses/test/test-tame-promise-min-override.js create mode 100644 packages/ses/test/test-tame-promise-unsafe.js create mode 100644 packages/ses/test/test-tame-promise.js diff --git a/packages/ses/index.d.ts b/packages/ses/index.d.ts index d06b0993c0..c92cb06b56 100644 --- a/packages/ses/index.d.ts +++ b/packages/ses/index.d.ts @@ -28,6 +28,7 @@ export interface LockdownOptions { overrideTaming?: 'moderate' | 'min' | 'severe'; overrideDebug?: Array; domainTaming?: 'safe' | 'unsafe'; + promiseTaming?: 'safe' | 'unsafe'; __allowUnsafeMonkeyPatching__?: 'safe' | 'unsafe'; } diff --git a/packages/ses/src/lockdown-shim.js b/packages/ses/src/lockdown-shim.js index a9bc7ca78d..b70887d421 100644 --- a/packages/ses/src/lockdown-shim.js +++ b/packages/ses/src/lockdown-shim.js @@ -45,6 +45,7 @@ import { makeSafeEvaluator } from './make-safe-evaluator.js'; import { initialGlobalPropertyNames } from './whitelist.js'; import { tameFunctionToString } from './tame-function-tostring.js'; import { tameDomains } from './tame-domains.js'; +import { tamePromise } from './tame-promise.js'; import { tameConsole } from './error/tame-console.js'; import tameErrorConstructor from './error/tame-error-constructor.js'; @@ -166,6 +167,7 @@ export const repairIntrinsics = (options = {}) => { overrideTaming = getenv('LOCKDOWN_OVERRIDE_TAMING', 'moderate'), stackFiltering = getenv('LOCKDOWN_STACK_FILTERING', 'concise'), domainTaming = getenv('LOCKDOWN_DOMAIN_TAMING', 'safe'), + promiseTaming = getenv('LOCKDOWN_PROMISE_TAMING', 'safe'), evalTaming = getenv('LOCKDOWN_EVAL_TAMING', 'safeEval'), overrideDebug = arrayFilter( stringSplit(getenv('LOCKDOWN_OVERRIDE_DEBUG', ''), ','), @@ -377,6 +379,11 @@ export const repairIntrinsics = (options = {}) => { // @ts-ignore enablePropertyOverrides does its own input validation enablePropertyOverrides(intrinsics, overrideTaming, overrideDebug); + // tamePromise happens after enablePropertyOverrides and thus must + // cope with `Promise.prototype.constructor` being either + // a data property or an accessor property. + tamePromise(promiseTaming); + if (__allowUnsafeMonkeyPatching__ !== 'unsafe') { // Finally register and optionally freeze all the intrinsics. This // must be the operation that modifies the intrinsics. diff --git a/packages/ses/src/tame-promise.js b/packages/ses/src/tame-promise.js new file mode 100644 index 0000000000..d5dd500752 --- /dev/null +++ b/packages/ses/src/tame-promise.js @@ -0,0 +1,70 @@ +// @ts-check + +import { + Promise, + TypeError, + defineProperties, + objectHasOwnProperty, + freeze, + promisePrototype, + getPrototypeOf, + setPrototypeOf, + construct, +} from './commons.js'; + +const originalThen = promisePrototype.then; + +export const tamePromise = (promiseTaming = 'safe') => { + if (promiseTaming !== 'safe' && promiseTaming !== 'unsafe') { + throw new TypeError(`unrecognized promiseTaming ${promiseTaming}`); + } + if (promiseTaming === 'unsafe') { + return; + } + + function FakePromiseConstructor(...args) { + return construct(Promise, args, Promise); + } + defineProperties(FakePromiseConstructor, { + prototype: { + value: promisePrototype, + }, + }); + setPrototypeOf(FakePromiseConstructor, Promise); + freeze(FakePromiseConstructor); + + // At this point, enable-property-overrides may or may not have + // make `Promise.prototype.constructor` into an accessor. If it is + // a data property, we need to turn it into an accessor with the following + // getter and an `undefined` setter, so that after freezing it will + // have the same override mistake problem that the data property would + // have. + // If it is already an accessor property, we need to replace the getter with + // the following getter, but leave the setter that was presumably + // installed by enable-property-overrides. + const constructorDescDelta = { + get() { + freeze(this); + if ( + getPrototypeOf(this) === promisePrototype && + !objectHasOwnProperty(this, 'then') && + this.then === originalThen + ) { + return Promise; + } + return FakePromiseConstructor; + }, + }; + defineProperties(constructorDescDelta.get, { + originalValue: { + value: Promise, + writable: false, + enumerable: false, + configurable: false, + }, + }); + + defineProperties(promisePrototype, { + constructor: constructorDescDelta, + }); +}; diff --git a/packages/ses/src/whitelist.js b/packages/ses/src/whitelist.js index 71377e4080..8e8c1de908 100644 --- a/packages/ses/src/whitelist.js +++ b/packages/ses/src/whitelist.js @@ -1288,6 +1288,10 @@ export const whitelist = { '%PromisePrototype%': { // Properties of the Promise Prototype Object catch: fn, + // Note that tame-promise may turn this `constructor` property + // into an accessor. + // But it would do this after `whitelistIntrinsics` so 'Promise' + // remains the correct setting here in the whitelist. constructor: 'Promise', finally: fn, then: fn, diff --git a/packages/ses/test/test-tame-promise-min-override.js b/packages/ses/test/test-tame-promise-min-override.js new file mode 100644 index 0000000000..7eb004a65c --- /dev/null +++ b/packages/ses/test/test-tame-promise-min-override.js @@ -0,0 +1,54 @@ +// @ts-check + +import '../index.js'; +import test from 'ava'; + +const { setPrototypeOf, defineProperty } = Object; + +lockdown({ promiseTaming: 'safe', overrideTaming: 'min' }); + +const normalPromise = Promise.resolve('normal'); +const thenable = { then: onSuccess => onSuccess('thenable') }; +const badOwnPromise = Promise.resolve('badOwnPromise'); +defineProperty(badOwnPromise, 'then', { + value: onSuccess => onSuccess('badThenResult'), +}); + +// Even though `badSubPromise` has no own `then` property, +// it still inherits a bad one. +const badSubPromise = Promise.resolve('badSubPromise'); +setPrototypeOf(badSubPromise, badOwnPromise); + +// Even though `badSpacerPromise.then` is the original `then` now, +// and it is not an own property, and it is inherited from +// the real Promise.prototype, it is still not reliable. +// The intermediate object on the prototype chain means that +// `then` be something else later. +const spacerProto = { __proto__: Promise.prototype }; +const badSpacerPromise = Promise.resolve('badSpacerPromise'); +setPrototypeOf(badSpacerPromise, spacerProto); + +test('tamePromise safe (min override) Promise.resolve', t => { + t.is(Promise.resolve(normalPromise), normalPromise); + // @ts-ignore It isn't supposed to match, so inconsistent types are ok. + t.not(Promise.resolve(thenable), thenable); + t.not(Promise.resolve(badOwnPromise), badOwnPromise); + t.not(Promise.resolve(badSubPromise), badSubPromise); + t.not(Promise.resolve(badSpacerPromise), badSpacerPromise); +}); + +const victim = p => { + let i = 'good'; + Promise.resolve(p).then(() => { + i = 'bad'; + }); + return i; +}; + +test('tamePromise safe (min override) reentrancy', t => { + t.is(victim(normalPromise), 'good'); + t.is(victim(thenable), 'good'); + t.is(victim(badOwnPromise), 'good'); + t.is(victim(badSubPromise), 'good'); + t.is(victim(badSpacerPromise), 'good'); +}); diff --git a/packages/ses/test/test-tame-promise-unsafe.js b/packages/ses/test/test-tame-promise-unsafe.js new file mode 100644 index 0000000000..7e46316d34 --- /dev/null +++ b/packages/ses/test/test-tame-promise-unsafe.js @@ -0,0 +1,57 @@ +// @ts-check + +import '../index.js'; +import test from 'ava'; + +const { setPrototypeOf, defineProperty } = Object; + +lockdown({ promiseTaming: 'unsafe' }); + +const normalPromise = Promise.resolve('normal'); +const thenable = { then: onSuccess => onSuccess('thenable') }; +const badOwnPromise = Promise.resolve('badOwnPromise'); +defineProperty(badOwnPromise, 'then', { + value: onSuccess => onSuccess('badThenResult'), +}); + +// Even though `badSubPromise` has no own `then` property, +// it still inherits a bad one. +const badSubPromise = Promise.resolve('badSubPromise'); +setPrototypeOf(badSubPromise, badOwnPromise); + +// Even though `badSpacerPromise.then` is the original `then` now, +// and it is not an own property, and it is inherited from +// the real Promise.prototype, it is still not reliable. +// The intermediate object on the prototype chain means that +// `then` be something else later. +const spacerProto = { __proto__: Promise.prototype }; +const badSpacerPromise = Promise.resolve('badSpacerPromise'); +setPrototypeOf(badSpacerPromise, spacerProto); + +test('tamePromise unsafe Promise.resolve', t => { + t.is(Promise.resolve(normalPromise), normalPromise); + // @ts-ignore It isn't supposed to match, so inconsistent types are ok. + t.not(Promise.resolve(thenable), thenable); + t.is(Promise.resolve(badOwnPromise), badOwnPromise); + t.is(Promise.resolve(badSubPromise), badSubPromise); + t.is(Promise.resolve(badSpacerPromise), badSpacerPromise); +}); + +const victim = p => { + let i = 'good'; + Promise.resolve(p).then(() => { + i = 'bad'; + }); + return i; +}; + +test('tamePromise unsafe reentrancy', t => { + t.is(victim(normalPromise), 'good'); + t.is(victim(thenable), 'good'); + t.is(victim(badOwnPromise), 'bad'); + t.is(victim(badSubPromise), 'bad'); + defineProperty(spacerProto, 'then', { + value: onSuccess => onSuccess('space attack'), + }); + t.is(victim(badSpacerPromise), 'bad'); +}); diff --git a/packages/ses/test/test-tame-promise.js b/packages/ses/test/test-tame-promise.js new file mode 100644 index 0000000000..d9527c2e8a --- /dev/null +++ b/packages/ses/test/test-tame-promise.js @@ -0,0 +1,54 @@ +// @ts-check + +import '../index.js'; +import test from 'ava'; + +const { setPrototypeOf, defineProperty } = Object; + +lockdown({ promiseTaming: 'safe' }); + +const normalPromise = Promise.resolve('normal'); +const thenable = { then: onSuccess => onSuccess('thenable') }; +const badOwnPromise = Promise.resolve('badOwnPromise'); +defineProperty(badOwnPromise, 'then', { + value: onSuccess => onSuccess('badThenResult'), +}); + +// Even though `badSubPromise` has no own `then` property, +// it still inherits a bad one. +const badSubPromise = Promise.resolve('badSubPromise'); +setPrototypeOf(badSubPromise, badOwnPromise); + +// Even though `badSpacerPromise.then` is the original `then` now, +// and it is not an own property, and it is inherited from +// the real Promise.prototype, it is still not reliable. +// The intermediate object on the prototype chain means that +// `then` be something else later. +const spacerProto = { __proto__: Promise.prototype }; +const badSpacerPromise = Promise.resolve('badSpacerPromise'); +setPrototypeOf(badSpacerPromise, spacerProto); + +test('tamePromise safe Promise.resolve', t => { + t.is(Promise.resolve(normalPromise), normalPromise); + // @ts-ignore It isn't supposed to match, so inconsistent types are ok. + t.not(Promise.resolve(thenable), thenable); + t.not(Promise.resolve(badOwnPromise), badOwnPromise); + t.not(Promise.resolve(badSubPromise), badSubPromise); + t.not(Promise.resolve(badSpacerPromise), badSpacerPromise); +}); + +const victim = p => { + let i = 'good'; + Promise.resolve(p).then(() => { + i = 'bad'; + }); + return i; +}; + +test('tamePromise safe reentrancy', t => { + t.is(victim(normalPromise), 'good'); + t.is(victim(thenable), 'good'); + t.is(victim(badOwnPromise), 'good'); + t.is(victim(badSubPromise), 'good'); + t.is(victim(badSpacerPromise), 'good'); +});