Skip to content

node:http: clamp negative IncomingMessage.setTimeout seconds to 0 - #32308

Closed
robobun wants to merge 3 commits into
mainfrom
farm/3fa3563f/fix-nodehttp-settimeout-clamp
Closed

robobun wants to merge 3 commits into
mainfrom
farm/3fa3563f/fix-nodehttp-settimeout-clamp

Conversation

@robobun

@robobun robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

NodeHTTPResponse__setTimeout (the Rust port of NodeHTTPResponse.zig:1165) computed the seconds value as:

let secs = (seconds.to_int32() as c_uint).min(255) as u8;

For negative inputs this bit-reinterprets the signed i32 as a large unsigned value before clamping, so e.g. -1 becomes 4294967295 which then clamps to 255. The Zig reference routes through JSValue.toU32, which clamps negatives to 0 and saturates at u32::MAX:

this.raw_response.?.timeout(@intCast(@min(seconds.to(c_uint), 255)));

The sibling Request__setTimeout in src/runtime/webcore/Request.rs was already ported correctly with seconds.to_u32() and carries a comment calling out exactly this hazard; this site was missed.

Reachable from node:http via IncomingMessage.prototype.setTimeout(msecs) on a server-side request: _http_incoming.ts calls setRequestTimeout(req, Math.ceil(msecs / 1000)), which dispatches to NodeHTTPResponse__setTimeout when the underlying handle is a NodeHTTPResponse. So req.setTimeout(-1000) silently set a 255-second idle timeout instead of disabling it (0).

Fix

Use seconds.to_u32().min(255), matching both the Zig reference and Request__setTimeout.

Verification

The 0-vs-255 difference is only externally observable after ~255 seconds (uSockets timeout(0) disables the idle timeout; timeout(255) fires after ~256s), so a bun:internal-for-testing hook was added that reads back HttpResponseData::idleTimeout from the underlying uWS response. The new test at test/js/node/http/node-http-req-settimeout-clamp.test.ts asserts the stored value for a matrix of inputs.

With the buggy conversion in place (getter kept, fix reverted):

  {
-   "-1000": 0,
-   "-1000000": 0,
-   "-Infinity": 0,
+   "-1000": 255,
+   "-1000000": 255,
+   "-Infinity": 255,
    "0": 0,
    "1e12": 255,
    "2000": 2,
    ...
  }

With the fix, all cases match the Zig reference semantics.

NodeHTTPResponse__setTimeout computed (to_int32() as c_uint).min(255),
which bit-reinterprets a negative i32 as a large unsigned value before
clamping, so req.setTimeout(-1000) set a 255-second idle timeout instead
of disabling it. The Zig reference uses JSValue.toU32 (clamp negatives
to 0, saturate at u32::MAX) and the sibling Request__setTimeout was
already ported that way; this brings NodeHTTPResponse into line.

Adds a bun:internal-for-testing hook to read back the stored uWS
HttpResponseData::idleTimeout so the clamping can be asserted directly;
the 0-vs-255 difference is otherwise only observable after ~255 seconds.
@coderabbitai

coderabbitai Bot commented Jun 15, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4049b341-9ff8-4c53-9920-801d52a93560

📥 Commits

Reviewing files that changed from the base of the PR and between e0acad3 and e97cb5e.

📒 Files selected for processing (7)
  • src/js/internal-for-testing.ts
  • src/jsc/bindings/NodeHTTP.cpp
  • src/jsc/bindings/NodeHTTP.h
  • src/runtime/server/NodeHTTPResponse.rs
  • src/uws_sys/Response.rs
  • src/uws_sys/libuwsockets.cpp
  • test/js/node/http/node-http-req-settimeout-clamp.test.ts

Walkthrough

