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 })