From baad9eebe4c4a381be1e69ab0021acfea382f339 Mon Sep 17 00:00:00 2001 From: Kaoru0822 Date: Wed, 14 Oct 2026 20:21:34 -0700 Subject: [PATCH] fix(javascript): reject FIFO recovery inputs without blocking (#3788) * fix(javascript): reject FIFO recovery inputs without blocking scanRecoveryFile opened the selected path with a blocking O_RDONLY open before checking that it is a regular file. A FIFO with no writer blocks that open in the libuv threadpool, so cancellation and the recovery deadline could not settle the read, and the process could not exit. Admit the input through the shared openRegularFile owner, which opens with O_NONBLOCK and verifies a regular file before any read. A FIFO, directory or socket now fails admission immediately with the existing regular-file error; symlink rejection and cancellation are unchanged. Fixes #3332 Co-Authored-By: Claude Opus 5.5 * fix(javascript): keep a failed admission close owned on cancellation Rethrow RegularFileAdmissionFailure before mapping cancellation, so a descriptor whose close failed stays owned by its error. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- src/javascript/recovery/RecoveryFiles.test.ts | 55 +++++++++++++++++++ src/javascript/recovery/RecoveryFiles.ts | 27 +++++++-- .../wakaruIntegration.test.ts | 32 ++++++++++- 3 files changed, 109 insertions(+), 5 deletions(-) create mode 100644 src/javascript/recovery/RecoveryFiles.test.ts diff --git a/src/javascript/recovery/RecoveryFiles.test.ts b/src/javascript/recovery/RecoveryFiles.test.ts new file mode 100644 index 0000000000..574699abd5 --- /dev/null +++ b/src/javascript/recovery/RecoveryFiles.test.ts @@ -0,0 +1,55 @@ +import { execFile } from "node:child_process"; +import { writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { promisify } from "node:util"; +import { expect, it, vi } from "vitest"; +import { AnalysisOutputError } from "../../domain/analysisErrorCore.js"; +import { OwnedFileHandle } from "../../filesystem/OwnedFileHandle.js"; +import { + openRegularFile, + RegularFileAdmissionFailure, +} from "../../filesystem/RegularFile.js"; +import { readWithoutFifoWriter } from "../../../tests/fixtures/fifoInput.js"; +import { createTestTempDirectory } from "../../../tests/fixtures/temporaryDirectory.js"; +import { readRecoveryFile } from "./RecoveryFiles.js"; + +vi.mock("../../filesystem/RegularFile.js", async (importOriginal) => { + const actual = + await importOriginal(); + return { ...actual, openRegularFile: vi.fn(actual.openRegularFile) }; +}); + +it.skipIf(process.platform === "win32")( + "rejects a named pipe without waiting for a writer", + async () => { + const root = await createTestTempDirectory("rea-recovery-fifo-"); + const regular = join(root, "regular.js"); + await writeFile(regular, "module.exports = 1;\n"); + await expect(readRecoveryFile(regular, 1024)).resolves.toHaveLength(20); + + const fifoPath = join(root, "sample.js"); + await promisify(execFile)("mkfifo", [fifoPath]); + const outcome = await readWithoutFifoWriter(fifoPath, () => + readRecoveryFile(fifoPath, 1024).catch((cause: unknown) => cause), + ); + if (outcome.state !== "completed") + throw new Error("Recovery input read waited for a FIFO writer"); + expect(outcome.result).toBeInstanceOf(AnalysisOutputError); + }, +); + +it("keeps a failed admission close owned when cancellation is also requested", async () => { + const owner = new OwnedFileHandle({ fd: -1 } as never); + const failure = new RegularFileAdmissionFailure( + "/selected.js", + new Error("cancelled during admission"), + new Error("close failed"), + owner, + ); + vi.mocked(openRegularFile).mockRejectedValueOnce(failure); + const controller = new AbortController(); + controller.abort(); + await expect( + readRecoveryFile("/selected.js", 1024, controller.signal), + ).rejects.toBe(failure); +}); diff --git a/src/javascript/recovery/RecoveryFiles.ts b/src/javascript/recovery/RecoveryFiles.ts index cd1dcb6669..6cea1b81b6 100644 --- a/src/javascript/recovery/RecoveryFiles.ts +++ b/src/javascript/recovery/RecoveryFiles.ts @@ -1,6 +1,5 @@ import { createHash } from "node:crypto"; -import { constants } from "node:fs"; -import { lstat, mkdir, open, opendir, writeFile } from "node:fs/promises"; +import { lstat, mkdir, opendir, writeFile } from "node:fs/promises"; import { dirname, isAbsolute, join, relative, resolve, sep } from "node:path"; import { TextDecoder } from "node:util"; import { normalizeArtifactPath } from "../../artifacts/ArtifactPaths.js"; @@ -10,6 +9,11 @@ import { AnalysisOutputError, AnalysisTimeoutError, } from "../../domain/analysisErrorCore.js"; +import { + NonRegularFileReadError, + openRegularFile, + RegularFileAdmissionFailure, +} from "../../filesystem/RegularFile.js"; import { RECOVERY_LIMITS } from "./WakaruRelease.js"; const OPERATION = "recover_javascript_sources"; @@ -25,11 +29,26 @@ const scanRecoveryFile = async ( consume: (chunk: Buffer) => void, signal?: AbortSignal, ) => { - const handle = await open(path, constants.O_RDONLY | constants.O_NOFOLLOW); + // A FIFO without a writer must be rejected before it can block the open. + const handle = await openRegularFile(path, { + symlinks: "reject", + signal, + }).catch((cause: unknown) => { + // A descriptor whose close failed stays owned by its error, even when + // cancellation was also requested. + if (cause instanceof RegularFileAdmissionFailure) throw cause; + checkRecoveryCancellation(signal); + if (cause instanceof NonRegularFileReadError) + throw new AnalysisOutputError( + OPERATION, + `Expected a regular file no larger than ${String(maximum)} bytes: ${path}`, + ); + throw cause; + }); const hash = createHash("sha256"); try { const before = await handle.stat(); - if (!before.isFile() || before.size > maximum) + if (before.size > maximum) throw new AnalysisOutputError( OPERATION, `Expected a regular file no larger than ${String(maximum)} bytes: ${path}`, diff --git a/tests/boundary/javascript-recovery/wakaruIntegration.test.ts b/tests/boundary/javascript-recovery/wakaruIntegration.test.ts index 9cb27999eb..8e34ba1973 100644 --- a/tests/boundary/javascript-recovery/wakaruIntegration.test.ts +++ b/tests/boundary/javascript-recovery/wakaruIntegration.test.ts @@ -1,5 +1,7 @@ +import { execFile } from "node:child_process"; import { access, mkdir, open, readFile, writeFile } from "node:fs/promises"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; +import { promisify } from "node:util"; import { describe, expect, it, vi } from "vitest"; import { SafeOutputTree } from "../../../src/artifacts/SafeOutputTree.js"; import { SafeOutputTreeCreationFailure } from "../../../src/artifacts/SafeOutputTreeCreationFailure.js"; @@ -11,6 +13,7 @@ import { assertRecoveryCleanup, recoveryFixture, } from "../../fixtures/javascript-recovery/provider.js"; +import { readWithoutFifoWriter } from "../../fixtures/fifoInput.js"; describe.skipIf(process.platform !== "linux" || process.arch !== "x64")( "Wakaru recovery boundary", @@ -548,3 +551,30 @@ describe.skipIf(process.platform !== "linux" || process.arch !== "x64")( }); }, ); + +describe.skipIf(process.platform !== "linux" || process.arch !== "x64")( + "Wakaru input admission", + () => { + it("rejects a named pipe without waiting for a writer or launching recovery", async () => { + const fixture = await recoveryFixture(); + const fifoPath = join(dirname(fixture.path), "pipe.js"); + await promisify(execFile)("mkfifo", [fifoPath]); + const outcome = await readWithoutFifoWriter(fifoPath, () => + fixture.service.recover({ + path: fifoPath, + output_directory: fixture.output, + }), + ); + if (outcome.state !== "completed") + throw new Error("Recovery input read waited for a FIFO writer"); + if (outcome.result.ok) throw new Error("expected non-regular input"); + expect(projectAnalysisError(outcome.result.error).code).toBe( + "invalid_request", + ); + expect(fixture.launches).toHaveLength(0); + await expect(access(fixture.output)).rejects.toMatchObject({ + code: "ENOENT", + }); + }); + }, +);