From c96e1d2f9728c5dfc9517cfb7364e15615bf0664 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 28 May 2024 13:59:28 +0000 Subject: [PATCH] fix(xsnap): Avoid snapshot data memory leak (#9405) Fixes #9316 Add testing code to observe memory leaks related to xsnap snapshots, and fix the leak that retains them. None. None. None. Covered! None (this is kernel code). --- packages/xsnap/package.json | 1 + packages/xsnap/src/xsnap.js | 184 +++++++++++++++----------- packages/xsnap/test/leakiness.mjs | 183 +++++++++++++++++++++++++ packages/xsnap/test/leakiness.test.js | 90 +++++++++++++ 4 files changed, 379 insertions(+), 79 deletions(-) create mode 100644 packages/xsnap/test/leakiness.mjs create mode 100644 packages/xsnap/test/leakiness.test.js diff --git a/packages/xsnap/package.json b/packages/xsnap/package.json index 060d83c70d0..269f52407f4 100644 --- a/packages/xsnap/package.json +++ b/packages/xsnap/package.json @@ -42,6 +42,7 @@ }, "devDependencies": { "@endo/base64": "0.2.31", + "@endo/nat": "4.1.27", "ava": "^5.2.0", "c8": "^7.13.0" }, diff --git a/packages/xsnap/src/xsnap.js b/packages/xsnap/src/xsnap.js index 7f0a2c06e4c..4434cf3e559 100644 --- a/packages/xsnap/src/xsnap.js +++ b/packages/xsnap/src/xsnap.js @@ -55,6 +55,93 @@ function echoCommand(arg) { const safeHintFromDescription = description => description.replaceAll(/[^a-zA-Z0-9_.-]/g, '-'); +/** + * @typedef {object} SnapshotLoader + * @property {string} snapPath + * where XS can load the snapshot from, either a filesystem path or a string + * like `@${fileDescriptorNumber}:${readableDescription}` + * @property {(destStream?: Writable) => Promise} afterSpawn + * callback for providing a destination stream to which the data should be + * piped (only relevant for a stream-based loader) + * @property {() => Promise} cleanup + * callback to free resources when the loader is no longer needed + */ + +/** + * @callback MakeSnapshotLoader + * @param {AsyncIterable} sourceBytes + * @param {string} description + * @param {{fs: Pick, ptmpName: (opts: import('tmp').TmpNameOptions) => Promise}} ioPowers + * @returns {Promise} + */ + +/** @type {MakeSnapshotLoader} */ +const makeSnapshotLoaderWithFS = async ( + sourceBytes, + description, + { fs, ptmpName }, +) => { + const snapPath = await ptmpName({ + template: `load-snapshot-${safeHintFromDescription( + description, + )}-XXXXXX.xss`, + }); + + const afterSpawn = async () => {}; + const cleanup = async () => fs.unlink(snapPath); + + try { + // eslint-disable-next-line @jessie.js/no-nested-await + const tmpSnap = await fs.open(snapPath, 'w'); + // eslint-disable-next-line @jessie.js/no-nested-await + await tmpSnap.writeFile( + // @ts-expect-error incorrect typings, does support AsyncIterable + sourceBytes, + ); + // eslint-disable-next-line @jessie.js/no-nested-await + await tmpSnap.close(); + } catch (e) { + // eslint-disable-next-line @jessie.js/no-nested-await + await cleanup(); + throw e; + } + + return harden({ + snapPath, + afterSpawn, + cleanup, + }); +}; + +/** @type {MakeSnapshotLoader} */ +const makeSnapshotLoaderWithPipe = async ( + sourceBytes, + description, + _ioPowers, +) => { + let done = Promise.resolve(); + + const cleanup = async () => done; + + /** @param {Writable} loadSnapshotsStream */ + const afterSpawn = async loadSnapshotsStream => { + assert(description); + const destStream = loadSnapshotsStream; + + const sourceStream = Readable.from(sourceBytes); + sourceStream.pipe(destStream, { end: false }); + + done = finished(sourceStream); + done.catch(noop).then(() => sourceStream.unpipe(destStream)); + }; + + return harden({ + snapPath: `@${SNAPSHOT_LOAD_FD}:${safeHintFromDescription(description)}`, + afterSpawn, + cleanup, + }); +}; + /** * @param {XSnapOptions} options * @@ -86,7 +173,7 @@ export async function xsnap(options) { netstringMaxChunkSize = undefined, parserBufferSize = undefined, snapshotStream, - snapshotDescription = snapshotStream && 'unknown', + snapshotDescription = 'unknown', snapshotUseFs = false, stdout = 'ignore', stderr = 'ignore', @@ -103,70 +190,6 @@ export async function xsnap(options) { if (platform === undefined) { throw Error(`xsnap does not support platform ${os}`); } - - /** @type {(opts: import('tmp').TmpNameOptions) => Promise} */ - const ptmpName = fs.tmpName && promisify(fs.tmpName); - - const makeLoadSnapshotHandlerWithFS = async () => { - assert(snapshotStream); - const snapPath = await ptmpName({ - template: `load-snapshot-${safeHintFromDescription( - snapshotDescription, - )}-XXXXXX.xss`, - }); - - const afterSpawn = async () => {}; - const cleanup = async () => fs.unlink(snapPath); - - try { - // eslint-disable-next-line @jessie.js/no-nested-await - const tmpSnap = await fs.open(snapPath, 'w'); - // eslint-disable-next-line @jessie.js/no-nested-await - await tmpSnap.writeFile( - // @ts-expect-error incorrect typings, does support AsyncIterable - snapshotStream, - ); - // eslint-disable-next-line @jessie.js/no-nested-await - await tmpSnap.close(); - } catch (e) { - // eslint-disable-next-line @jessie.js/no-nested-await - await cleanup(); - throw e; - } - - return harden({ - snapPath, - afterSpawn, - cleanup, - }); - }; - - const makeLoadSnapshotHandlerWithPipe = async () => { - let done = Promise.resolve(); - - const cleanup = async () => done; - - /** @param {Writable} loadSnapshotsStream */ - const afterSpawn = async loadSnapshotsStream => { - assert(snapshotStream); - const destStream = loadSnapshotsStream; - - const sourceStream = Readable.from(snapshotStream); - sourceStream.pipe(destStream, { end: false }); - - done = finished(sourceStream); - done.catch(noop).then(() => sourceStream.unpipe(destStream)); - }; - - return harden({ - snapPath: `@${SNAPSHOT_LOAD_FD}:${safeHintFromDescription( - snapshotDescription, - )}`, - afterSpawn, - cleanup, - }); - }; - let bin = new URL( `../xsnap-native/xsnap/build/bin/${platform}/${ debug ? 'debug' : 'release' @@ -179,15 +202,18 @@ export async function xsnap(options) { assert(!/^-/.test(name), `name '${name}' cannot start with hyphen`); - let loadSnapshotHandler = await (snapshotStream && - (snapshotUseFs - ? makeLoadSnapshotHandlerWithFS - : makeLoadSnapshotHandlerWithPipe)()); + /** @type {(opts: import('tmp').TmpNameOptions) => Promise} */ + const ptmpName = fs.tmpName && promisify(fs.tmpName); + const makeSnapshotLoader = snapshotUseFs + ? makeSnapshotLoaderWithFS + : makeSnapshotLoaderWithPipe; + let snapshotLoader = await (snapshotStream && + makeSnapshotLoader(snapshotStream, snapshotDescription, { fs, ptmpName })); let args = [name]; - if (loadSnapshotHandler) { - args.push('-r', loadSnapshotHandler.snapPath); + if (snapshotLoader) { + args.push('-r', snapshotLoader.snapPath); } if (meteringLimit) { @@ -258,13 +284,13 @@ export async function xsnap(options) { const snapshotSaveStream = xsnapProcessStdio[SNAPSHOT_SAVE_FD]; const snapshotLoadStream = xsnapProcessStdio[SNAPSHOT_LOAD_FD]; - await loadSnapshotHandler?.afterSpawn(snapshotLoadStream); + await snapshotLoader?.afterSpawn(snapshotLoadStream); - if (loadSnapshotHandler) { + if (snapshotLoader) { vatExit.promise.catch(noop).then(() => { - if (loadSnapshotHandler) { - const { cleanup } = loadSnapshotHandler; - loadSnapshotHandler = undefined; + if (snapshotLoader) { + const { cleanup } = snapshotLoader; + snapshotLoader = undefined; return cleanup(); } }); @@ -286,9 +312,9 @@ export async function xsnap(options) { async function runToIdle() { for await (const _ of forever) { const iteration = await messagesFromXsnap.next(undefined); - if (loadSnapshotHandler) { - const { cleanup } = loadSnapshotHandler; - loadSnapshotHandler = undefined; + if (snapshotLoader) { + const { cleanup } = snapshotLoader; + snapshotLoader = undefined; // eslint-disable-next-line @jessie.js/no-nested-await await cleanup(); } diff --git a/packages/xsnap/test/leakiness.mjs b/packages/xsnap/test/leakiness.mjs new file mode 100644 index 00000000000..96534df237b --- /dev/null +++ b/packages/xsnap/test/leakiness.mjs @@ -0,0 +1,183 @@ +/* global setTimeout, process */ + +// This file functions as both an ava-importable module and a standalone script. +// See below for usage detail about the latter. +import 'ses'; +import '@endo/eventual-send/shim.js'; +import 'data:text/javascript,try { lockdown(); } catch (_err) {}'; + +import * as proc from 'child_process'; +import * as os from 'os'; +import fs from 'fs'; +import { tmpName } from 'tmp'; +import { parseArgs } from 'util'; +import { isMainThread } from 'worker_threads'; + +import { Nat } from '@endo/nat'; +import { makePromiseKit } from '@endo/promise-kit'; + +import { xsnap } from '../src/xsnap.js'; + +import { options as makeXSnapOptions } from './message-tools.js'; + +const io = { spawn: proc.spawn, os: os.type(), fs, tmpName }; // WARNING: ambient + +/** @import {XSnapOptions} from '@agoric/xsnap/src/xsnap.js' */ + +/** + * Creates an Xsnap instance that responds to each inbound command by + * interpreting it as a decimal count of bytes to retain. + * + * @param {Partial} [xsnapOptions] + * @returns {Promise>>} + */ +export const makeRetentiveVat = async xsnapOptions => { + const vat = await xsnap({ ...makeXSnapOptions(io), ...xsnapOptions }); + // Retain data for each message in a new ArrayBuffer, populating each such + // buffer with globally descending binary64 values (as buffer space permits) + // to limit compressibility. + await vat.evaluate(` + const decoder = new TextDecoder(); + const buffers = []; + let x = Number.MAX_SAFE_INTEGER; + function handleCommand(message) { + const n = +decoder.decode(message); + const view = new DataView(new ArrayBuffer(n)); + for (let offset = 0; offset + 8 <= n; offset += 8) { + view.setFloat64(offset, x); + x -= 1; + } + buffers.push(view.buffer); + } + `); + return vat; +}; +harden(makeRetentiveVat); + +/** + * Spawns a heap-preserving sequence of Xsnap instances, each retaining + * approximately the same amount of additional data. + * + * @param {object} options + * @param {(newVat: Awaited>, loadedSnapshotStream: AsyncIterable | undefined) => Promise} [options.afterCommand] + * a callback to run after a vat handles its command, for e.g. interrogation and/or + * inserting delays between instances + * @param {number} options.chunkCount the number of instances to spawn + * @param {number} options.chunkSize the count of bytes to retain per instance + * @param {Partial} [options.xsnapOptions] + * @returns {Promise} + */ +export const spawnRetentiveVatSequence = async ({ + afterCommand, + chunkCount, + chunkSize, + xsnapOptions, +}) => { + await null; + /** @type {Awaited> | undefined} */ + let vat = undefined; + try { + for (let i = 0; i < chunkCount; i += 1) { + // Make a new vat, replacing a previous vat if present. + const oldVat = vat; + const snapshotStream = oldVat?.makeSnapshotStream(); + vat = await makeRetentiveVat({ + ...xsnapOptions, + snapshotStream, + }); + await oldVat?.close(); + + // Issue the command. + const label = `command number ${i}`; + try { + await vat.issueStringCommand(`${chunkSize}`); + } catch (err) { + throw Error(`Error with ${label}`, { cause: err }); + } + await afterCommand?.(vat, snapshotStream); + } + } finally { + await vat?.close(); + } +}; +harden(spawnRetentiveVatSequence); + +/** + * Spawns a heap-preserving sequence of Xsnap instances with intervening delays. + * + * @param {object} options + * @param {number} options.chunkCount + * @param {number} options.chunkSize + * @param {number} options.idleDuration + * @param {Partial} [options.xsnapOptions] + * @returns {Promise} + */ +const main = async ({ chunkCount, chunkSize, idleDuration, xsnapOptions }) => { + const afterCommand = async (_vat, _snapshotStream) => { + const { promise, resolve } = makePromiseKit(); + setTimeout(resolve, idleDuration); + return promise; + }; + return spawnRetentiveVatSequence({ + afterCommand, + chunkCount, + chunkSize, + xsnapOptions, + }); +}; + +const isEntryPoint = + !/[/]node_modules[/]/.test(process.argv[1]) && + // cf. https://github.com/avajs/ava/blob/f8bf00cd988b5e981b6c7d87523a1e0c5dc947c0/lib/worker/guard-environment.cjs + typeof process.send !== 'function' && + isMainThread !== false; +if (isEntryPoint) { + /** @typedef {{type: 'string' | 'boolean', short?: string, multiple?: boolean, default?: string | boolean | string[] | boolean[]}} ParseArgsOptionConfig */ + /** @type {Record} */ + const cliOptions = { + help: { type: 'boolean' }, + chunkCount: { + type: 'string', + default: '10', + }, + chunkSize: { + type: 'string', + default: '10000', + }, + idleDuration: { + type: 'string', + default: '10000', + }, + xsnapOptions: { + type: 'string', + default: '{ "snapshotUseFs": false }', + }, + }; + const { values: config } = parseArgs({ options: cliOptions }); + let chunkCount, chunkSize, idleDuration, xsnapOptions; + try { + if (config.help) throw Error(); + const parseNat = str => Nat(/[0-9]/.test(str || '') ? Number(str) : NaN); + chunkCount = Number(parseNat(config.chunkCount)); + chunkSize = Number(parseNat(config.chunkSize)); + idleDuration = Number(parseNat(config.idleDuration)); + xsnapOptions = JSON.parse(/** @type {string} */ (config.xsnapOptions)); + } catch (err) { + const log = config.help ? console.log : console.error; + if (!config.help) log(err.message); + log(`Usage: node [--inspect-brk] $script \\ + [--chunkCount N (default ${cliOptions.chunkCount.default})] \\ + [--chunkSize BYTE_COUNT (default ${cliOptions.chunkSize.default})] \\ + [--idleDuration MILLISECONDS (default ${cliOptions.idleDuration.default})] \\ + [--xsnapOptions JSON (default ${cliOptions.xsnapOptions.default})] + +Spawns a heap-preserving sequence of $chunkCount xsnap instances, each retaining +approximately $chunkSize additional bytes, waiting $idleDuration milliseconds +before spawning each instance's successor.`); + process.exit(64); + } + main({ chunkCount, chunkSize, idleDuration, xsnapOptions }).catch(err => { + console.error(err); + process.exitCode ||= 1; + }); +} diff --git a/packages/xsnap/test/leakiness.test.js b/packages/xsnap/test/leakiness.test.js new file mode 100644 index 00000000000..903ecc3cdf4 --- /dev/null +++ b/packages/xsnap/test/leakiness.test.js @@ -0,0 +1,90 @@ +/** global FinalizationRegistry */ + +import test from 'ava'; + +import process from 'node:process'; +import v8 from 'node:v8'; + +import engineGC from '@agoric/internal/src/lib-nodejs/engine-gc.js'; +import { waitUntilQuiescent } from '@agoric/internal/src/lib-nodejs/waitUntilQuiescent.js'; + +import { spawnRetentiveVatSequence } from './leakiness.mjs'; + +/** @import {XSnapOptions} from '@agoric/xsnap/src/xsnap.js' */ + +/** + * @param {import('ava').ExecutionContext} t + * @param {Partial} xsnapOptions + */ +const testRetention = async (t, xsnapOptions) => { + let snapshotsCreated = 0; + let snapshotsFreed = 0; + let lastVat = null; + /** @type {FinalizationRegistry} */ + const fr = new FinalizationRegistry(() => { + snapshotsFreed += 1; + }); + + t.plan(4); + + await waitUntilQuiescent(); + engineGC(); + if (process.env.TAKE_HEAP_SNAPSHOT) { + v8.writeHeapSnapshot( + `Heap-${process.pid}-${t.title}-${Date.now()}-before.heapsnapshot`, + ); + } + + await spawnRetentiveVatSequence({ + afterCommand: async (vat, snapshotStream) => { + lastVat = vat; + if (snapshotStream) { + snapshotsCreated += 1; + fr.register(snapshotStream); + } + }, + chunkCount: 10, + chunkSize: 1000, + xsnapOptions, + }); + + t.truthy(lastVat); + t.true(snapshotsCreated >= 1); + + await waitUntilQuiescent(); + engineGC(); + await waitUntilQuiescent(); + if (process.env.TAKE_HEAP_SNAPSHOT) { + v8.writeHeapSnapshot( + `Heap-${process.pid}-${t.title}-${Date.now()}-after-tail.heapsnapshot`, + ); + } + + const snapshotsFreedWithLastVatAlive = snapshotsFreed; + + lastVat = null; + + await waitUntilQuiescent(); + engineGC(); + await waitUntilQuiescent(); + if (process.env.TAKE_HEAP_SNAPSHOT) { + v8.writeHeapSnapshot( + `Heap-${process.pid}-${t.title}-${Date.now()}-after-all.heapsnapshot`, + ); + } + + t.is(snapshotsFreed, snapshotsCreated, 'all snapshots freed'); + t.is( + snapshotsFreedWithLastVatAlive, + snapshotsFreed, + 'snapshots not tail retained', + ); +}; + +test.serial('snapshot GC with pipes', testRetention, { + snapshotUseFs: false, +}); + +test.serial('snapshot GC with temp files', testRetention, { + snapshotUseFs: true, +});