From 91f843bf38b0e4ec8576f41f3140488b4ee8ff32 Mon Sep 17 00:00:00 2001 From: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com> Date: Sun, 11 Oct 2026 06:26:30 -0700 Subject: [PATCH 1/3] fix(artifacts): mark Windows host executables by extension (#1823) Windows has no POSIX execute bit; Node reports file modes from the read-only attribute alone, so every host file, including PE images, reported `executable: false` while the same occurrence reported `artifact_kind: executable`. Derive the host executable marking once: POSIX permission bits stay authoritative, and Windows marks regular files with the CreateProcess/cmd launchable extensions. Fixes #1819 Co-authored-by: Claude Opus 5.5 --- src/artifacts/DirectoryArtifactReader.ts | 3 +- src/artifacts/hostExecutableMarking.test.ts | 36 +++++++++++++++++++++ src/artifacts/hostExecutableMarking.ts | 22 +++++++++++++ src/artifacts/inventory/scanCanonical.ts | 3 +- 4 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 src/artifacts/hostExecutableMarking.test.ts create mode 100644 src/artifacts/hostExecutableMarking.ts diff --git a/src/artifacts/DirectoryArtifactReader.ts b/src/artifacts/DirectoryArtifactReader.ts index 1e70c0e0e4..cbbbbba69e 100644 --- a/src/artifacts/DirectoryArtifactReader.ts +++ b/src/artifacts/DirectoryArtifactReader.ts @@ -14,6 +14,7 @@ import { type ArtifactEntry, type ArtifactReader, } from "./ArtifactReader.js"; +import { hostExecutableMarking } from "./hostExecutableMarking.js"; import { OwnedDirectoryHandle } from "../filesystem/OwnedDirectoryHandle.js"; import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; import { readFileHandleChunks } from "../filesystem/readFileHandleChunks.js"; @@ -270,7 +271,7 @@ const artifactEntry = ( kind, declaredSize: kind === "file" ? metadata.size : null, compressedSize: null, - executable: (metadata.mode & 0o111) !== 0, + executable: hostExecutableMarking(absolute, metadata), encrypted: false, byteOffset: null, declaredSha256: null, diff --git a/src/artifacts/hostExecutableMarking.test.ts b/src/artifacts/hostExecutableMarking.test.ts new file mode 100644 index 0000000000..7aff5bbdac --- /dev/null +++ b/src/artifacts/hostExecutableMarking.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from "vitest"; + +import { hostExecutableMarking } from "./hostExecutableMarking.js"; + +const file = (mode: number) => ({ mode, isFile: () => true }); +const directory = (mode: number) => ({ mode, isFile: () => false }); + +describe("hostExecutableMarking", () => { + it("marks Windows launchable files by extension because NTFS has no execute bit", () => { + expect( + hostExecutableMarking("C:\\scratch\\whoami.exe", file(0o666), "win32"), + ).toBe(true); + expect( + hostExecutableMarking("C:\\scratch\\SETUP.CMD", file(0o444), "win32"), + ).toBe(true); + expect( + hostExecutableMarking("C:\\scratch\\library.dll", file(0o666), "win32"), + ).toBe(false); + expect( + hostExecutableMarking( + "C:\\scratch\\tools.exe", + directory(0o666), + "win32", + ), + ).toBe(false); + }); + + it("keeps POSIX permission bits authoritative regardless of extension", () => { + expect(hostExecutableMarking("/scratch/tool", file(0o755), "linux")).toBe( + true, + ); + expect( + hostExecutableMarking("/scratch/whoami.exe", file(0o644), "darwin"), + ).toBe(false); + }); +}); diff --git a/src/artifacts/hostExecutableMarking.ts b/src/artifacts/hostExecutableMarking.ts new file mode 100644 index 0000000000..dcc4b4fb24 --- /dev/null +++ b/src/artifacts/hostExecutableMarking.ts @@ -0,0 +1,22 @@ +import type { Stats } from "node:fs"; +import { extname } from "node:path"; + +/** Extensions Windows launches directly through CreateProcess or cmd.exe. */ +const windowsLaunchableExtensions = new Set([".bat", ".cmd", ".com", ".exe"]); + +/** + * Whether a host filesystem entry carries the host's executable marking. + * + * POSIX hosts mark executability with permission bits. Windows has no execute + * bit (Node reports file modes from the read-only attribute alone), so it marks + * launchable files by extension instead. + */ +export const hostExecutableMarking = ( + path: string, + metadata: Pick, + platform: NodeJS.Platform = process.platform, +): boolean => + platform === "win32" + ? metadata.isFile() && + windowsLaunchableExtensions.has(extname(path).toLowerCase()) + : (metadata.mode & 0o111) !== 0; diff --git a/src/artifacts/inventory/scanCanonical.ts b/src/artifacts/inventory/scanCanonical.ts index 1eb3a74d2b..ef4df6d713 100644 --- a/src/artifacts/inventory/scanCanonical.ts +++ b/src/artifacts/inventory/scanCanonical.ts @@ -34,6 +34,7 @@ import { artifactManifestId, } from "../../domain/artifactIdentity.js"; import { classifyAndHashRootForInventory } from "./classify.js"; +import { hostExecutableMarking } from "../hostExecutableMarking.js"; import type { HashResult } from "../ArtifactHash.js"; import { hashStableRootArtifact, @@ -211,7 +212,7 @@ const buildInventorySnapshot = async ( rekeyOccurrences(rootNode.artifact_id, input.occurrences); const rootOccurrence = rootOccurrenceFor(rootNode, { size: metadata.size, - executable: (metadata.mode & 0o111) !== 0, + executable: hostExecutableMarking(path, metadata), format: rootFormat, path, }); From 6ac5c36503492d3b6da4fcf2a5dee87aff60fc8c Mon Sep 17 00:00:00 2001 From: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com> Date: Sun, 11 Oct 2026 06:28:19 -0700 Subject: [PATCH 2/3] fix(javascript): follow bundler namespace interop for Electron APIs (#1827) * fix(javascript): follow bundler namespace interop for Electron APIs Rolldown/tsdown emit `let electron = require("electron"); electron = __toESM(electron);` and esbuild/tsc emit `__toESM(require("electron"))` or `__importStar(require("electron"))` for namespace imports. The reassignment or the helper call hid the Electron origin, so bundled main processes reported zero BrowserWindows and preloads. Locally declared, never reassigned interop helpers recognized by name now pass the module through, and a binding's rewrap of its own value is not treated as a reassignment. Other writes stay unresolved. Fixes #1802 Co-Authored-By: Claude Opus 5.5 * fix(javascript): require emitted interop helper definitions Only trust an interop helper name when it has a single program-scope function definition, and state that helper bodies are not evaluated. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- docs/javascript-artifact-reconstruction.md | 11 ++- .../electronStaticAnalysisIdentity.test.ts | 47 ++++++++++++ .../javascriptElectronMemberWrites.ts | 2 +- .../javascript/javascriptSemanticAnalysis.ts | 70 +++++++++++++++--- .../javascriptSemanticRequireOrigin.ts | 71 +++++++++++++++++++ 5 files changed, 187 insertions(+), 14 deletions(-) diff --git a/docs/javascript-artifact-reconstruction.md b/docs/javascript-artifact-reconstruction.md index 1c61520ea8..82e2cef99a 100644 --- a/docs/javascript-artifact-reconstruction.md +++ b/docs/javascript-artifact-reconstruction.md @@ -323,8 +323,15 @@ substitute version-dependent Electron defaults for omitted `webPreferences`. Dynamic option objects, bridge keys, API objects, and IPC channel expressions remain unknown and make coverage partial. -Electron API identity follows proven lexical module aliases. Reassigned bindings, -dynamic selections, and visibly overwritten namespace members are unresolved. +Electron API identity follows proven lexical module aliases. Bundled CommonJS +namespace imports are also followed through the interop helpers that bundlers +and compilers emit (`__toESM`, `__importStar`, `_interopRequireWildcard`), +including the `electron = __toESM(electron)` rewrap that Rolldown emits. This +step is inferred: a helper is recognized by its emitted name and a single +program-scope function definition, and its body is not evaluated, so a local +function that reuses one of these names is trusted to pass the module through. +Other reassigned bindings, dynamic selections, and visibly overwritten namespace +members are unresolved. Direct and destructuring assignment, loop, update, and delete targets are checked through namespace aliases; unrelated member writes do not discard observed API identity. This static check does diff --git a/src/domain/javascript/electronStaticAnalysisIdentity.test.ts b/src/domain/javascript/electronStaticAnalysisIdentity.test.ts index 78e4b314ca..99a3805e88 100644 --- a/src/domain/javascript/electronStaticAnalysisIdentity.test.ts +++ b/src/domain/javascript/electronStaticAnalysisIdentity.test.ts @@ -73,4 +73,51 @@ describe("Electron API identity", () => { }, ]); }); + + it("follows bundler namespace interop helpers without accepting other writes", () => { + const analysis = analyzeJavaScriptStaticSource(` + var __toESM = (mod) => mod; + var __importStar = (this && this.__importStar) || ((mod) => mod); + let electron = require("electron"); + electron = __toESM(electron); + new electron.BrowserWindow({ webPreferences: { preload: "./rolldown.js" } }); + var import_electron = __toESM(require("electron")); + new import_electron.BrowserWindow({ webPreferences: { preload: "./esbuild.js" } }); + function _interopRequireWildcard(mod) { return mod; } + const Electron = __importStar(require("electron")); + new Electron.BrowserWindow({ webPreferences: { preload: "./tsc.js" } }); + + let replaced = require("electron"); + replaced = __toESM(require("./fake")); + new replaced.BrowserWindow({ webPreferences: { preload: "./replaced.js" } }); + let rewritten = require("electron"); + rewritten = __toESM(rewritten); + rewritten = fake; + new rewritten.BrowserWindow({ webPreferences: { preload: "./rewritten.js" } }); + const babel = _interopRequireWildcard(require("electron")); + new babel.BrowserWindow({ webPreferences: { preload: "./babel.js" } }); + function nested() { + const __toESM = (mod) => mod; + const scoped = __toESM(require("electron")); + new scoped.BrowserWindow({ webPreferences: { preload: "./nested.js" } }); + } + `); + + expect( + analysis.electron.browser_windows.map(({ preload_path }) => preload_path), + ).toEqual(["./rolldown.js", "./esbuild.js", "./tsc.js", "./babel.js"]); + }); + + it("does not trust unbound helper names or non-function helper values", () => { + const analysis = analyzeJavaScriptStaticSource(` + const __toESM = makeLookalike(); + const electron = __toESM(require("electron")); + new electron.BrowserWindow({}); + let unbound = require("electron"); + unbound = __importStar(unbound); + new unbound.BrowserWindow({}); + `); + + expect(analysis.electron.browser_windows).toEqual([]); + }); }); diff --git a/src/domain/javascript/javascriptElectronMemberWrites.ts b/src/domain/javascript/javascriptElectronMemberWrites.ts index b8cd3839bd..696bb68aaa 100644 --- a/src/domain/javascript/javascriptElectronMemberWrites.ts +++ b/src/domain/javascript/javascriptElectronMemberWrites.ts @@ -24,7 +24,7 @@ export const electronExportPath = ( /** Static identity recovery cannot establish the absence of Electron usage. */ export const ELECTRON_IDENTITY_LIMITATION = - "Electron API identity follows proven lexical module aliases. Reassigned bindings, dynamic selections, and visibly overwritten namespace members are unresolved; missing findings do not establish absence. Arbitrary call side effects and runtime registration are not evaluated."; + "Electron API identity follows proven lexical module aliases, including results of namespace interop helpers (__toESM, __importStar, _interopRequireWildcard) recognized by name and a single program-scope function definition; helper bodies are not evaluated. Other reassigned bindings, dynamic selections, and visibly overwritten namespace members are unresolved; missing findings do not establish absence. Arbitrary call side effects and runtime registration are not evaluated."; /** Preserve exact property segments instead of flattening literal dotted keys. */ export const electronMemberPath = ( diff --git a/src/domain/javascript/javascriptSemanticAnalysis.ts b/src/domain/javascript/javascriptSemanticAnalysis.ts index 425cbef455..c4d8153c14 100644 --- a/src/domain/javascript/javascriptSemanticAnalysis.ts +++ b/src/domain/javascript/javascriptSemanticAnalysis.ts @@ -37,7 +37,10 @@ import { completeSemanticSteps, traverseJavaScriptAstSteps, } from "./javascriptSemanticTraversal.js"; -import { semanticRequireOrigin } from "./javascriptSemanticRequireOrigin.js"; +import { + semanticNamespaceInteropOperand, + semanticRequireOrigin, +} from "./javascriptSemanticRequireOrigin.js"; import { collectSemanticMemberMutationsSteps } from "./javascriptSemanticMemberMutations.js"; import { collectSemanticReturns, @@ -384,7 +387,10 @@ export function* classifyParsedJavaScriptElectronBindingsSteps( } return exported ?? undefined; } - const origin = semanticRequireOrigin(root, state)?.origin; + const origin = semanticRequireOrigin( + semanticNamespaceInteropOperand(root, state) ?? root, + state, + )?.origin; return origin !== undefined && ELECTRON_MODULE.test(origin.specifier) ? electronExportPath(origin.importedPath) : undefined; @@ -416,14 +422,15 @@ const electronExport = ( binding: JavaScriptSemanticBindingState, state: JavaScriptSemanticAnalysisState, ): readonly string[] | null => { - if (binding.definitions.some(({ kind }) => kind === "assignment")) - return null; - const directOrigin = bindingOrigin(binding, state); - const [initializer] = binding.initializers; + const initializers = namespacePreservingInitializers(binding, state); + if (initializers === undefined) return null; + const directOrigin = bindingOrigin(binding, initializers, state); + const [initializer] = initializers; if ( directOrigin === undefined && (initializer === undefined || - binding.initializers.length !== 1 || + initializers.length !== 1 || + initializers.length !== binding.initializers.length || state.conditionalInitializers.has(initializer.node)) ) return null; @@ -441,25 +448,66 @@ const electronExport = ( : null; }; +/** + * Return the initializers that can change which module a binding names. + * Bundlers emit `name = __toESM(name)` after a CommonJS require; rewrapping + * the binding's own value keeps every member, so it is not a reassignment. + */ +const namespacePreservingInitializers = ( + binding: JavaScriptSemanticBindingState, + state: JavaScriptSemanticAnalysisState, +): JavaScriptSemanticBindingState["initializers"] | undefined => { + const initializers = binding.initializers.filter( + ({ node }) => !rewrapsOwnNamespace(binding, node, state), + ); + const rewraps = binding.initializers.length - initializers.length; + const assignments = binding.definitions.filter( + ({ kind }) => kind === "assignment", + ).length; + return assignments === rewraps ? initializers : undefined; +}; + +const rewrapsOwnNamespace = ( + binding: JavaScriptSemanticBindingState, + node: t.Node, + state: JavaScriptSemanticAnalysisState, +): boolean => { + const assignment = state.parentsByNode.get(node); + const operand = semanticNamespaceInteropOperand(node, state); + return ( + t.isAssignmentExpression(assignment) && + assignment.operator === "=" && + assignment.right === node && + t.isIdentifier(assignment.left, { name: binding.name }) && + t.isIdentifier(operand, { name: binding.name }) && + !semanticResolutionBlocked(state, operand, operand.name) && + resolveSemanticBindingState(state, operand, operand.name) === binding + ); +}; + const bindingOrigin = ( binding: JavaScriptSemanticBindingState, + initializers: JavaScriptSemanticBindingState["initializers"], state: JavaScriptSemanticAnalysisState, ): JavaScriptModuleOrigin | undefined => { if (binding.directOrigins.length > 0) return binding.directOrigins.length === 1 ? binding.directOrigins[0] : undefined; - const [initializer] = binding.initializers; + const [initializer] = initializers; // A var/let declaration permits writes without proving one occurred. // Require one unconditional initializer and no actual binding writes. if ( initializer === undefined || - binding.initializers.length !== 1 || - binding.definitions.some(({ kind }) => kind === "assignment") || + initializers.length !== 1 || state.conditionalInitializers.has(initializer.node) ) return undefined; - const origin = semanticRequireOrigin(initializer.node, state)?.origin; + const origin = semanticRequireOrigin( + semanticNamespaceInteropOperand(initializer.node, state) ?? + initializer.node, + state, + )?.origin; const projection = initializer.projection; return origin === undefined || !projection.every((key): key is string => typeof key === "string") diff --git a/src/domain/javascript/javascriptSemanticRequireOrigin.ts b/src/domain/javascript/javascriptSemanticRequireOrigin.ts index 00f0805ba1..ec9ec9ae63 100644 --- a/src/domain/javascript/javascriptSemanticRequireOrigin.ts +++ b/src/domain/javascript/javascriptSemanticRequireOrigin.ts @@ -5,6 +5,7 @@ import { resolveSemanticBindingState, semanticResolutionBlocked, type JavaScriptSemanticAnalysisState, + type JavaScriptSemanticBindingState, } from "./javascriptSemanticState.js"; import { semanticStaticPropertyKey, @@ -58,3 +59,73 @@ export const semanticRequireOrigin = ( call: current, }; }; + +// esbuild/Rolldown, TypeScript, and Babel emit these helpers for namespace +// imports of CommonJS modules; each returns an object exposing every own +// member of its operand, so member paths through the result are unchanged. +// Helper bodies are not evaluated: recognition trusts a single program-scope +// function definition under one of these emitted names. +const NAMESPACE_INTEROP_HELPERS = new Set([ + "__importStar", + "__toESM", + "_interopRequireWildcard", +]); + +/** Return the module operand of a locally declared namespace interop helper. */ +export const semanticNamespaceInteropOperand = ( + node: t.Node, + state: JavaScriptSemanticAnalysisState, +): t.Expression | undefined => { + const call = unwrapJavaScriptExpression(node).node; + if ( + !t.isCallExpression(call) || + !t.isIdentifier(call.callee) || + !NAMESPACE_INTEROP_HELPERS.has(call.callee.name) || + semanticResolutionBlocked(state, call.callee, call.callee.name) + ) + return undefined; + const helper = resolveSemanticBindingState( + state, + call.callee, + call.callee.name, + ); + const [operand] = call.arguments; + return helper !== undefined && + emittedInteropHelper(helper, state) && + t.isExpression(operand) + ? operand + : undefined; +}; + +// Emitters declare each helper once at program scope as a function, an +// arrow/function expression, or `(this && this.name) || `. +const emittedInteropHelper = ( + helper: JavaScriptSemanticBindingState, + state: JavaScriptSemanticAnalysisState, +): boolean => { + const [initializer] = helper.initializers; + if ( + initializer === undefined || + helper.initializers.length !== 1 || + helper.definitions.length !== 1 || + (helper.kind !== "function" && helper.kind !== "variable") || + state.scopesById.get(helper.scopeId)?.kind !== "program" + ) + return false; + const value = unwrapJavaScriptExpression(initializer.node).node; + const emitted = + t.isLogicalExpression(value, { operator: "||" }) && + t.isLogicalExpression(unwrapJavaScriptExpression(value.left).node, { + operator: "&&", + }) + ? unwrapJavaScriptExpression(value.right).node + : value; + return ( + t.isFunctionDeclaration(emitted) || + t.isFunctionExpression(emitted) || + t.isArrowFunctionExpression(emitted) || + (t.isCallExpression(emitted) && + (t.isFunctionExpression(emitted.callee) || + t.isArrowFunctionExpression(emitted.callee))) + ); +}; From 0b4ce83618bb800627264ae6221e2e35140417b5 Mon Sep 17 00:00:00 2001 From: Ysopwnable <161784452+N0zoM1z0@users.noreply.github.com> Date: Sun, 11 Oct 2026 21:33:25 +0800 Subject: [PATCH 3/3] fix(windows): retain failed native inspection ownership (#1826) --- .github/workflows/ci.yml | 12 +++ docs/testing.md | 7 ++ native/windows/README.md | 24 +++++ native/windows/src/addon.cc | 40 +++++++- native/windows/src/authority.hpp | 2 +- native/windows/src/process.cc | 12 ++- scripts/build-windows-native.mjs | 11 ++- scripts/verify-windows-native-inspection.mjs | 95 +++++++++++++++++++ src/windows/WindowsNativeLoader.test.ts | 13 +-- src/windows/WindowsNativeLoader.ts | 23 ++++- .../windows-native-inspection-failure.cc | 20 ++++ 11 files changed, 240 insertions(+), 19 deletions(-) create mode 100644 scripts/verify-windows-native-inspection.mjs create mode 100644 tests/fixtures/windows-native-inspection-failure.cc diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5e35e443e5..cb7e620678 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -305,6 +305,12 @@ jobs: sudo apt-get install --no-install-recommends -y g++-mingw-w64-x86-64 - run: npm run build:windows-native - run: npm run verify:windows-native:artifact + - run: node scripts/build-windows-native.mjs --inspection-fixture + - uses: actions/upload-artifact@v4 + with: + name: windows-native-inspection-fixture + path: .cache/windows-native-inspection-fixture/ + if-no-files-found: error - uses: actions/upload-artifact@v4 with: name: windows-native-controls @@ -505,6 +511,12 @@ jobs: tests/process-global/updateMaintenance.test.ts tests/conformance/providers/ghidra/realGhidraWindowsWorkflow.test.ts - run: npm run verify:windows-native + - uses: actions/download-artifact@v4 + with: + name: windows-native-inspection-fixture + path: .cache/windows-native-inspection-fixture/ + - name: Verify native inspection failure ownership and recovery + run: node scripts/verify-windows-native-inspection.mjs - name: Verify real Node Inspector discovery and observation timeout-minutes: 10 env: diff --git a/docs/testing.md b/docs/testing.md index 548b1867d5..dbbdb0b3c4 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -482,6 +482,13 @@ not require Ghidra or Java. It includes Job Object commit-limit controls (a limited child that reaches the limit, a control that does not, and invalid limit rejection). An optional independently compiled Windows fixture adds in-place reparse, breakaway, environment, and token observations. +The separate `scripts/verify-windows-native-inspection.mjs` lane uses the +`.cache/windows-native-inspection-fixture` artifact built on Linux with +`node scripts/build-windows-native.mjs --inspection-fixture`. Its controlled +first deletion failure checks retained NTFS ownership, later-query recovery, +successful-admission caching, and environment-teardown cleanup. It neither +establishes the cause of the intermittent #1701 host failure nor replaces +verification with the shipped production artifact. `npm run verify:ghidra:windows:package` additionally packs and installs REA into an isolated prefix and runs ordinary-user CLI/MCP operations against their canonical schemas. Run the controlled fixture generator before that lane, or diff --git a/native/windows/README.md b/native/windows/README.md index 288fd03b19..82dd5526ba 100644 --- a/native/windows/README.md +++ b/native/windows/README.md @@ -106,6 +106,30 @@ npm run verify:windows-native npm run verify:ghidra:windows ``` +The native inspection probe retains its directory owner if deletion fails. A +later availability query retries that cleanup before creating another probe; +successful admission and static artifact failures remain cached. Environment +teardown makes one final cleanup attempt and reports a persistent failure to +stderr rather than claiming the directory was removed. + +CI separately builds and verifies a test-only artifact: + +```sh +node scripts/build-windows-native.mjs --inspection-fixture +``` + +On Windows, after compiling the runtime: + +```powershell +node scripts/verify-windows-native-inspection.mjs +``` + +This artifact injects one access-denied disposition result while using a real +NTFS directory. The verifier checks retained ownership, recovery on a later +query, cached successful admission, and cleanup on environment teardown. It +does not reproduce or identify the host-dependent cause reported in #1701. +The fixture is stored under `.cache/` and is excluded from the shipped addon. + The native lane checks DACL readback independently, source write/replacement rejection, unrelated sibling renames, reparse rejection, immutable snapshots, asynchronous cancellation, cleanup containment, diff --git a/native/windows/src/addon.cc b/native/windows/src/addon.cc index 4d332aad74..0e39b84c0b 100644 --- a/native/windows/src/addon.cc +++ b/native/windows/src/addon.cc @@ -1,6 +1,7 @@ #include "authority.hpp" #include +#include namespace rea { @@ -89,17 +90,45 @@ napi_value failureValue(napi_env env, const Failure& failure) { return error; } +struct InspectionContext { std::unique_ptr probe; }; + +static void cleanupInspection(void* data) noexcept { + std::unique_ptr context(static_cast(data)); + if (!context->probe) return; + try { closeRuntime(*context->probe); } + catch (const Failure& failure) { + // Environment teardown cannot retain an owner or return an error. Report + // terminal cleanup failure instead of claiming the directory was removed. + std::fprintf(stderr, "REA Windows probe cleanup failed (Win32 %lu) path=\"", + static_cast(failure.win32)); + // Emit reversible UTF-16 escapes without allocation or CRT locale conversion. + for (wchar_t unit : failure.path) { + if (unit == L'"' || unit == L'\\') std::fputc('\\', stderr); + if (unit >= 0x20 && unit <= 0x7e) std::fputc(static_cast(unit), stderr); + else std::fprintf(stderr, "\\u%04x", static_cast(unit)); + } + std::fprintf(stderr, "\"\n"); + } + catch (const std::exception& failure) { + std::fprintf(stderr, "REA Windows probe cleanup failed: %s\n", failure.what()); + } + catch (...) { + std::fprintf(stderr, "REA Windows probe cleanup failed: unknown native exception\n"); + } +} + static napi_value call(napi_env env, napi_callback_info info) { try { std::array args; size_t count = args.size(); - check(napi_get_cb_info(env, info, &count, args.data(), nullptr, nullptr)); + void* data = nullptr; + check(napi_get_cb_info(env, info, &count, args.data(), nullptr, &data)); require(count == 2, "Native call requires operation and arguments", L"", ERROR_INVALID_PARAMETER); const auto operation = wide(env, args[0]); const auto values = array(env, args[1]); if (operation == L"inspect") { require(values.empty(), "Inspect takes no arguments", L"", ERROR_INVALID_PARAMETER); - return inspect(env); + return inspect(env, static_cast(data)->probe); } if (operation.rfind(L"process_", 0) == 0) return processCall(env, operation, values); return filesystemCall(env, operation, values); @@ -114,8 +143,13 @@ static napi_value call(napi_env env, napi_callback_info info) { } static napi_value initialize(napi_env env, napi_value exports) { + auto context = std::make_unique(); + if (napi_add_env_cleanup_hook(env, cleanupInspection, context.get()) != napi_ok) return nullptr; + // The environment owns callback data before a function can expose it. Failed + // initialization still leaves a valid context for the registered cleanup hook. + auto* data = context.release(); napi_value function; - if (napi_create_function(env, "call", NAPI_AUTO_LENGTH, call, nullptr, &function) != napi_ok) return nullptr; + if (napi_create_function(env, "call", NAPI_AUTO_LENGTH, call, data, &function) != napi_ok) return nullptr; if (napi_set_named_property(env, exports, "call", function) != napi_ok) return nullptr; return exports; } diff --git a/native/windows/src/authority.hpp b/native/windows/src/authority.hpp index c003aae621..cc713ce0bd 100644 --- a/native/windows/src/authority.hpp +++ b/native/windows/src/authority.hpp @@ -132,6 +132,6 @@ std::unique_ptr spawnProcess(const std::wstring& command, const std::ws bool protocolStdin = false, ULONGLONG memoryLimit = 0); napi_value processCall(napi_env env, const std::wstring& operation, const std::vector& args); -napi_value inspect(napi_env env); +napi_value inspect(napi_env env, std::unique_ptr& probe); } // namespace rea diff --git a/native/windows/src/process.cc b/native/windows/src/process.cc index 8ebf06fb2e..ede2b56223 100644 --- a/native/windows/src/process.cc +++ b/native/windows/src/process.cc @@ -412,13 +412,17 @@ napi_value processCall(napi_env env, const std::wstring& operation, const std::v throw Failure("Unknown native process operation", L"", ERROR_INVALID_PARAMETER); } -napi_value inspect(napi_env env) { +napi_value inspect(napi_env env, std::unique_ptr& probe) { + // Settle the previous owner before admitting another probe. Failed cleanup + // retains its handles, including when identity construction itself failed. + if (probe) { closeRuntime(*probe); probe.reset(); } wchar_t temp[32768]; const auto length = GetTempPathW(32768, temp); require(length > 0 && length < 32768, "Read temporary directory coordinate failed"); - auto root = createRuntime(std::wstring(temp, length), L"rea-native-probe-"); - auto filesystem = identity(env, root->directory.get(), root->path); - closeRuntime(*root); + probe = createRuntime(std::wstring(temp, length), L"rea-native-probe-"); + auto filesystem = identity(env, probe->directory.get(), probe->path); + closeRuntime(*probe); + probe.reset(); auto job = createJob(); Attributes attributes(1); HANDLE raw = job.get(); diff --git a/scripts/build-windows-native.mjs b/scripts/build-windows-native.mjs index 152f3eddb3..e5eae61115 100644 --- a/scripts/build-windows-native.mjs +++ b/scripts/build-windows-native.mjs @@ -11,7 +11,12 @@ const root = join(dirname(fileURLToPath(import.meta.url)), ".."); const nodeVersion = "22.19.0"; const compiler = "x86_64-w64-mingw32-g++"; const cache = join(homedir(), ".cache", "rea-windows-native"); -const output = join(root, "native", "windows", "build"); +const inspectionFixture = process.argv[2] === "--inspection-fixture"; +if (process.argv.length > 3 || (process.argv.length > 2 && !inspectionFixture)) + throw new Error("Unknown Windows native build argument"); +const output = inspectionFixture + ? join(root, ".cache", "windows-native-inspection-fixture") + : join(root, "native", "windows", "build"); const run = (command, args) => { const result = spawnSync(command, args, { cwd: root, @@ -120,7 +125,9 @@ run(compiler, [ "-Wl,--no-insert-timestamp", `-I${headers}`, "native/windows/src/addon.cc", - "native/windows/src/filesystem.cc", + inspectionFixture + ? "tests/fixtures/windows-native-inspection-failure.cc" + : "native/windows/src/filesystem.cc", "native/windows/src/process.cc", library, "-ladvapi32", diff --git a/scripts/verify-windows-native-inspection.mjs b/scripts/verify-windows-native-inspection.mjs new file mode 100644 index 0000000000..e5f807da31 --- /dev/null +++ b/scripts/verify-windows-native-inspection.mjs @@ -0,0 +1,95 @@ +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import { + copyFile, + mkdir, + mkdtemp, + readdir, + rm, + symlink, + writeFile, +} from "node:fs/promises"; +import { createRequire } from "node:module"; +import { tmpdir } from "node:os"; +import { dirname, join, resolve } from "node:path"; +import { pathToFileURL } from "node:url"; + +if (process.platform !== "win32") + throw new Error("Native inspection ownership requires real Windows NTFS"); +const root = resolve(process.argv[2] ?? "."); +const artifact = resolve( + process.argv[3] ?? ".cache/windows-native-inspection-fixture", +); +const workspace = await mkdtemp(join(tmpdir(), "rea-inspection-conformance-")); +try { + // Isolate the fixture from the packaged production artifact and loader cache. + await mkdir(join(workspace, "dist", "windows"), { recursive: true }); + await mkdir(join(workspace, "native", "windows", "build"), { + recursive: true, + }); + await writeFile(join(workspace, "package.json"), '{"type":"module"}'); + await copyFile( + join(root, "dist", "windows", "WindowsNativeLoader.js"), + join(workspace, "dist", "windows", "WindowsNativeLoader.js"), + ); + await copyFile( + join(root, "dist", "generatedPackageMetadata.js"), + join(workspace, "dist", "generatedPackageMetadata.js"), + ); + for (const name of ["manifest.json", "rea-windows-x64.node"]) + await copyFile( + join(artifact, name), + join(workspace, "native", "windows", "build", name), + ); + const require = createRequire(join(root, "package.json")); + await mkdir(join(workspace, "node_modules")); + await symlink( + dirname(require.resolve("zod/package.json")), + join(workspace, "node_modules", "zod"), + "junction", + ); + const loader = pathToFileURL( + join(workspace, "dist", "windows", "WindowsNativeLoader.js"), + ).href; + const source = `import assert from 'node:assert/strict'; +import {readdirSync,renameSync,statSync} from 'node:fs'; +import {join} from 'node:path'; +import {systemWindowsNativeAuthority} from ${JSON.stringify(loader)}; +const temp=process.env.TEMP; +const first=systemWindowsNativeAuthority(); +assert.equal(first.available,false); +assert.equal(first.retryable,true); +assert.match(first.reason,/Delete owned object by handle failed.*Win32 5/s); +const [name,...extra]=readdirSync(temp);assert.ok(name);assert.equal(extra.length,0); +const path=join(temp,name);const identity=statSync(path,{bigint:true}); +assert.throws(()=>renameSync(path,path+'-replacement')); +assert.equal(statSync(path,{bigint:true}).ino,identity.ino); +if(process.argv[2]!=='teardown'){ + const second=systemWindowsNativeAuthority();assert.equal(second.available,true); + assert.equal(readdirSync(temp).length,0); + assert.equal(systemWindowsNativeAuthority(),second); +} +console.log(JSON.stringify({mode:process.argv[2],firstFailed:true,retainedOwner:true,inode:identity.ino.toString()})); +`; + const fixture = join(workspace, "probe.mjs"); + await writeFile(fixture, source); + for (const mode of ["retry", "teardown"]) { + const temp = join(workspace, mode); + await mkdir(temp); + const observed = JSON.parse( + execFileSync(process.execPath, [fixture, mode], { + encoding: "utf8", + timeout: 20_000, + windowsHide: true, + env: { ...process.env, TEMP: temp, TMP: temp }, + }), + ); + assert.equal(observed.retainedOwner, true); + assert.deepEqual(await readdir(temp), []); + } + process.stdout.write( + `${JSON.stringify({ ok: true, controlledDispositionFailure: true, retainedOwner: true, retryRecovers: true, successfulAdmissionCached: true, environmentTeardownCleansProbe: true, issue1701HostCauseReproduced: false })}\n`, + ); +} finally { + await rm(workspace, { recursive: true, force: true }); +} diff --git a/src/windows/WindowsNativeLoader.test.ts b/src/windows/WindowsNativeLoader.test.ts index 9e8cfe6b46..1aa8d73759 100644 --- a/src/windows/WindowsNativeLoader.test.ts +++ b/src/windows/WindowsNativeLoader.test.ts @@ -169,17 +169,18 @@ describe("packaged Windows native admission", () => { }), ).toMatchObject({ available: false, + retryable: true, reason: expect.stringContaining("private DACL: access denied"), }); }); it("requires every inspected native fact", () => { const value = fixture(); - expect( - loadWindowsNativeAuthority({ - ...value.host, - loadArtifact: () => ({ call: () => ({ privateDacl: true }) }), - }).available, - ).toBe(false); + const loaded = loadWindowsNativeAuthority({ + ...value.host, + loadArtifact: () => ({ call: () => ({ privateDacl: true }) }), + }); + expect(loaded.available).toBe(false); + expect(loaded).not.toHaveProperty("retryable"); }); }); diff --git a/src/windows/WindowsNativeLoader.ts b/src/windows/WindowsNativeLoader.ts index ba58359774..7719a0a854 100644 --- a/src/windows/WindowsNativeLoader.ts +++ b/src/windows/WindowsNativeLoader.ts @@ -67,7 +67,12 @@ export interface WindowsNativeAuthority { /** Availability of the bundled native implementation, including load failures. */ export type WindowsNativeLoadResult = | { readonly available: true; readonly authority: WindowsNativeAuthority } - | { readonly available: false; readonly reason: string }; + | { + readonly available: false; + readonly reason: string; + /** Only a failed native invocation can recover on a later query. */ + readonly retryable?: true; + }; /** Trusted package reads and module loading, replaceable in loader tests. */ export interface WindowsNativeLoadHost { @@ -106,7 +111,17 @@ export const loadWindowsNativeAuthority = ( ); assertWindowsX64Artifact(artifact); const binding = bindingSchema.parse(host.loadArtifact()); - const inspection = inspectionSchema.parse(binding.call("inspect", [])); + let observed: unknown; + try { + observed = binding.call("inspect", []); + } catch (cause: unknown) { + return { + available: false, + retryable: true, + reason: `REA Windows native self-test failed: ${cause instanceof Error ? cause.message : String(cause)}`, + }; + } + const inspection = inspectionSchema.parse(observed); return { available: true, authority: { call: binding.call, inspection } }; } catch (cause: unknown) { return { @@ -131,8 +146,10 @@ const assertWindowsX64Artifact = (bytes: Buffer): void => { let cached: WindowsNativeLoadResult | undefined; -/** Load only the package-owned artifact, lazily and once per process. */ +/** Cache stable admission; let a later query retry failed owned inspection. */ export const systemWindowsNativeAuthority = (): WindowsNativeLoadResult => { + if (cached?.available === false && cached.retryable === true) + cached = undefined; cached ??= loadWindowsNativeAuthority({ platform: process.platform, architecture: process.arch, diff --git a/tests/fixtures/windows-native-inspection-failure.cc b/tests/fixtures/windows-native-inspection-failure.cc new file mode 100644 index 0000000000..df93e48535 --- /dev/null +++ b/tests/fixtures/windows-native-inspection-failure.cc @@ -0,0 +1,20 @@ +// Only the separate conformance artifact uses this translation unit. The +// shipped addon always compiles filesystem.cc directly, without interposition. +#include "../../native/windows/src/authority.hpp" +#include + +static BOOL inspectionDisposition(HANDLE handle, FILE_INFO_BY_HANDLE_CLASS kind, + LPVOID value, DWORD size) { + // A real NTFS probe is created normally. Fail its first disposition call so + // the verifier can observe ownership after failure, independently of host + // filter drivers or the intermittent error reported in issue #1701. + static std::atomic_flag failed = ATOMIC_FLAG_INIT; + if (kind == FileDispositionInfo && !failed.test_and_set()) { + SetLastError(ERROR_ACCESS_DENIED); + return FALSE; + } + return SetFileInformationByHandle(handle, kind, value, size); +} + +#define SetFileInformationByHandle inspectionDisposition +#include "../../native/windows/src/filesystem.cc"