Adds a getRequestTimeout read path for Node HTTP responses, threading from a new uws_res_get_timeout C function through Rust AnyResponse/NodeHTTPResponse FFI exports, a JSC host binding jsHTTPGetTimeout, and a JS nodeHttpInternals testing export. Also fixes setTimeout to use ECMAScript ToU32 semantics (negatives clamp to 0).

Changes

Node HTTP request timeout getter and setTimeout clamping fix

Layer / File(s) Summary
uWS C wrapper and Rust FFI for get_timeout
src/uws_sys/libuwsockets.cpp, src/uws_sys/Response.rs
Adds uws_res_get_timeout reading idleTimeout from uWS::HttpResponse, declares the FFI binding in the Rust c module, and exposes get_timeout on Response<SSL> and AnyResponse (returns 0 for H3).
NodeHTTPResponse ToU32 fix and ffi_get_timeout export
src/runtime/server/NodeHTTPResponse.rs
Changes ffi_set_timeout to use seconds.to_u32() so negative millisecond values clamp to 0; removes c_uint import. Adds ffi_get_timeout exported as NodeHTTPResponse__getTimeout returning current timeout as i32 or -1 when the raw response is absent.
JSC host function, JS export, and clamping test
src/jsc/bindings/NodeHTTP.h, src/jsc/bindings/NodeHTTP.cpp, src/js/internal-for-testing.ts, test/js/node/http/node-http-req-settimeout-clamp.test.ts
Declares and implements jsHTTPGetTimeout JSC host function calling NodeHTTPResponse__getTimeout; wires getRequestTimeout and kHandle into the nodeHttpInternals export. Test verifies clamping for negative, zero, and large msecs values passed to IncomingMessage.setTimeout.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: fixing clamping of negative IncomingMessage.setTimeout seconds to 0, which directly matches the primary objective of the PR.
Description check ✅ Passed The description is comprehensive and well-structured, covering the problem statement, the fix, and verification approach. However, it does not explicitly follow the provided template's section headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented Jun 15, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 10:05 AM PT - Jun 15th, 2026

❌ @robobun, your commit 5c5d0ad has some failures in Build #62573 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 32308

That installs a local version of the PR into your bun-32308 executable, so you can run:

bun-32308 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — one-line clamping fix matching the Zig reference, plus mechanical read-only test plumbing.

Extended reasoning...

Overview

The functional change is a single line in src/runtime/server/NodeHTTPResponse.rs: (seconds.to_int32() as c_uint).min(255) → seconds.to_u32().min(255). This fixes negative req.setTimeout(msecs) values wrapping to 255 instead of clamping to 0. The remaining ~50 lines thread a read-only get_timeout accessor through uWS C++ → Rust FFI → JSC host fn → bun:internal-for-testing so the test can observe the stored idleTimeout byte without waiting 255s.

Security risks

None. The new getter only reads a uint8_t field from HttpResponseData and is reachable solely via bun:internal-for-testing (debug/CI only). No new user-facing surface, no untrusted input parsing.

Level of scrutiny

Low-to-medium. The core fix is trivially verifiable: JSValue::to_u32() is documented at src/jsc/JSValue.rs:798 as clamping negatives to 0 and saturating at u32::MAX, exactly matching the Zig reference and the already-correct sibling Request__setTimeout (src/runtime/webcore/Request.rs:411). The test plumbing is mechanical — each new function is a near-copy of its adjacent setter (uws_res_get_timeout mirrors uws_res_timeout, jsHTTPGetTimeout mirrors jsHTTPSetTimeout, etc.) and follows established patterns in every layer.

Other factors

  • Verified idleTimeout is a uint8_t field on HttpResponseData (packages/bun-uws/src/HttpResponseData.h:110), so the C accessor and Rust u8 return type are correct.
  • Verified kHandle is exported from internal/http.
  • The new test covers a matrix of negative/zero/large/infinite inputs and the PR description shows it failing without the fix.
  • robobun reports CI failures on the prior commit, but they're uniform -no-pie linker warnings / generic ci.ts build failures across all platforms — characteristic of base-branch infra noise rather than anything this diff introduced (the autofix commit hasn't reported yet).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — one-line clamping fix matching the sibling Request__setTimeout, with a clean test-only readback path to verify it.

