From dbc74059503b629202823b1bc0010f0d5624dff6 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 23 Mar 2026 16:26:13 -0700 Subject: [PATCH 1/2] esm: fix source phase identity bug in loadCache eviction PR-URL: https://github.com/nodejs/node/pull/62415 Reviewed-By: Yagiz Nizipli Reviewed-By: Benjamin Gruenbaum --- lib/internal/modules/esm/module_job.js | 15 +++++++++++---- .../test-esm-wasm-source-phase-identity.mjs | 11 +++++++++++ .../test-wasm-source-phase-identity-parent.mjs | 6 ++++++ .../test-wasm-source-phase-identity.mjs | 14 ++++++++++++++ 4 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 test/es-module/test-esm-wasm-source-phase-identity.mjs create mode 100644 test/fixtures/es-modules/test-wasm-source-phase-identity-parent.mjs create mode 100644 test/fixtures/es-modules/test-wasm-source-phase-identity.mjs diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 929577c0da6d08..22032f79e90d44 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -145,7 +145,9 @@ class ModuleJobBase { */ syncLink(requestType) { // Store itself into the cache first before linking in case there are circular - // references in the linking. + // references in the linking. Track whether we're overwriting an existing entry + // so we know whether to remove the temporary entry in the finally block. + const hadPreviousEntry = this.loader.loadCache.get(this.url, this.type) !== undefined; this.loader.loadCache.set(this.url, this.type, this); const moduleRequests = this.module.getModuleRequests(); // Modules should be aligned with the moduleRequests array in order. @@ -169,9 +171,14 @@ class ModuleJobBase { } this.module.link(modules); } finally { - // Restore it - if it succeeds, we'll reset in the caller; Otherwise it's - // not cached and if the error is caught, subsequent attempt would still fail. - this.loader.loadCache.delete(this.url, this.type); + if (!hadPreviousEntry) { + // Remove the temporary entry. On failure this ensures subsequent attempts + // don't return a broken job. On success the caller + // (#getOrCreateModuleJobAfterResolve) will re-insert under the correct key. + this.loader.loadCache.delete(this.url, this.type); + } + // If there was a previous entry (ensurePhase() path), leave this in cache - + // it is the upgraded job and the caller will not re-insert. } return evaluationDepJobs; diff --git a/test/es-module/test-esm-wasm-source-phase-identity.mjs b/test/es-module/test-esm-wasm-source-phase-identity.mjs new file mode 100644 index 00000000000000..2d742dfcff61ab --- /dev/null +++ b/test/es-module/test-esm-wasm-source-phase-identity.mjs @@ -0,0 +1,11 @@ +// Regression test for source phase import identity with mixed eval/source +// phase imports of the same module in one parent. +import '../common/index.mjs'; +import { spawnSyncAndAssert } from '../common/child_process.js'; +import * as fixtures from '../common/fixtures.mjs'; + +spawnSyncAndAssert( + process.execPath, + ['--no-warnings', fixtures.path('es-modules/test-wasm-source-phase-identity.mjs')], + { stdout: '', stderr: '', trim: true } +); diff --git a/test/fixtures/es-modules/test-wasm-source-phase-identity-parent.mjs b/test/fixtures/es-modules/test-wasm-source-phase-identity-parent.mjs new file mode 100644 index 00000000000000..36d5765c17f0ad --- /dev/null +++ b/test/fixtures/es-modules/test-wasm-source-phase-identity-parent.mjs @@ -0,0 +1,6 @@ +import * as mod1 from './simple.wasm'; +import * as mod2 from './simple.wasm'; +import source mod3 from './simple.wasm'; +import source mod4 from './simple.wasm'; + +export { mod1, mod2, mod3, mod4 }; diff --git a/test/fixtures/es-modules/test-wasm-source-phase-identity.mjs b/test/fixtures/es-modules/test-wasm-source-phase-identity.mjs new file mode 100644 index 00000000000000..84cf6261139038 --- /dev/null +++ b/test/fixtures/es-modules/test-wasm-source-phase-identity.mjs @@ -0,0 +1,14 @@ +import { strictEqual } from 'node:assert'; + +// Pre-load simple.wasm at kSourcePhase to prime the loadCache. +const preloaded = await import.source('./simple.wasm'); +strictEqual(preloaded instanceof WebAssembly.Module, true); + +// Import a parent that has both eval-phase and source-phase imports of the +// same wasm file, which triggers ensurePhase(kEvaluationPhase) on the cached +// job and exposes the loadCache eviction bug. +const { mod1, mod2, mod3, mod4 } = + await import('./test-wasm-source-phase-identity-parent.mjs'); + +strictEqual(mod1, mod2, 'two eval-phase imports of the same wasm must be identical'); +strictEqual(mod3, mod4, 'two source-phase imports of the same wasm must be identical'); From 53bcd114b10021c4a883b08df4d3c2ff6946b430 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 26 Mar 2026 23:18:32 +0100 Subject: [PATCH 2/2] zlib: fix use-after-free when reset() is called during write The Reset() method did not check the write_in_progress_ flag before resetting the compression stream. This allowed reset() to free the compression library's internal state while a worker thread was still using it during an async write, causing a use-after-free. Add a write_in_progress_ guard to Reset() that throws an error if a write is in progress, matching the existing pattern used by Close() and Write(). PR-URL: TODO Refs: https://hackerone.com/reports/3609132 PR-URL: https://github.com/nodejs/node/pull/62325 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Yagiz Nizipli Reviewed-By: Luigi Pinca --- src/node_zlib.cc | 6 +++++ test/parallel/test-zlib-reset-during-write.js | 23 +++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 test/parallel/test-zlib-reset-during-write.js diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 9d49f13d07c125..792d800847e105 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -644,6 +644,12 @@ class CompressionStream : public AsyncWrap, CompressionStream* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.This()); + if (wrap->write_in_progress_) { + wrap->env()->ThrowError( + "Cannot reset zlib stream while a write is in progress"); + return; + } + AllocScope alloc_scope(wrap); const CompressionError err = wrap->context()->ResetStream(); if (err.IsError()) diff --git a/test/parallel/test-zlib-reset-during-write.js b/test/parallel/test-zlib-reset-during-write.js new file mode 100644 index 00000000000000..35c4c853e5ea59 --- /dev/null +++ b/test/parallel/test-zlib-reset-during-write.js @@ -0,0 +1,23 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const { createBrotliCompress, createDeflate } = require('zlib'); + +// Tests that calling .reset() while an async write is in progress +// throws an error instead of causing a use-after-free. + +for (const factory of [createBrotliCompress, createDeflate]) { + const stream = factory(); + const input = Buffer.alloc(1024, 0x41); + + stream.write(input, common.mustCall()); + stream.on('error', common.mustNotCall()); + + // The write has been dispatched to the thread pool. + // Calling reset while write is in progress must throw. + assert.throws(() => { + stream._handle.reset(); + }, { + message: 'Cannot reset zlib stream while a write is in progress', + }); +}