From b668cea2af0fa2608885563195eef6cdea0cb602 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=90=B4=E4=BD=B3=E6=B5=A9Alben?= <35291372+XiaoBinGan@users.noreply.github.com> Date: Wed, 4 Nov 2026 19:43:33 +0800 Subject: [PATCH] fix(javascript): emit no fs acquisition for a literal non-flag string (#8125) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(javascript): emit no fs acquisition for a literal non-flag string `fs.open` and `fs.openSync` validate a literal flag before touching the filesystem: if (!isFd(flags) && !FLAGS_SET.has(flags)) throw new ERR_INVALID_ARG_VALUE('flags', flags, ...); The throw is synchronous, before a descriptor exists, but the analysis reported an `acquire` operation for those calls. Measured on Node 24.22.1 against a file that does not exist, so each valid flag is exercised without an EEXIST masking it: ACCEPTED as a flags string: a a+ as as+ ax ax+ r r+ rs rs+ sa sa+ sr sr+ w w+ wx wx+ xa xa+ xw xw+ REJECTED with ERR_INVALID_ARG_VALUE: "" A R W rb wb s s+ x r,x A Python-style `"rb"`/`"wb"` is the common case: those are valid in Python and fatal at the Node call site. `FS_OPEN_FLAGS` in `javascriptStaticAnalysisHelpers.ts` already holds exactly those 22 accepted flags. It is now exported and reused so the accepted set has one owner. A numeric flag is a platform bitmask and a computed value may still be valid, so only an exactly readable non-flag string is dropped. `fs.createReadStream` keeps its acquisition: it does not validate the flag synchronously, it returns a stream and reports the failure on the error event. * chore: retrigger title and readiness checks after the title was corrected * fix(javascript): reject every fs flag literal Node rejects The first version only decided string flags. Node's check covers every flag shape, and each one below throws `ERR_INVALID_ARG_VALUE` or `ERR_OUT_OF_RANGE` synchronously, before a descriptor exists: openSync(p, 1.5) -> THREW ERR_OUT_OF_RANGE openSync(p, -1) -> THREW EINVAL openSync(p, -1.5) -> THREW ERR_OUT_OF_RANGE openSync(p, 1e400) -> THREW ERR_OUT_OF_RANGE openSync(p, NaN) -> THREW ERR_OUT_OF_RANGE openSync(p, Infinity) -> THREW ERR_OUT_OF_RANGE openSync(p, 0n) -> THREW ERR_INVALID_ARG_VALUE openSync(p, true) -> THREW ERR_INVALID_ARG_VALUE openSync(p, false) -> THREW ERR_INVALID_ARG_VALUE openSync(p, {}) -> THREW ERR_INVALID_ARG_VALUE openSync(p, []) -> THREW ERR_INVALID_ARG_VALUE --- accepted --- openSync(p, 0) / 1 / 65536 / +5 -> ok openSync(p) / undefined / null -> ok openSync(p, "r") / "wx" / "sr" -> ok `NaN` and `Infinity` are identifiers rather than literals, so they are decided only through `isUnmodifiedGlobalReference`; a shadowed binding keeps the acquisition. * fix(javascript): judge an fs flag by its value, and ignore a shifting spread Two P1 findings on the first version. **A unary operator was judged by its operand.** `openSync(p, void 1.5)` passes `undefined`, so Node applies the default read flag, but the branch saw a fractional operand and dropped a real acquisition. `openSync(p, -0)` is the valid zero bitmask and was also dropped. `staticFlagBits` now evaluates the operator: openSync(p, void 1.5) -> ok (keeps the acquisition) openSync(p, void 0) -> ok openSync(p, -0) -> ok openSync(p, -0.0) -> ok openSync(p, +0) -> ok openSync(p, ~-1) -> ok openSync(p, -1) -> THREW EINVAL openSync(p, -1.5) -> THREW ERR_OUT_OF_RANGE openSync(p, +1.5) -> THREW ERR_OUT_OF_RANGE openSync(p, !0) -> THREW ERR_INVALID_ARG_VALUE openSync(p, ~0) -> THREW EINVAL openSync(p, typeof 1) -> THREW ERR_INVALID_ARG_VALUE **A spread before the flag slot shifts the runtime position.** With `args = [path, "w"]`, `openSync(...args, "0600")` opens with mode `"0600"`, but argument 1 is then the mode rather than the flag. The check now keeps the acquisition whenever argument 0 is a spread, because the flag it inspects is not provably the flag at runtime. * fix(javascript): leave a signed fs bitmask undecided greptile caught that the flag check rejected every negative integer, but Node accepts some signed bitmasks. Measured on Node 24.22.1 against an existing file: openSync(p, -2147483648) -> ok openSync(p, -1073741824) -> ok openSync(p, -1) -> THREW EINVAL openSync(p, -2) -> THREW EINVAL openSync(p, 2147483647) -> THREW EINVAL openSync(p, 2147483648) -> THREW ERR_OUT_OF_RANGE openSync(p, -2147483649) -> THREW ERR_OUT_OF_RANGE So the sign does not decide: some negative values open a descriptor and others do not, and which is which is a platform bitmask question this analysis cannot settle. The check now rejects only a non-integer or a value outside the signed 32-bit range, and leaves an in-range negative undecided — the same choice `greptile` asked for, on the principle that a call which may succeed must keep its acquisition. `!`, `typeof` and `delete` are still rejected, because those yield a boolean or a string regardless of the operand. * fix(javascript): read a no-expression template literal flag Review finding. A no-expression template literal is exactly readable, so it is the same flag string Node receives, but the guard only read `StringLiteral`: fs.openSync(p, ` + "`rb`" + `) -> THREW ERR_INVALID_ARG_VALUE fs.openSync(p, ` + "`r`" + `) -> ok The flag slot now accepts an interpolation-free template; an interpolated template stays undecided. Four regressions cover both directions; two fail without the change. --- ...SemanticAnalysis.open_flag_literal.test.ts | 135 ++++++++++++++++++ .../javascript/javascriptSemanticResources.ts | 91 +++++++++++- .../javascriptStaticAnalysisHelpers.ts | 2 +- 3 files changed, 226 insertions(+), 2 deletions(-) create mode 100644 src/domain/javascript/javascriptSemanticAnalysis.open_flag_literal.test.ts diff --git a/src/domain/javascript/javascriptSemanticAnalysis.open_flag_literal.test.ts b/src/domain/javascript/javascriptSemanticAnalysis.open_flag_literal.test.ts new file mode 100644 index 0000000000..cd1106b3a0 --- /dev/null +++ b/src/domain/javascript/javascriptSemanticAnalysis.open_flag_literal.test.ts @@ -0,0 +1,135 @@ +import { expect, it } from "vitest"; + +import { analyzeJavaScriptSemantics } from "./javascriptSemanticAnalysis.js"; + +/** + * `fs.open` and `fs.openSync` validate a literal flag string before touching the + * filesystem and throw `ERR_INVALID_ARG_VALUE` synchronously, so no descriptor + * exists and no resource is acquired. Node's accepted flag set is measured, not + * guessed: `a`, `a+`, `as`, `as+`, `ax`, `ax+`, `r`, `r+`, `rs`, `rs+`, `sa`, + * `sa+`, `sr`, `sr+`, `w`, `w+`, `wx`, `wx+`, `xa`, `xa+`, `xw`, `xw+`. + * + * A numeric flag is a platform bitmask and a computed value may still be a valid + * flag, so only an exactly readable non-flag string is dropped. + */ +const acquisitionsFor = (call: string) => + analyzeJavaScriptSemantics( + `import { open, openSync, createReadStream } from "node:fs";\nimport { open as openPromise } from "node:fs/promises";\nfunction g(p, cb) { ${call}; }`, + ).resourceOperations.map(({ kind, method }) => ({ kind, method })); + +it.each([ + ["a Python-style read flag", `openSync(p, "rb")`], + ["a Python-style write flag", `openSync(p, "wb")`], + ["an append-binary flag", `openSync(p, "ab")`], + ["a lowercase x flag", `openSync(p, "x")`], + ["a comma flag", `openSync(p, "r,x")`], + ["an empty flag", `openSync(p, "")`], + ["an uppercase flag", `openSync(p, "R")`], + ["a callback-form read flag", `open(p, "rb", cb)`], + ["a promise-form read flag", `openPromise(p, "rb")`], + // Node also rejects every non-string flag that is not a nullish value or a + // usable bitmask integer. + ["a fractional flag", `openSync(p, 1.5)`], + // A value outside the signed 32-bit range cannot be a bitmask at all. + ["an int32-overflow flag", `openSync(p, 2147483648)`], + ["an int32-underflow flag", `openSync(p, -2147483649)`], + ["a negated fractional flag", `openSync(p, -1.5)`], + ["a non-finite numeric flag", `openSync(p, 1e400)`], + ["a NaN flag", `openSync(p, NaN)`], + ["an Infinity flag", `openSync(p, Infinity)`], + ["a BigInt flag", `openSync(p, 0n)`], + ["a true flag", `openSync(p, true)`], + ["a false flag", `openSync(p, false)`], + ["an object flag", `openSync(p, {})`], + ["an array flag", `openSync(p, [])`], + // A unary operator is judged by its result, not by its operand: `!` yields a + // boolean and `~0` yields `-1`. + ["a logical-not flag", `openSync(p, !0)`], +] as const)("emits no acquisition for %s", (_name, call) => { + expect(acquisitionsFor(call)).toEqual([]); +}); + +it.each([ + ["a read flag", `openSync(p, "r")`], + ["a read-plus flag", `openSync(p, "r+")`], + ["an append-plus flag", `openSync(p, "a+")`], + ["an exclusive write flag", `openSync(p, "wx")`], + ["a symbolic-link flag", `openSync(p, "sr")`], + ["an omitted flag", `openSync(p)`], + ["a computed flag", `openSync(p, flag)`], + ["a numeric bitmask", `openSync(p, 1)`], + ["a zero bitmask", `openSync(p, 0)`], + ["a large bitmask", `openSync(p, 65536)`], + ["a unary-plus bitmask", `openSync(p, +5)`], + ["an explicit undefined flag", `openSync(p, undefined)`], + ["an explicit null flag", `openSync(p, null)`], + // `void ` is `undefined`, which Node accepts as the default read flag, + // so the operand must never decide the outcome. + ["a void flag over a fraction", `openSync(p, void 1.5)`], + ["a void flag over a string", `openSync(p, void "r")`], + // `-0` is the valid zero bitmask, and `~-1` is `0`. + // The sign does not prove rejection: Node accepts some signed bitmasks such as + // `-2147483648` and rejects others such as `-1`, so an in-range negative is + // deliberately left undecided and keeps its acquisition. + ["a minimum int32 bitmask", `openSync(p, -2147483648)`], + ["a negative power-of-two bitmask", `openSync(p, -1073741824)`], + ["an in-range negative bitmask", `openSync(p, -1)`], + // `~0` is `-1` and `~1` is `-2`. Node happens to reject both, but that is the + // same undecidable signed-bitmask question, so they stay undecided too. + ["a bitwise-not of zero", `openSync(p, ~0)`], + ["a bitwise-not of one", `openSync(p, ~1)`], + ["a maximum int32 bitmask", `openSync(p, 2147483647)`], + ["a negative-zero flag", `openSync(p, -0)`], + ["a negative-zero expression flag", `openSync(p, -0.0)`], + ["a unary-plus zero flag", `openSync(p, +0)`], + ["a bitwise-not of negative one", `openSync(p, ~-1)`], + // A spread before the flag slot shifts the runtime position, so argument 1 is + // not provably the flag: `openSync(...args, "0600")` opens with a mode. + ["a spread before the flag", `openSync(...args, "0600")`], + ["a spread supplying every argument", `openSync(...args)`], + ["a spread in the flag slot", `openSync(p, ...flags)`], +] as const)("keeps the acquisition for %s", (_name, call) => { + expect(acquisitionsFor(call)).toEqual([ + { kind: "acquire", method: "openSync" }, + ]); +}); + +it("keeps the callback-form acquisition with an accepted flag", () => { + expect(acquisitionsFor(`open(p, "r", cb)`)).toEqual([ + { kind: "acquire", method: "open" }, + ]); +}); + +/** + * `fs.createReadStream` does not validate the flag synchronously: it returns a + * stream and surfaces the failure on the stream's error event, so the stream + * object is still acquired. + */ +it("keeps a read stream whose flags option is invalid", () => { + expect(acquisitionsFor(`createReadStream(p, { flags: "rb" })`)).toEqual([ + { kind: "acquire", method: "createReadStream" }, + ]); +}); + +/** + * Review finding: a no-expression template literal is exactly readable, so it is the + * same flag string Node receives. + * + * fs.openSync(p, `rb`) -> THREW ERR_INVALID_ARG_VALUE + * fs.openSync(p, `r`) -> ok + */ +it.each([ + ["a rejected template flag", 'openSync("x", `rb`)'], + ["another rejected template flag", 'openSync("x", `bad`)'], +] as const)("emits no fs acquisition for %s", (_name, call) => { + expect(acquisitionsFor(call)).toEqual([]); +}); + +it.each([ + ["an accepted template flag", 'openSync("x", `r`)'], + ["an interpolated template flag", 'openSync("x", `${flag}`)'], +] as const)("keeps the fs acquisition for %s", (_name, call) => { + expect(acquisitionsFor(call)).toEqual([ + { kind: "acquire", method: "openSync" }, + ]); +}); diff --git a/src/domain/javascript/javascriptSemanticResources.ts b/src/domain/javascript/javascriptSemanticResources.ts index d441c70d1c..1e5534d40a 100644 --- a/src/domain/javascript/javascriptSemanticResources.ts +++ b/src/domain/javascript/javascriptSemanticResources.ts @@ -26,7 +26,7 @@ import { traverseDataEffects, type DataEffectTraversalContext, } from "./javascriptSemanticDataEffectHelpers.js"; -import { range } from "./javascriptStaticAnalysisHelpers.js"; +import { FS_OPEN_FLAGS, range } from "./javascriptStaticAnalysisHelpers.js"; const RELEASE_METHODS = ["close", "destroy", "end"] as const; @@ -75,6 +75,90 @@ export const collectJavaScriptSemanticResources = ( ]; }; +/** + * `fs.open`/`fs.openSync` validate the flag before touching the filesystem, so + * a flag Node rejects at the call site throws synchronously and no descriptor + * exists. Node accepts only a known flag string, a nullish value (which defaults + * to `"r"`), or a bitmask integer; everything else throws. A computed value may + * still be valid, so only an exactly readable literal is decided. + */ +const rejectsOpenFlagLiteral = ( + node: t.CallExpression | t.OptionalCallExpression, + context: DataEffectTraversalContext, +): boolean => { + // A spread before the flag slot shifts the runtime position, so argument 1 is + // not provably the flag. `openSync(...args, "0600")` with + // `args = [path, "w"]` opens with mode `"0600"`, not with an invalid flag. + if (t.isSpreadElement(node.arguments[0])) return false; + const flag = node.arguments[1]; + if (flag === undefined || t.isSpreadElement(flag)) return false; + const value = unwrapJavaScriptExpression(flag).node; + // A no-expression template literal is exactly readable, so it is the same flag + // string Node receives. + const flagText = t.isStringLiteral(value) + ? value.value + : t.isTemplateLiteral(value) && value.expressions.length === 0 + ? value.quasis[0]?.value.cooked + : undefined; + if (flagText !== undefined && flagText !== null) + return !FS_OPEN_FLAGS.has(flagText); + // A non-integer or out-of-int32 value is not a usable bitmask. The sign alone + // does not decide: Node accepts some signed bitmasks such as `-2147483648` and + // rejects others such as `-1`, so an in-range negative is left undecided. + const bits = staticFlagBits(value); + if (bits !== undefined) + return ( + bits !== null && + (!Number.isInteger(bits) || bits > 2_147_483_647 || bits < -2_147_483_648) + ); + // `NaN` and `Infinity` are unshadowed globals, and neither is a bitmask. + if ( + t.isIdentifier(value) && + (value.name === "NaN" || value.name === "Infinity") && + isUnmodifiedGlobalReference(value, context.state, value.name) + ) + return true; + // BigInt, boolean, object and array literals all fail Node's flag check. + return ( + t.isBigIntLiteral(value) || + t.isBooleanLiteral(value) || + t.isObjectExpression(value) || + t.isArrayExpression(value) + ); +}; + +/** + * The number a literal flag argument evaluates to, or `null` when it evaluates to + * `undefined`. Returns `undefined` when the value is not exactly readable. + * + * `void ` is `undefined`, which Node accepts as the default read flag, so + * it must never be judged by its operand. `!`, `typeof` and `delete` yield a + * boolean or a string rather than a bitmask. + */ +const staticFlagBits = (value: t.Node): number | null | undefined => { + if (t.isNumericLiteral(value)) return value.value; + if (!t.isUnaryExpression(value)) return undefined; + if (value.operator === "void") return null; + if ( + value.operator === "!" || + value.operator === "typeof" || + value.operator === "delete" + ) + return Number.NaN; + const operand = unwrapJavaScriptExpression(value.argument).node; + if (!t.isNumericLiteral(operand)) return undefined; + if (value.operator === "-") return -operand.value; + if (value.operator === "+") return operand.value; + if (value.operator === "~") return ~operand.value; + return undefined; +}; + +/** `fs.open`, `fs.openSync` and `fs/promises.open` take the flag in slot 1. */ +const openFlagInFirstArgument = (reference: AcquisitionReference): boolean => + reference.method === "open" || reference.method === "openSync" + ? reference.specifier === "fs" || reference.specifier === "fs/promises" + : false; + const collectAcquisitions = ( program: t.Program, context: DataEffectTraversalContext, @@ -84,6 +168,11 @@ const collectAcquisitions = ( if (!t.isCallExpression(node) && !t.isOptionalCallExpression(node)) return; const reference = acquisitionMethod(node.callee, context.state); if (reference === null) return; + if ( + openFlagInFirstArgument(reference) && + rejectsOpenFlagLiteral(node, context) + ) + return; output.push({ node, specifier: reference.specifier, diff --git a/src/domain/javascript/javascriptStaticAnalysisHelpers.ts b/src/domain/javascript/javascriptStaticAnalysisHelpers.ts index 40a2f4166e..209b3eacdd 100644 --- a/src/domain/javascript/javascriptStaticAnalysisHelpers.ts +++ b/src/domain/javascript/javascriptStaticAnalysisHelpers.ts @@ -530,7 +530,7 @@ const WINDOW_OPEN_CALL = /(?:^|\.)(?:window|self|globalThis|top|parent|opener|frames|document)\.open$/u; /** Node `fs.open` flag strings, which are never request URLs. */ -const FS_OPEN_FLAGS = new Set([ +export const FS_OPEN_FLAGS: ReadonlySet = new Set([ "a", "a+", "as",