Extended reasoning...

Overview

The functional change is a single line in src/runtime/server/NodeHTTPResponse.rs: (seconds.to_int32() as c_uint).min(255) → seconds.to_u32().min(255), so negative IncomingMessage.setTimeout inputs clamp to 0 instead of wrapping to 255. I confirmed JSValue::to_u32 (src/jsc/JSValue.rs:803) is to_int64().clamp(0, u32::MAX as i64) as u32, and that the sibling Request__setTimeout (src/runtime/webcore/Request.rs:411) already uses the same pattern with a comment calling out exactly this hazard. The remaining ~50 lines thread a read-only idleTimeout getter from uWS (uws_res_get_timeout reading HttpResponseData::idleTimeout, a uint8_t) up through Rust FFI, a JSC host function, and a bun:internal-for-testing export so the new test can assert the stored value without waiting ~255s.

Security risks

None. The fix narrows behavior (negatives now disable the timeout instead of setting 255s). The new getter is read-only, returns a u8 widened to i32, and is only reachable via bun:internal-for-testing which is gated to debug builds / Bun CI.

Level of scrutiny

Low–medium. The core fix is mechanical and mirrors an already-correct sibling; the test plumbing follows the established uws_res_* shim pattern (compare adjacent uws_res_timeout/uws_res_reset_timeout) and the `` / internal-for-testing conventions used throughout the file. None of the touched paths match CODEOWNERS.

Other factors

The added test covers negatives, zero, in-range, boundary (254/255/256), and huge inputs, and the PR description shows it failing on the old code and passing on the new. No outstanding reviewer comments and no bugs were flagged by the bug-hunting pass.

@robobun

robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the diff is green.

Across both builds (#62443 and the retrigger #62573), 280+ lanes passed including the new node-http-req-settimeout-clamp.test.ts on every platform that ran. The remaining red is unrelated to this change:

  • darwin 14/26 test lanes: repeatedly expiring while waiting for an agent, then auto-retrying (macOS runner availability, not test failures)
  • build #62443 hard fail: test/js/web/fetch/fetch-leak.test.ts RSS-threshold flake on darwin-14-aarch64 only (off by ~4MB)
  • flaky bucket (auto-classified, passed on retry): streams-leak, serve-body-leak, s3.leak, rm.test, bun-run-dir, 30205 (napi), dev-and-prod (bake HMR), update_interactive_install, sql-postgres-datetime-roundtrip, websocket-server, node-http-connect

None of these touch NodeHTTPResponse__setTimeout, the uWS timeout getter, or node:http IncomingMessage.setTimeout. Ready for review/merge.

@robobun

robobun commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Stale PR review: closing.

This PR has had no human activity since 2026-06-15 and conflicts with main. The function it fixes no longer exists: #32488 (merged 2026-07-16) changed IncomingMessage.prototype.setTimeout to call this.socket?.setTimeout(msecs) (src/js/node/_http_incoming.ts:343-347), and #39249 (merged 2026-08-16) then removed NodeHTTPResponse__setTimeout and setRequestTimeout as dead code. A grep of src/ on main finds neither name.

On current main, req.setTimeout(-1000) on a server request throws through validateMsecs (src/js/internal/http.ts:189-191) before any native code runs, as Node does (Node throws ERR_OUT_OF_RANGE, Bun ERR_INVALID_ARG_TYPE). A rebase would keep only the test-only getRequestTimeout read-back chain and a test whose negative rows assert a silent clamp to 0, which neither Node nor main does. The remaining caller of NodeHTTPResponse::set_timeout (src/runtime/server/server_body.rs:1566-1574) already converts with to_u32().

Reopen if this evidence is wrong.

@robobun robobun closed this Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant