Repository navigation
Conversation
…int32
The `inputFlags`, `outputFlags`, `localFlags` and `controlFlags` setters of
`Bun.Terminal` coerced the assigned value with ToNumber and clamped it with
`f64::min`, which returns the operand that is not NaN. So `undefined`,
`NaN`, `"bogus"` and `{}` all wrote `tcflag_t::MAX` (every bit) to the PTY.
The setters now validate the value with `validate_uint32`. A value that is
not a number throws a TypeError (ERR_INVALID_ARG_TYPE). NaN, an infinity, a
fraction, a negative number and a number above 0xffffffff throw a
RangeError (ERR_OUT_OF_RANGE). The PTY is not touched in those cases.
An assignment on a closed terminal, and on Windows, stays a no-op.
|
Status: the fix is complete and waits for a maintainer. The diff is green on every platform. CI is red only on tests that this PR does not touch. How to reproduce (bun 1.4.3-canary and const t = new Bun.Terminal({ cols: 80, rows: 24, data() {} });
const before = t.localFlags; // 35387 (0x8a3b)
for (const v of [undefined, NaN, "bogus", {}]) {
t.localFlags = before;
t.localFlags = v;
console.log(String(v), t.localFlags); // 4294967295 (0xffffffff) every time
}
t.close();The same happens for Tests: CI:
Needs from a maintainer: a review, and an answer to the question in the PR notes: keep strict uint32, or accept a negative int32 as its bit pattern. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughTerminal flag setters now validate unsigned 32-bit inputs. Documentation and tests describe behavior for invalid values, accepted boundaries, closed terminals, Windows, and object inputs. ChangesTerminal flag validation
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The terminal flag validation changes have no remaining identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and found no bugs. Because it changes user-facing setter semantics (previously-coerced values now throw), the author has an open strict-vs-lenient question for a maintainer, and packages/bun-types/ has a CODEOWNER, a human sign-off is still worthwhile.
What was reviewed:
set_termios_flag: closed/Windows short-circuit still precedes validation;validate_uint32never runs user JS, so the oldvalueOf-during-coercion reentrancy path is gone;u32 as libc::tcflag_twidens on macOS and is identity on Linux.- Tests:
test.eachmatrix asserts error class +code+ property name (not full message),>>> 0boundary and 0/0xffffffff accepted,toPrimitivemock proves no coercion; the closed-terminal no-op test is meaningful on POSIX and vacuously true on Windows (setter short-circuits before validation there anyway). - JSDoc: matches the new runtime contract; no
*/hazards.
Extended reasoning...
Overview
The PR replaces the f64 coerce-and-clamp in Terminal::set_termios_flag (src/runtime/api/bun/Terminal.rs) with a call to the shared crate::node::validators::validate_uint32, so assigning a non-number to inputFlags/outputFlags/localFlags/controlFlags throws ERR_INVALID_ARG_TYPE and a non-uint32 number throws ERR_OUT_OF_RANGE. It updates the JSDoc for those four properties in packages/bun-types/bun.d.ts and replaces the old "NaN clamps to max" test in test/js/bun/terminal/terminal.test.ts with a describe.each-style matrix covering type errors, range boundaries, the >>> 0 bit-31 case, no-coercion, and closed-terminal no-op.
Security risks
None. The change tightens input validation on a local PTY attribute setter and removes a path that ran user JS (coerce_f64 → valueOf/Symbol.toPrimitive) while holding terminal state; validate_uint32 checks is_number() before as_number() and never enters user code. No auth, crypto, network, or filesystem surface is touched.
Level of scrutiny
Medium. The native diff is small and mechanical (swap hand-rolled clamp for the in-tree validator), and the test coverage is thorough. However, this is a user-facing behavioral change on a Bun.* API: values that previously coerced silently (e.g. "8", true, or a negative int32 from flags & ~ECHO when bit 31 is set) now throw. The PR description explicitly asks a maintainer to choose between strict uint32 and accepting negative int32 bit patterns — that is an API-design call a human should make. Additionally, packages/bun-types/ is CODEOWNER-gated.
Other factors
The implementation follows REVIEW.md guidance: it uses the shared validator instead of hand-rolled range logic, the tests assert error code and class rather than pinning message text (which other open PRs are rewriting), boundaries are tested at and one-past, and the deleted test is fully superseded by the new suite. The u32 as libc::tcflag_t cast is safe on all supported targets (u32 on Linux/BSD, u64 on macOS — widening). The closed-terminal no-op test is not skipIf(isWindows)-gated; on Windows it passes for a different reason (the setter is unconditionally a no-op there), which is harmless but means it only guards the ordering invariant on POSIX. The PR also notes a textual conflict with open PR #39162 in the same function that will need manual resolution.
|
Updated 6:05 AM PT - Sep 13th, 2026
❌ @robobun, your commit fc35767 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 42589That installs a local version of the PR into your bun-42589 --bun |
Problem
undefined,NaN,"bogus"or{}toinputFlags,outputFlags,localFlagsorcontrolFlagsof an openBun.Terminalwritestcflag_t::MAX(every bit) to the PTY and throws nothing.set_termios_flag(src/runtime/api/bun/Terminal.rs:1338). It coerces with ToNumber, then clamps withnum.min(max_val).max(0.0).f64::minreturns the operand that is not NaN, so NaN becomes the maximum. Fix Bun.Terminal termios flag NaN clamping #32354 chose that order to match the Zig code.0. JS bitwise operators return signed 32-bit numbers, soflags & ~ECHOclears every flag when bit 31 is set.Fix
validate_uint32and does not coerce. A value that is not a number throws aTypeError(ERR_INVALID_ARG_TYPE). NaN, an infinity, a fraction, a negative number and a number above0xffffffffthrow aRangeError(ERR_OUT_OF_RANGE), before any syscall.test/js/bun/terminal/terminal.test.ts(13 new tests, 12 fail on 1.4.3-canary), and all oftest/js/bun/terminal/.Background
tcflag_t: 32 bits everywhere except macOS (64 bits).tcgetattr, replaces one word, and writes it back withtcsetattr.validate_uint32(src/runtime/node/util/validators.rs:169) ports Node'svalidateUint32. It never runs user JS.Notes
Question for a maintainer: strict uint32, or accept a negative int32 as its bit pattern?
This PR is strict.
t.localFlags = t.localFlags & ~ECHOthrowsERR_OUT_OF_RANGEwhen bit 31 is set (CRTSCTSinc_cflagon Linux,NOFLSHinc_lflagon macOS and the BSDs), because the expression is negative. The caller must write(t.localFlags & ~ECHO) >>> 0. The new JSDoc says so, and a test pins it.Buffer.writeUInt32LE(1 << 31),zlib.crc32("x", -1),fs.chmodSync(p, ~0o22)andprocess.umask(-1)all throwERR_OUT_OF_RANGEfor a negative int32, with the same message. The released build already breaks this idiom: it writes0, which clears every flag without an error. A later change from strict to lenient does not break anyone. The reverse does.-2147483648..4294967295, take a negative value as its unsigned 32-bit pattern): the read-modify-write idiom then works with no>>> 0, as it does in C. It isvalidate_integerwith a different range plus one cast.Say which one you want and I will switch it.
Order with #39162. #39162 is open and edits the same function. It makes
tcgetattrandtcsetattrfailures throw. The two PRs conflict inTerminal.rsand in the test file. If this PR lands first, #39162 keeps its syscall error throws and drops three things that no longer apply: thecoerce_f64call with the closed check moved after it, the testsetting flags on a terminal closed while coercing the value is a no-op(an object is now aTypeError,valueOf()is never called), and its edit of the NaN test.Replaced test.
NaN clamps to tcflag_t max, same as Infinitycame from #32354 and pinned the behaviour this PR removes. The 13 new tests replace it.JSDoc. The four properties said "Setting returns true on success, false on failure". A setter cannot return a value, so that sentence is gone. The new text gives the range, the two error types, the no-op cases, and the
>>> 0rule on the two properties where a defined flag uses bit 31. #39162 rewrites the same sentence, so that hunk needs a manual merge too.Why
validate_uint32and notglobal.validate_integer_range.validate_integer_rangemapsundefinedand NaN to a default, so a setter needs two checks in front of it, and it reports a fraction asmust be of type integer. Received number.validate_uint32covers every case in one call and gives the same messages asfs.chmodSyncandprocess.umaskfor the same kind of value. The tests pin the error class, the code, and that the message names the property. They do not pin the full text, because #38663, #35423 and #35429 rewrite theERR_INVALID_ARG_TYPEwording of the shared validators.Before and after, open terminal on Linux x64 (
localFlags, initial0x8a3b).undefined,"bogus",{}0xffffffffTypeErrorERR_INVALID_ARG_TYPEnull,[]0TypeErrorERR_INVALID_ARG_TYPE"8",true8,1TypeErrorERR_INVALID_ARG_TYPE{ valueOf() {...} }valueOf()TypeError,valueOf()not calledNaN,Infinity,2**320xffffffffRangeErrorERR_OUT_OF_RANGE-Infinity,-10RangeErrorERR_OUT_OF_RANGE1.51RangeErrorERR_OUT_OF_RANGE0to0xffffffff(integers)Other checks.
cargo clippy -p bun_runtime --no-depsandcargo check -p bun_runtimeare clean forx86_64-unknown-linux-gnuandaarch64-apple-darwin(the cast totcflag_twidens on macOS). macOS can hold flag bits above 31, but no flag is defined there and JS bitwise operators cannot produce them.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/terminal/terminal.test.ts