diff --git a/electron/media/audioPeaks.test.ts b/electron/media/audioPeaks.test.ts index fb20b2677..a49c28b06 100644 --- a/electron/media/audioPeaks.test.ts +++ b/electron/media/audioPeaks.test.ts @@ -1,7 +1,8 @@ // @vitest-environment node -import { existsSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import path from "node:path"; -import { describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { ffmpegCandidates, peakBlockCount, resolveFfmpeg } from "./audioPeaks"; const ROOT = path.resolve(__dirname, "..", ".."); @@ -52,6 +53,143 @@ describe("ffmpeg resolution", () => { it("returns null rather than throwing when nothing is staged", () => { expect(resolveFfmpeg(path.join(ROOT, "does", "not", "exist"))).toBeNull(); }); + + /** + * The shape that slipped through. A Linux dev checkout can have + * `electron/native/bin//ffmpeg` as a DIRECTORY of shared libraries + * rather than the binary; `existsSync` accepted it, resolution stopped + * there, and the failure only surfaced later as `spawn … EACCES`. + */ + it("skips a candidate that is a directory rather than the binary", () => { + const here = mkdtempSync(path.join(tmpdir(), "openscreen-ffmpeg-")); + const tag = `${process.platform}-${process.arch}`; + const name = process.platform === "win32" ? "ffmpeg-shared.exe" : "ffmpeg"; + const staged = path.join(here, "electron", "native", "bin", tag, name); + try { + // A directory sitting exactly where the executable is looked for. + mkdirSync(staged, { recursive: true }); + writeFileSync(path.join(staged, "libavcodec.so.62"), ""); + + expect(resolveFfmpeg(here)).toBeNull(); + } finally { + rmSync(here, { recursive: true, force: true }); + } + }); + + it("accepts a candidate that is an executable file", () => { + const here = mkdtempSync(path.join(tmpdir(), "openscreen-ffmpeg-")); + const tag = `${process.platform}-${process.arch}`; + const name = process.platform === "win32" ? "ffmpeg-shared.exe" : "ffmpeg"; + const staged = path.join(here, "electron", "native", "bin", tag, name); + try { + mkdirSync(path.dirname(staged), { recursive: true }); + writeFileSync(staged, "", { mode: 0o755 }); + + expect(resolveFfmpeg(here)).toBe(staged); + } finally { + rmSync(here, { recursive: true, force: true }); + } + }); + + // Non-executable files are the other half of the predicate, and the check is + // only meaningful where the OS enforces the bit. + it.runIf(process.platform !== "win32")( + "skips a candidate that is a file but not executable", + () => { + const here = mkdtempSync(path.join(tmpdir(), "openscreen-ffmpeg-")); + const staged = path.join( + here, + "electron", + "native", + "bin", + `${process.platform}-${process.arch}`, + "ffmpeg", + ); + try { + mkdirSync(path.dirname(staged), { recursive: true }); + writeFileSync(staged, "", { mode: 0o644 }); + + expect(resolveFfmpeg(here)).toBeNull(); + } finally { + rmSync(here, { recursive: true, force: true }); + } + }, + ); + + /** + * REJECTING IS NOT THE SAME AS CONTINUING, and only the second is the + * property the predicate exists for: swallowing every failure is what stops + * one bad path from denying a later working one. The tests above prove the + * first — with a single candidate staged, `null` is equally consistent with + * "skipped it" and "gave up on the whole list". + * + * `OPENSCREEN_FFMPEG_PATH` is the vehicle because `ffmpegCandidates` puts it + * FIRST, so a bad value there is the one case that could shadow every real + * candidate behind it. + */ + describe("falling through to a later candidate", () => { + let here: string; + let staged: string; + + beforeEach(() => { + here = mkdtempSync(path.join(tmpdir(), "openscreen-ffmpeg-")); + staged = path.join( + here, + "electron", + "native", + "bin", + `${process.platform}-${process.arch}`, + process.platform === "win32" ? "ffmpeg-shared.exe" : "ffmpeg", + ); + mkdirSync(path.dirname(staged), { recursive: true }); + writeFileSync(staged, "", { mode: 0o755 }); + }); + + afterEach(() => { + delete process.env.OPENSCREEN_FFMPEG_PATH; + rmSync(here, { recursive: true, force: true }); + }); + + it("passes over a leading candidate that does not exist", () => { + process.env.OPENSCREEN_FFMPEG_PATH = path.join(here, "nowhere", "ffmpeg"); + + expect(resolveFfmpeg(here)).toBe(staged); + }); + + it("passes over a leading candidate that is a directory", () => { + const decoy = path.join(here, "decoy-ffmpeg"); + mkdirSync(decoy, { recursive: true }); + writeFileSync(path.join(decoy, "libavcodec.so.62"), ""); + process.env.OPENSCREEN_FFMPEG_PATH = decoy; + + expect(resolveFfmpeg(here)).toBe(staged); + }); + + it.runIf(process.platform !== "win32")( + "passes over a leading candidate that is not executable", + () => { + const decoy = path.join(here, "decoy-ffmpeg"); + writeFileSync(decoy, "", { mode: 0o644 }); + process.env.OPENSCREEN_FFMPEG_PATH = decoy; + + expect(resolveFfmpeg(here)).toBe(staged); + }, + ); + + // An unreadable-but-executable binary is legitimate on Unix, so it must be + // ACCEPTED rather than fallen through — `X_OK` is deliberately not paired + // with `R_OK`. Skipped as root, for whom access checks always pass. + it.runIf(process.platform !== "win32" && process.getuid?.() !== 0)( + "still accepts an execute-only binary", + () => { + const executableOnly = path.join(here, "exec-only-ffmpeg"); + writeFileSync(executableOnly, "", { mode: 0o111 }); + process.env.OPENSCREEN_FFMPEG_PATH = executableOnly; + + expect(resolveFfmpeg(here)).toBe(executableOnly); + }, + ); + }); }); // Only runs where the binary is actually staged; skipped elsewhere rather than diff --git a/electron/media/audioPeaks.ts b/electron/media/audioPeaks.ts index 056cab54e..f1beabeb5 100644 --- a/electron/media/audioPeaks.ts +++ b/electron/media/audioPeaks.ts @@ -1,6 +1,6 @@ import { spawn } from "node:child_process"; import { createHash } from "node:crypto"; -import { existsSync } from "node:fs"; +import { accessSync, constants as fsConstants, statSync } from "node:fs"; import { mkdir, readFile, stat, writeFile } from "node:fs/promises"; import path from "node:path"; import { app } from "electron"; @@ -98,10 +98,46 @@ export function ffmpegCandidates(here: string = process.cwd()): string[] { let cachedFfmpeg: string | null | undefined; -/** First candidate that exists, or null when none does (callers fall back). */ +/** + * Whether a candidate is something that can actually be run. + * + * EXISTENCE IS NOT ENOUGH, and the difference is not academic. `existsSync` was + * the test here, and it answers true for a DIRECTORY: on a Linux dev machine + * `electron/native/bin//ffmpeg` is a folder holding the shared libraries + * (`libavcodec.so.62` and friends) rather than the binary, so resolution picked + * the folder, every later candidate was skipped, and the failure surfaced much + * later as `spawn … EACCES` — a message that blames permissions rather than + * saying the wrong candidate was chosen. + * + * Every failure mode is swallowed on purpose. A candidate that is absent, not a + * regular file, or not executable is simply not this one; throwing out of + * resolution would let a single bad path deny a later, working one. + * + * `X_OK` and not `X_OK | R_OK`: executing a binary needs the execute bit, not + * the read bit, so an install shipped `--x` is legitimate and must not be + * refused. On Windows `X_OK` is not enforced at all — there the `isFile` check + * is the whole guard, which is enough for the failure this exists to stop, + * since `ffmpegCandidates` only ever proposes `.exe` names of its own. + */ +function isExecutableFile(candidate: string): boolean { + try { + if (!statSync(candidate).isFile()) { + return false; + } + accessSync(candidate, fsConstants.X_OK); + return true; + } catch { + return false; + } +} + +/** + * First candidate that is an executable file, or null when none is (callers + * fall back). + */ export function resolveFfmpeg(here?: string): string | null { if (cachedFfmpeg !== undefined && here === undefined) return cachedFfmpeg; - const found = ffmpegCandidates(here).find((p) => existsSync(p)) ?? null; + const found = ffmpegCandidates(here).find(isExecutableFile) ?? null; if (here === undefined) cachedFfmpeg = found; return found; }