Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make Promise.resolve safe. #1126

Closed
wants to merge 11 commits into from
55 changes: 55 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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') }}

# end macro

- name: Install dependencies
run: yarn --frozen-lockfile --ignore-engines

- name: Run yarn build
run: yarn build

- name: Run yarn test (@endo/init)
working-directory: packages/init
run: yarn test

cover:
name: cover

Expand Down
18 changes: 18 additions & 0 deletions packages/init/jsconfig.json
Original file line number Diff line number Diff line change
@@ -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"
]
}
3 changes: 3 additions & 0 deletions packages/init/node-async_hooks-patch.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { setup } from './node-async_hooks.js';

setup({ withDestroy: true });
235 changes: 235 additions & 0 deletions packages/init/node-async_hooks.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
import { createHook, AsyncResource } from 'async_hooks';

const asyncHooksSymbols = {
async_id_symbol: undefined,
trigger_async_id_symbol: undefined,
destroyed: undefined,
};

const promiseAsyncHookFallbackStates = new WeakMap();

const setAsyncSymbol = (description, symbol) => {
if (!(description in asyncHooksSymbols)) {
throw new Error('Unknown 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 (asyncHooksSymbols[description] !== symbol) {
// process._rawDebug(
// `Found duplicate ${description}:`,
// symbol,
// asyncHooksSymbols[description],
// );
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 asyncHooksSymbols) {
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 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();

// 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) {
// 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();
} else if (length) {
// process._rawDebug('No candidates matched');
}
return promiseData;
};

const findAsyncSymbolsFromPromiseCreateHook = () => {
const bootstrapPromise = getPromiseFromCreateHook();

if (bootstrapPromise) {
const { asyncId, triggerAsyncId, resource } = bootstrapPromise;
const symbols = Object.getOwnPropertySymbols(resource);
// const { length } = symbols;
let found = 0;
// if (length !== 3) {
// process._rawDebug(`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 {
// process._rawDebug(`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 = false } = {}) => {
let state = promiseAsyncHookFallbackStates.get(promise);
if (!state && create) {
state = {
[asyncHooksSymbols.async_id_symbol]: undefined,
[asyncHooksSymbols.trigger_async_id_symbol]: undefined,
};
if (asyncHooksSymbols.destroyed) {
state[asyncHooksSymbols.destroyed] = undefined;
}
promiseAsyncHookFallbackStates.set(promise, state);
}
return state;
};

const setAsyncIdFallback = (promise, symbol, value) => {
const state = getAsyncHookFallbackState(promise, { create: 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 = false } = {},
) => ({
set(value) {
if (Object.isExtensible(this)) {
Object.defineProperty(this, symbol, {
value,
writable: false,
configurable: false,
enumerable: false,
});
} else {
// process._rawDebug('fallback set of async id', symbol, value, new Error().stack);
setAsyncIdFallback(this, symbol, value);
}
},
get() {
if (disallowGet) {
return undefined;
}
const state = getAsyncHookFallbackState(this, { create: false });
return state && state[symbol];
},
enumerable: false,
configurable: true,
});

export const setup = ({ withDestroy = true } = {}) => {
if (withDestroy) {
findAsyncSymbolsFromPromiseCreateHook();
} else {
findAsyncSymbolsFromAsyncResource();
}

if (
!asyncHooksSymbols.async_id_symbol ||
!asyncHooksSymbols.trigger_async_id_symbol
) {
// process._rawDebug(`Async symbols not found, moving on`);
return;
}

const PromiseProto = Promise.prototype;
Object.defineProperty(
PromiseProto,
asyncHooksSymbols.async_id_symbol,
getAsyncHookSymbolPromiseProtoDesc(asyncHooksSymbols.async_id_symbol),
);
Object.defineProperty(
PromiseProto,
asyncHooksSymbols.trigger_async_id_symbol,
getAsyncHookSymbolPromiseProtoDesc(
asyncHooksSymbols.trigger_async_id_symbol,
),
);

if (asyncHooksSymbols.destroyed) {
Object.defineProperty(
PromiseProto,
asyncHooksSymbols.destroyed,
getAsyncHookSymbolPromiseProtoDesc(asyncHooksSymbols.destroyed, {
disallowGet: true,
}),
);
} else if (withDestroy) {
// process._rawDebug(`Couldn't find destroyed symbol to setup trap`);
}
};
7 changes: 6 additions & 1 deletion packages/init/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
"exports": {
".": "./index.js",
"./debug.js": "./debug.js",
"./pre.js": "./pre.js",
"./pre.js": {
"node": "./pre-node.js",
"default": "./pre.js"
},
"./pre-remoting.js": "./pre-remoting.js",
"./pre-bundle-source.js": "./pre-bundle-source.js",
"./package.json": "./package.json"
Expand All @@ -21,6 +24,8 @@
"lint": "eslint '**/*.js'"
},
"devDependencies": {
"@endo/compartment-mapper": "0.7.1",
"@endo/ses-ava": "0.2.21",
"ava": "^3.12.1"
},
"dependencies": {
Expand Down
4 changes: 3 additions & 1 deletion packages/init/pre-bundle-source.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
4 changes: 4 additions & 0 deletions packages/init/pre-node.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Generic preamble for all shims.

import './node-async_hooks-patch.js';
import '@endo/lockdown';
4 changes: 3 additions & 1 deletion packages/init/pre-remoting.js
Original file line number Diff line number Diff line change
@@ -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';
1 change: 1 addition & 0 deletions packages/init/pre.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
// Generic preamble for all shims.

import '@endo/lockdown';
5 changes: 5 additions & 0 deletions packages/init/test/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"rules": {
"import/no-extraneous-dependencies": ["error", { "devDependencies": true }]
}
}
2 changes: 2 additions & 0 deletions packages/init/test/fixture/ses-boot.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// eslint-disable-next-line import/no-extraneous-dependencies
import '@endo/init';
Loading