Clone data into the transfer's arena

There can be cases where the data referenced by Request does not live for the
duration of a Transfer. The most obvious case is a QueuedNavigation..this is
generally a problem, but it's particularly problematic under load when the
request gets queued in the HttpClient.

Most of the data is small, so just _always_ duping it is worth it. The body
can be large, so we provide a flag for callers to set to turn off the body
duping (essentially saying "I promise to keep the body alive for the duration
of the transfer). Currently, only XHR is able to make this guarantee.
This commit is contained in:
Karl Seguin
2026-06-01 21:20:44 +08:00
parent 9712e4171e
commit 8a76153cbb
2 changed files with 48 additions and 17 deletions

View File

@@ -524,24 +524,50 @@ fn requestT(self: *Client, req: Request, owner: ?*Owner) !*Transfer {
return err;
};
const transfer = arena.create(Transfer) catch |err| {
req.headers.deinit();
self.arena_pool.release(arena);
return err;
};
const transfer = blk: {
errdefer {
req.headers.deinit();
self.arena_pool.release(arena);
}
transfer.* = .{
.req = req,
.client = self,
.arena = arena,
.id = self.incrReqId(),
.start_time = timestamp(.monotonic),
// owner is set AFTER we've actually appended to the owner list,
// so transfer.deinit's `if (self.owner)` branch only fires when
// we're truly linked. Otherwise we'd try to remove a node from
// a list it was never in.
.owner = null,
.owner_node = .{},
var owned = req;
// Most of the time, the req data will outlive the transfer. But not
// always. The most problematic case is with a QueuedNavigation which
// is freed quite quickly and would definetly not survive a queued
// request.
//
// These are all small, so duping them into the transfer's arena is
// cheap and can solve some nasty UAF.
owned.url = try arena.dupeZ(u8, req.url);
owned.cookie_origin = try arena.dupeZ(u8, req.cookie_origin);
if (req.credentials) |c| {
owned.credentials = try arena.dupeZ(u8, c);
}
// The body can be larger, so callers can signal, via the
// `body_outlives_request` flag that they guarantee that the body
// will outlive the transfer (and thus doesn't need to be duped)
if (req.body) |b| {
if (req.body_outlives_request == false) {
owned.body = try arena.dupe(u8, b);
}
}
const t = try arena.create(Transfer);
t.* = .{
.req = owned,
.client = self,
.arena = arena,
.id = self.incrReqId(),
.start_time = timestamp(.monotonic),
// owner is set AFTER we've actually appended to the owner list,
// so transfer.deinit's `if (self.owner)` branch only fires when
// we're truly linked. Otherwise we'd try to remove a node from
// a list it was never in.
.owner = null,
.owner_node = .{},
};
break :blk t;
};
// From here, transfer owns req+arena. Any subsequent failure flows
@@ -1229,6 +1255,10 @@ pub const Request = struct {
timeout_ms: u32 = 0,
skip_robots: bool = false,
// When false, the caller does not guarantee that the body outlives the
// transfer, and thus we'll need to dupe it.
body_outlives_request: bool = false,
// arbitrary data that can be associated with this request
ctx: *anyopaque = undefined,

View File

@@ -283,6 +283,7 @@ pub fn send(self: *XMLHttpRequest, body_: ?BodyInit, exec_: *const Execution) !v
.done_callback = httpDoneCallback,
.error_callback = httpErrorCallback,
.shutdown_callback = httpShutdownCallback,
.body_outlives_request = true,
}) catch |err| {
self.releaseSelfRef();
return err;