Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 114 additions & 4 deletions src/runtime/server/RequestContext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1186,6 +1186,8 @@
pub fn end_already_responded_stream(&mut self) {
ctx_log!("endAlreadyRespondedStream");
debug_assert!(!HTTP3);
// Resume before `take()`: READABLE was disabled while paused, so the socket has not been recycled.
self.resume_request_body_socket();

Check failure on line 1190 in src/runtime/server/RequestContext.rs

View check run for this annotation

Claude / Claude Code Review

resume_request_body_socket() in end_already_responded_stream may dereference a freed uWS socket

The fix in adf72bd added `resume_request_body_socket()` to `end_already_responded_stream()`, but this function's own doc comment (lines 1170-1178) states that `self.resp` may point to a socket uSockets has **already freed** via `us_internal_free_closed_sockets` and must be released *without dereferencing it*. When `REQUEST_BODY_PAUSED` is set, `resp.resume_()` → `us_socket_resume(s)` reads `s->fin_deferred` (socket.c:852, libuv) / `s->flags.is_paused` (:857) on freed memory, and `HttpResponse::r
Comment thread
robobun marked this conversation as resolved.
Outdated
if self.resp.take().is_some() {
self.flags.set_is_waiting_for_request_body(false);
self.flags.set_has_abort_handler(false);
Expand Down Expand Up @@ -1506,6 +1508,7 @@
}
self.response_weakref.deref();

self.clear_request_body_stream_drain_handler(global_this);
self.request_body_readable_stream_ref.deinit();

// Releases the ref taken in `set_cookies` (via `CookieMapRef::drop`).
Expand Down Expand Up @@ -2374,6 +2377,10 @@
self.request_body_buf = Vec::new();

if let Some(resp) = self.resp.take() {
if self.flags.request_body_paused() {
self.flags.set_request_body_paused(false);
resp.resume_();
}
Comment thread
robobun marked this conversation as resolved.
if self.flags.is_waiting_for_request_body() {
self.flags.set_is_waiting_for_request_body(false);
resp.clear_on_data();
Expand Down Expand Up @@ -3901,6 +3908,7 @@
if this.request_body_streamed_len > server.config().max_request_body_size {
this.resp.expect("infallible: resp bound").clear_on_data();
this.flags.set_is_waiting_for_request_body(false);
this.resume_request_body_socket();

let _exit = vm.enter_event_loop_scope();

Expand All @@ -3911,6 +3919,9 @@

readable.value.ensure_still_alive();
if let Some(bytes) = readable.ptr.bytes() {
let source = bytes.parent_const();
source.drain_handler.set(None);
source.drain_ctx.set(None);
let mut err = Body::ValueError::Message(BunString::static_(
"Request body exceeded maxRequestBodySize",
));
Expand Down Expand Up @@ -3951,7 +3962,17 @@
);
// TODO: properly propagate exception upwards
let _ = bytes.on_data(WebCore::streams::Result::Temporary(borrowed));

// What `on_data` buffered; `on_stream_drained` resumes once it empties.
let buffered = bytes.buffer.get().len().saturating_sub(bytes.offset.get());
if bytes.buffer_action.get().is_some() || bytes.pipe.get().ctx.is_some() {
// `.text()`-after-`.body` / native pipe want it all; no `on_pull` will fire.
this.resume_request_body_socket();
} else if buffered >= REQUEST_BODY_HIGH_WATER_MARK {
this.pause_request_body_socket();
}
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
} else {
this.resume_request_body_socket();
// Moved out so the Strong (and its underlying GC handle) is
// released at scope exit via `Drop` on `strong::Optional`.
let _strong = core::mem::take(&mut this.request_body_readable_stream_ref);
Expand All @@ -3966,6 +3987,9 @@
let bytes = bun_ptr::BackRef::from(
NonNull::new(bytes_ptr).expect("Source::Bytes payload is non-null"),
);
let source = bytes.parent_const();
source.drain_handler.set(None);
source.drain_ctx.set(None);
// TODO: properly propagate exception upwards
let _ = bytes.on_data(WebCore::streams::Result::TemporaryAndDone(borrowed));
}
Expand Down Expand Up @@ -4055,6 +4079,72 @@
);
}
this.request_body_buf.extend_from_slice(chunk);

// Pre-stream backpressure; resumed by `on_stream_drained` / `on_start_buffering`.
if !this.flags.request_body_buffer_all()
&& this.request_body_buf.len() >= REQUEST_BODY_HIGH_WATER_MARK
{
this.pause_request_body_socket();
}

Check failure on line 4088 in src/runtime/server/RequestContext.rs

View check run for this annotation

Claude / Claude Code Review

Bun.write(file, req) in a Bun.serve handler hangs for request bodies > 1 MiB

`await Bun.write(file, req)` in a `Bun.serve` handler now hangs for any body > 1 MiB. That path (Blob.rs:5232-5250, `BodyValue::Locked` → `WriteFileWaitFromLockedValueTask`) sets `locked.on_receive_value` and awaits the last chunk without calling `on_start_buffering` or creating a ByteStream, so `REQUEST_BODY_BUFFER_ALL` is never set, the pre-stream HWM check pauses the socket at 1 MiB, and nothing ever resumes it — same bug class as the `buffer_action` deadlock adf72bd fixed, on the `on_receive
Comment thread
robobun marked this conversation as resolved.
}
}

fn pause_request_body_socket(&mut self) {
if self.flags.request_body_paused() {
return;
}
let Some(resp) = self.resp else {
return;
};
ctx_log!("pauseRequestBodySocket");
self.flags.set_request_body_paused(true);
resp.pause();
}
Comment thread
robobun marked this conversation as resolved.

fn resume_request_body_socket(&mut self) {
if !self.flags.request_body_paused() {
return;
}
ctx_log!("resumeRequestBodySocket");
self.flags.set_request_body_paused(false);
if let Some(resp) = self.resp {
resp.resume_();
}
}

/// Detach the body ByteStream's `drain_handler` (the stream can outlive this ctx in JS).
fn clear_request_body_stream_drain_handler(&self, global_this: &JSGlobalObject) {
let Some(readable) = self.request_body_readable_stream_ref.get(global_this) else {
return;
};
if let Some(bytes) = readable.ptr.bytes() {
let source = bytes.parent_const();
source.drain_handler.set(None);
source.drain_ctx.set(None);
}
}

/// # Safety
/// `ctx` must be a `*mut RequestContext` previously registered as the body
/// `on_stream_drained` context.
Comment thread
robobun marked this conversation as resolved.
pub(crate) fn on_request_body_stream_drained_callback(ctx: Option<*mut c_void>) {
let Some(ctx) = ctx else { return };
let this = ctx.cast::<Self>();
// SAFETY: `ctx` is the registered `*mut RequestContext`. `ByteStream::
// on_data` can re-enter here while `on_buffered_body_chunk` already
// holds `&mut Self` (borrow = ptr), so dispatch via the raw pointer.
unsafe {
let flags = &raw mut (*this).flags;
if !(*flags).request_body_paused() {
return;
}
if (*this).resp.is_none() || (*flags).aborted() {
return;
}
(*flags).set_request_body_paused(false);
if let Some(resp) = (*this).resp {
resp.resume_();
}
}
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

Expand Down Expand Up @@ -4089,6 +4179,9 @@
pub fn on_start_buffering(&mut self) {
if let Some(server) = self.server {
ctx_log!("onStartBuffering");
// `.text()`/`.json()` want the whole body; disable pre-stream backpressure.
self.flags.set_request_body_buffer_all(true);
self.resume_request_body_socket();
// TODO: check if is someone calling onStartBuffering other than onStartBufferingCallback
// if is not, this should be removed and only keep protect + setAbortHandler
// HTTP/3 (RFC 9114): Content-Length is optional; the body is
Expand Down Expand Up @@ -4183,6 +4276,9 @@

const MAX_REQUEST_BODY_PREALLOCATE_LENGTH: usize = 1024 * 256;

/// Pause socket reads at this many unconsumed request-body bytes (two 512 KB uWS recv buffers).
const REQUEST_BODY_HIGH_WATER_MARK: usize = 1024 * 1024;

// Trap host fn for the `(false, _, true)` arms of `exported_host_fns`. Those
// `RequestContext` monomorphs (plain-HTTP/3) are type-reachable via the
// blanket H3 impls but never serve requests at runtime — HTTP/3 always
Expand Down Expand Up @@ -4390,12 +4486,12 @@
pub total: BlobSizeType,
}

// All flags are bool (with two debug-conditional ones), so `bitflags!` over u16
// works. We keep all bits in every build and just gate the
// `is_web_browser_navigation` / `has_finalized` accessors on the const params.
// All flags are bool (with two debug-conditional ones). We keep all bits in
// every build and just gate the `is_web_browser_navigation` / `has_finalized`
// accessors on the const params.
Comment thread
robobun marked this conversation as resolved.
bitflags::bitflags! {
#[derive(Default, Clone, Copy)]
pub struct FlagsBits: u16 {
pub struct FlagsBits: u32 {
const HAS_MARKED_COMPLETE = 1 << 0;
const HAS_MARKED_PENDING = 1 << 1;
const HAS_ABORT_HANDLER = 1 << 2;
Expand All @@ -4416,6 +4512,10 @@
const ABORTED = 1 << 13;
const HAS_FINALIZED = 1 << 14;
const IS_ERROR_PROMISE_PENDING = 1 << 15;
/// Socket reads are paused because the request-body buffer is over its high-water mark.
const REQUEST_BODY_PAUSED = 1 << 16;
/// `on_start_buffering` fired (`.text()` etc.); skip pre-stream backpressure.
const REQUEST_BODY_BUFFER_ALL = 1 << 17;
}
}

Expand Down Expand Up @@ -4495,6 +4595,16 @@
set_is_error_promise_pending,
IS_ERROR_PROMISE_PENDING
);
flag_accessor!(
request_body_paused,
set_request_body_paused,
REQUEST_BODY_PAUSED
);
flag_accessor!(
request_body_buffer_all,
set_request_body_buffer_all,
REQUEST_BODY_BUFFER_ALL
);

#[inline]
pub fn is_web_browser_navigation(self) -> bool {
Expand Down
3 changes: 3 additions & 0 deletions src/runtime/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -818,6 +818,9 @@ impl<const SSL: bool, const DEBUG: bool> NewServer<SSL, DEBUG> {
on_readable_stream_available: Some(
ServerRequestContext::<SSL, DEBUG>::on_request_body_readable_stream_available,
),
on_stream_drained: Some(
ServerRequestContext::<SSL, DEBUG>::on_request_body_stream_drained_callback,
),
..Default::default()
});
}
Expand Down
6 changes: 6 additions & 0 deletions src/runtime/server/server_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ pub(super) trait RequestCtxOps: RequestCtx {
global_this: &JSGlobalObject,
readable: WebCore::ReadableStream,
);
fn on_request_body_stream_drained_callback(this: Option<*mut c_void>);
}

impl<ThisServer, const SSL: bool, const DBG: bool, const H3: bool> RequestCtxOps
Expand Down Expand Up @@ -275,6 +276,10 @@ where
) {
Self::on_request_body_readable_stream_available(this, global_this, readable)
}
#[inline]
fn on_request_body_stream_drained_callback(this: Option<*mut c_void>) {
Self::on_request_body_stream_drained_callback(this)
}
}

// NOTE: local request/response trait so generic `Ctx::Req` / `Ctx::Resp`
Expand Down Expand Up @@ -3187,6 +3192,7 @@ where
on_readable_stream_available: Some(
Ctx::on_request_body_readable_stream_available,
),
on_stream_drained: Some(Ctx::on_request_body_stream_drained_callback),
..Default::default()
});
}
Expand Down
Loading
Loading