diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5e35e443e..cb7e62067 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 548b1867d..dbbdb0b3c 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 288fd03b1..82dd5526b 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 4d332aad7..0e39b84c0 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 c003aae62..cc713ce0b 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 8ebf06fb2..ede2b5622 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 152f3eddb..e5eae6111 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 000000000..e5f807da3 --- /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 9e8cfe6b4..1aa8d7375 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 ba5835977..7719a0a85 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 000000000..df93e4853 --- /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"