mirror of
https://github.com/morluto/rea.git
synced 2026-11-05 22:36:08 +00:00
fix(javascript): emit no fs acquisition for a literal non-flag string (#8125)
* 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.
This commit is contained in:
1 parent
65311d4992
commit
b668cea2af
3 files changed
+226
-2
No files matched your search
@@ -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 <expr>` 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" },
|
||||
]);
|
||||
});
|
||||
@@ -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 <expr>` 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,
|
||||
|
||||
@@ -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<string> = new Set([
|
||||
"a",
|
||||
"a+",
|
||||
"as",
|
||||
|
||||
Reference in new issue
Block a user