From b0caa00bedafee681f1c81846aecf7d7e79af88a Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 23:36:23 -0700 Subject: [PATCH] fix(windows): stop shipping an unpatched conpty prebuild a cross-arch package can load Two halves of one hole. node-pty's loader falls through build/Release -> build/Debug -> prebuilds/-, and the published prebuild has never carried the MSYS breakaway denial. The prune only removed that prebuild when `electronArch === process.arch`. That proxy stood in for "build/Release holds an addon the target can actually load", but it is false for an arm64 slice cross-built on an x64 Windows host -- a rebuild that DOES produce a correct arm64 addon. That slice shipped the unpatched prebuild as a live fallback. Read the PE `Machine` field instead of guessing (`conptyTargetsArch`, shaped after readElfMachine() in verify-linux-glibc-floor.cjs). Deleting it unconditionally was the other option and is wrong: in the true cross-host case (packaging Windows from macOS) build/Release is not a Windows binary at all, and removing the prebuild would leave the package with no loadable ConPTY. So the verifier closes the remainder. It treated "no build output" as "no addon at all" and warned -- which is exactly the package whose sole loadable addon is the unpatched prebuild. It now checks the prebuild the loader would fall through to, and a present-but-unmarked binary there is fatal. Absence still warns only when nothing else would load. Together there is no hole: either the prebuild is pruned, or it is the sole loadable addon and the verifier fails the release. Latent today -- the patched build output wins the load order. The trigger is someone packaging cross-arch. Verified by mutation, not assumed: restoring the old `electronArch === process.arch` prune fails 2 rows in packaged-node-pty-prebuild-prune, and disabling the verifier's prebuild fallback fails 2 rows in verify-packaged-node-pty-job-ownership. --- config/electron-builder.config.cjs | 6 +- config/packaged-runtime-node-modules.cjs | 25 +++++---- config/scripts/node-pty-job-ownership.cjs | 35 ++++++++++++ .../packaged-node-pty-prebuild-prune.test.mjs | 53 ++++++++++++++---- ...verify-packaged-node-pty-job-ownership.cjs | 34 +++++++++-- ...y-packaged-node-pty-job-ownership.test.mjs | 56 +++++++++++++++++++ 6 files changed, 183 insertions(+), 26 deletions(-) diff --git a/config/electron-builder.config.cjs b/config/electron-builder.config.cjs index e0a7c126e08..b3ff31381db 100644 --- a/config/electron-builder.config.cjs +++ b/config/electron-builder.config.cjs @@ -337,6 +337,7 @@ module.exports = { // Node cannot load cross-arch. `Arch` enum: ia32=0, x64=1, armv7l=2, // arm64=3, universal=4 (universal contains the host slice, so run it). const archEnumByNodeArch = { ia32: 0, x64: 1, armv7l: 2, arm64: 3 } + const nodeArchByArchEnum = { 0: 'ia32', 1: 'x64', 2: 'armv7l', 3: 'arm64' } const hostArchEnum = archEnumByNodeArch[process.arch] const canExecuteTargetArch = context.arch === hostArchEnum || context.arch === 4 if (context.electronPlatformName === 'win32') { @@ -347,7 +348,10 @@ module.exports = { // MSYS breakaway marker is a file read, and skipping it is how a // cross-host Windows release could ship the orphan bug. console.log('[verify-packaged-node-pty] skipped cross-platform or cross-arch package') - verifyPackagedConptyBreakawayMarker(resourcesDir) + // The arch names the prebuild directory the loader would fall through to. + verifyPackagedConptyBreakawayMarker(resourcesDir, { + arch: nodeArchByArchEnum[context.arch] + }) } } verifySkillsCliRuntime(join(resourcesDir, 'app.asar.unpacked', 'out'), resourcesDir, { diff --git a/config/packaged-runtime-node-modules.cjs b/config/packaged-runtime-node-modules.cjs index 1ee443f8288..385400d9996 100644 --- a/config/packaged-runtime-node-modules.cjs +++ b/config/packaged-runtime-node-modules.cjs @@ -9,6 +9,7 @@ const { } = require('node:fs') const { dirname, join, resolve } = require('node:path') const { builtinModules, createRequire } = require('node:module') +const { conptyTargetsArch } = require('./scripts/node-pty-job-ownership.cjs') const projectDir = resolve(__dirname, '..') const requireFromProject = createRequire(join(projectDir, 'package.json')) @@ -388,17 +389,19 @@ function prunePackagedNodePty(resourcesDir, electronPlatformName, electronArch) // require, and its caller resolves null with silent: true), and removes the // winpty backend that node-pty still selects below Windows build 18309. // - // Why the arch check: a cross-arch package copies the host's build/Release, - // so its mere presence does not mean it matches electronArch -- deleting the - // target-arch prebuild would then remove the only loadable binary. - if ( - electronPlatformName === 'win32' && - electronArch === process.arch && - existsSync(join(nodePtyDir, 'build', 'Release', 'conpty.node')) - ) { - const prebuildDir = join(nodePtyDir, 'prebuilds', `win32-${electronArch}`) - for (const staleFallback of ['conpty.node', 'conpty.pdb']) { - rmSync(join(prebuildDir, staleFallback), { force: true }) + // Why the arch check: a cross-HOST package can copy a build/Release that is not a Windows + // binary at all, so its mere presence does not mean it matches electronArch -- deleting the + // target-arch prebuild would then remove the only loadable binary. This used to approximate + // that with `electronArch === process.arch`, which also skipped the arm64 slice cross-built on + // an x64 Windows host -- a rebuild that DOES produce a correct arm64 addon. That slice then + // shipped the unpatched prebuild as a live fallback. Read the header instead of guessing. + if (electronPlatformName === 'win32') { + const releaseAddon = join(nodePtyDir, 'build', 'Release', 'conpty.node') + if (conptyTargetsArch(releaseAddon, electronArch)) { + const prebuildDir = join(nodePtyDir, 'prebuilds', `win32-${electronArch}`) + for (const staleFallback of ['conpty.node', 'conpty.pdb']) { + rmSync(join(prebuildDir, staleFallback), { force: true }) + } } } diff --git a/config/scripts/node-pty-job-ownership.cjs b/config/scripts/node-pty-job-ownership.cjs index 2d908b6082f..dbbfaddfee3 100644 --- a/config/scripts/node-pty-job-ownership.cjs +++ b/config/scripts/node-pty-job-ownership.cjs @@ -24,6 +24,40 @@ const NODE_PTY_JOB_EXPORTS = ['listJobProcessIds', 'terminateJob', 'assignCurren */ const CYGWIN_BREAKAWAY_MARKER = Buffer.from('msys-2.0.dll', 'utf16le') +/** PE `Machine`, for the Windows slices we package. Names match electron-builder's Arch enum. */ +const PE_MACHINE_BY_ARCH = Object.freeze({ ia32: 0x14c, x64: 0x8664, arm64: 0xaa64 }) + +/** + * Whether a `.node` is a PE built for `arch`, or null when the file cannot be read as one. + * + * Why the packaged prune needs this: it used to approximate "build/Release holds the target's + * addon" with `electronArch === process.arch`, which is false for an arm64 slice cross-built on + * x64 even though that rebuild does produce a correct arm64 addon. Reading the header answers the + * question the proxy was standing in for. Shape mirrors readElfMachine() in + * verify-linux-glibc-floor.cjs. + */ +function conptyTargetsArch(addonPath, arch) { + const expected = PE_MACHINE_BY_ARCH[arch] + if (expected === undefined) { + return null + } + let binary + try { + binary = readFileSync(addonPath) + } catch { + return null + } + // PE header offset lives at 0x3c; `Machine` is the first field after the 4-byte signature. + if (binary.length < 0x40) { + return null + } + const peHeaderOffset = binary.readUInt32LE(0x3c) + if (peHeaderOffset + 6 > binary.length) { + return null + } + return binary.readUInt16LE(peHeaderOffset + 4) === expected +} + /** * Absolute path of the addon `loadNativeModule` just resolved. * @@ -95,5 +129,6 @@ function assertCygwinBreakawayDenied(addonPath, native) { module.exports = { assertNodePtyJobOwnership, assertCygwinBreakawayDenied, + conptyTargetsArch, nodePtyAddonPath } diff --git a/config/scripts/packaged-node-pty-prebuild-prune.test.mjs b/config/scripts/packaged-node-pty-prebuild-prune.test.mjs index c0b24b8a8a2..f1542611bd9 100644 --- a/config/scripts/packaged-node-pty-prebuild-prune.test.mjs +++ b/config/scripts/packaged-node-pty-prebuild-prune.test.mjs @@ -22,18 +22,30 @@ describe('prunePackagedNodePty: the Windows conpty fallback', () => { const HOST_ARCH = process.arch const nodePty = () => join(resources, 'node_modules', 'node-pty') - const write = (relative) => { + const write = (relative, contents = 'x') => { const target = join(nodePty(), relative) mkdirSync(join(target, '..'), { recursive: true }) - writeFileSync(target, 'x') + writeFileSync(target, contents) + } + + /** Minimal PE far enough to carry a `Machine`, which is what the prune now reads. */ + const PE_MACHINE_BY_ARCH = { x64: 0x8664, arm64: 0xaa64 } + const peAddon = (arch) => { + const buffer = Buffer.alloc(0x50) + buffer.write('MZ') + buffer.writeUInt32LE(0x40, 0x3c) + buffer.write('PE\0\0', 0x40) + buffer.writeUInt16LE(PE_MACHINE_BY_ARCH[arch], 0x44) + return buffer } const prebuilt = (name, arch = HOST_ARCH) => join(nodePty(), 'prebuilds', `win32-${arch}`, name) /** What a real packaged win32 prebuilds/ directory holds. */ const SIBLINGS = ['conpty_console_list.node', 'pty.node', 'winpty.dll', 'winpty-agent.exe'] - const seedWindowsTree = (arch = HOST_ARCH) => { - write(join('build', 'Release', 'conpty.node')) + /** `buildArch` is what build/Release actually holds, which is not always the slice's arch. */ + const seedWindowsTree = (arch = HOST_ARCH, buildArch = arch) => { + write(join('build', 'Release', 'conpty.node'), peAddon(buildArch)) write(join('third_party', 'conpty', 'v1', `win10-${arch}`, 'conpty.dll')) write(join('third_party', 'conpty', 'v1', `win10-${arch}`, 'OpenConsole.exe')) write(join('prebuilds', `win32-${arch}`, 'conpty.node')) @@ -79,23 +91,44 @@ describe('prunePackagedNodePty: the Windows conpty fallback', () => { expect(existsSync(prebuilt('conpty.node'))).toBe(true) }) - it('keeps the fallback on a cross-arch package, where build/Release is the host arch', () => { - // electron-builder --win --arm64 on an x64 host copies an x64 - // build/Release; deleting the arm64 prebuild would remove the only binary - // the shipped app could load. + it('keeps the fallback when build/Release holds a different arch than the slice', () => { + // A cross-HOST package can copy a build/Release that the target cannot load; + // deleting the prebuild would remove the only binary the shipped app has. const target = HOST_ARCH === 'arm64' ? 'x64' : 'arm64' - seedWindowsTree(target) + seedWindowsTree(target, HOST_ARCH) prunePackagedNodePty(resources, 'win32', target) expect(existsSync(prebuilt('conpty.node', target))).toBe(true) }) + it('removes the fallback on a cross-arch package whose build/Release IS the slice arch', () => { + // Measured on Windows: `electron-builder --win --arm64` on an x64 host does + // cross-rebuild a correct arm64 addon. Keying off the host arch skipped this + // case, so the slice shipped the unpatched prebuild as a live fallback. + const target = HOST_ARCH === 'arm64' ? 'x64' : 'arm64' + seedWindowsTree(target, target) + + prunePackagedNodePty(resources, 'win32', target) + + expect(existsSync(prebuilt('conpty.node', target))).toBe(false) + expect(existsSync(prebuilt('pty.node', target))).toBe(true) + }) + + it('keeps the fallback when build/Release is not a readable PE at all', () => { + seedWindowsTree() + write(join('build', 'Release', 'conpty.node'), 'not-a-pe') + + prunePackagedNodePty(resources, 'win32', HOST_ARCH) + + expect(existsSync(prebuilt('conpty.node'))).toBe(true) + }) + it.each([ ['darwin', 'arm64'], ['linux', 'x64'] ])('leaves %s prebuilds alone', (platform, arch) => { - write(join('build', 'Release', 'conpty.node')) + write(join('build', 'Release', 'conpty.node'), peAddon('x64')) write(join('prebuilds', `${platform}-${arch}`, 'pty.node')) prunePackagedNodePty(resources, platform, arch) diff --git a/config/scripts/verify-packaged-node-pty-job-ownership.cjs b/config/scripts/verify-packaged-node-pty-job-ownership.cjs index ad347f79f08..1584d2eb1be 100644 --- a/config/scripts/verify-packaged-node-pty-job-ownership.cjs +++ b/config/scripts/verify-packaged-node-pty-job-ownership.cjs @@ -12,6 +12,13 @@ function packagedConptyPath(resourcesDir) { return join(resourcesDir, 'node_modules', 'node-pty', 'build', 'Release', 'conpty.node') } +/** The last entry in node-pty's loader order, and the one the published tarball fills. */ +function prebuiltConptyPath(resourcesDir, arch) { + return arch + ? join(resourcesDir, 'node_modules', 'node-pty', 'prebuilds', `win32-${arch}`, 'conpty.node') + : null +} + function loadPackagedConpty(resourcesDir) { const packagedRequire = createRequire(join(resourcesDir, 'package.json')) const utilsPath = packagedRequire.resolve('./node_modules/node-pty/lib/utils') @@ -42,16 +49,35 @@ function verifyPackagedNodePtyJobOwnership(resourcesDir, options = {}) { * release built elsewhere could ship a node-pty that leaks every MSYS pane * child out of its job. Reading the binary needs neither. * - * Absence is logged rather than thrown: an unrecognised layout must not fail a - * release that was packaging fine, and the export check still covers the - * same-host case. A binary that IS there and lacks the marker is fatal. + * Absence is logged rather than thrown ONLY when nothing else would load: an + * unrecognised layout must not fail a release that was packaging fine, and the + * export check still covers the same-host case. A binary that IS there and + * lacks the marker is fatal. + * + * Why the prebuild is checked too: node-pty's loader falls through + * build/Release -> build/Debug -> prebuilds/-, so a package + * with no build/Release addon loads the prebuild -- and the published prebuild + * has never carried the breakaway denial. Warning there would pass exactly the + * package that ships the bug. */ function verifyPackagedConptyBreakawayMarker(resourcesDir, options = {}) { // Deliberately no host-platform gate: the caller has already established that // the *target* is Windows, and gating on the host is the very skip this // closes. + const exists = options.exists ?? existsSync const addonPath = (options.packagedConptyPath ?? packagedConptyPath)(resourcesDir) - if (!(options.exists ?? existsSync)(addonPath)) { + if (!exists(addonPath)) { + const fallbackPath = (options.prebuiltConptyPath ?? prebuiltConptyPath)( + resourcesDir, + options.arch + ) + if (fallbackPath && exists(fallbackPath)) { + assertCygwinBreakawayDenied(fallbackPath, { dir: fallbackPath }) + console.log( + '[verify-packaged-node-pty] OK — packaged ConPTY denies MSYS job breakaway (prebuild fallback)' + ) + return + } console.warn( `[verify-packaged-node-pty] no addon at ${addonPath}; could not check the MSYS ` + 'job-breakaway denial for this cross-host package.' diff --git a/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs b/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs index 08eaccb9852..cac58ce97ce 100644 --- a/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs +++ b/config/scripts/verify-packaged-node-pty-job-ownership.test.mjs @@ -109,6 +109,62 @@ describe('verifyPackagedConptyBreakawayMarker', () => { warn.mockRestore() }) + // node-pty falls through build/Release -> build/Debug -> prebuilds/-, so a + // package with no build/Release addon loads the prebuild. Warning there passes exactly the + // package that ships the bug. + it('fails when the only loadable addon is the unpatched prebuild', () => { + expect(() => + verifyPackagedConptyBreakawayMarker('resources', { + arch: 'arm64', + packagedConptyPath: () => join(fixtureDir, 'absent.node'), + prebuiltConptyPath: () => PRE_MSYS_ADDON + }) + ).toThrow(/predates the Cygwin\/MSYS job-breakaway denial/) + }) + + it('accepts a package whose only addon is a patched prebuild', () => { + expect(() => + verifyPackagedConptyBreakawayMarker('resources', { + arch: 'arm64', + packagedConptyPath: () => join(fixtureDir, 'absent.node'), + prebuiltConptyPath: () => CURRENT_ADDON + }) + ).not.toThrow() + }) + + it('still warns when neither the build output nor a prebuild is there', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + expect(() => + verifyPackagedConptyBreakawayMarker('resources', { + arch: 'arm64', + packagedConptyPath: () => join(fixtureDir, 'absent.node'), + prebuiltConptyPath: () => join(fixtureDir, 'also-absent.node') + }) + ).not.toThrow() + expect(warn).toHaveBeenCalledWith(expect.stringContaining('could not check the MSYS')) + warn.mockRestore() + }) + + it('looks where the loader would find the prebuild fallback', () => { + const exists = vi.fn().mockReturnValue(false) + verifyPackagedConptyBreakawayMarker(join('out', 'win-arm64-unpacked', 'resources'), { + arch: 'arm64', + exists + }) + expect(exists).toHaveBeenCalledWith( + join( + 'out', + 'win-arm64-unpacked', + 'resources', + 'node_modules', + 'node-pty', + 'prebuilds', + 'win32-arm64', + 'conpty.node' + ) + ) + }) + it('looks where electron-builder actually lands the addon', () => { const exists = vi.fn().mockReturnValue(false) verifyPackagedConptyBreakawayMarker(join('out', 'win-unpacked', 'resources'), { exists })