diff --git a/src/Arena.zig b/src/Arena.zig index a72327aa9..96b288025 100644 --- a/src/Arena.zig +++ b/src/Arena.zig @@ -48,6 +48,10 @@ bucket: *ArenaPool.Bucket, // Only meaningful while this arena sits in its bucket's free list. next: ?*Arena, +// Used to detect a double-release. Allows us to fail when it happens rather +// than triggering a double-free which will fail in a seemingly unrelated way +released: bool, + // Bytes _arena holds from the backing allocator, maintained by the vtable // below. Changes only when the arena grows or frees a node, so this costs // O(log n) updates over an arena's life, not one per allocation. diff --git a/src/ArenaPool.zig b/src/ArenaPool.zig index 784b7e8a9..605abaee3 100644 --- a/src/ArenaPool.zig +++ b/src/ArenaPool.zig @@ -146,6 +146,7 @@ fn _acquire(self: *ArenaPool, account: ?*Arena.Account, size_or_bucket: anytype, if (bucket.free_list) |entry| { bucket.free_list = entry.next; bucket.free_list_len -= 1; + entry.released = false; if (lp.IS_DEBUG) { entry.debug = debug; const gop = try self._leak_track.getOrPut(self.allocator, debug); @@ -164,6 +165,7 @@ fn _acquire(self: *ArenaPool, account: ?*Arena.Account, size_or_bucket: anytype, const entry = try self.entry_pool.create(self.allocator); entry.* = .{ .next = null, + .released = false, .pool = self, .bucket = bucket, .bytes = 0, @@ -191,23 +193,35 @@ pub fn release(self: *ArenaPool, entry: *Arena) void { const arena = &entry._arena; const bucket = entry.bucket; - lp.metrics.arena_inflight.decr(bucket.size); - - if (lp.IS_DEBUG) { + { self.mutex.lockUncancelable(lp.io); defer self.mutex.unlock(lp.io); - if (self._leak_track.getPtr(entry.debug)) |count| { - count.* -= 1; - if (count.* < 0) { - log.err(.bug, "ArenaPool double-free", .{ .name = entry.debug }); - @panic("ArenaPool: double-free detected"); + + if (entry.released) { + // This arena was already released. It's better to crash here + // because it [hopefully] gives us the stack that re-released, else + // it'll crash in some random code. + lp.assert(false, "ArenaPool double release", .{ + .bucket = @tagName(bucket.size), + .name = if (comptime lp.IS_DEBUG) entry.debug else "", + }); + } + entry.released = true; + + if (comptime lp.IS_DEBUG) { + if (self._leak_track.getPtr(entry.debug)) |count| { + // Can't go negative: the released check above already caught + // a double release of this entry. + count.* -= 1; + } else { + log.err(.bug, "ArenaPool release unknown", .{ .name = entry.debug }); + @panic("ArenaPool: release of untracked arena"); } - } else { - log.err(.bug, "ArenaPool release unknown", .{ .name = entry.debug }); - @panic("ArenaPool: release of untracked arena"); } } + lp.metrics.arena_inflight.decr(bucket.size); + entry.unpin(); _ = arena.reset(.{ .retain_with_limit = bucket.retain_bytes }); diff --git a/src/browser/ScriptManager.zig b/src/browser/ScriptManager.zig index 7d1dcce61..e9cd8b6aa 100644 --- a/src/browser/ScriptManager.zig +++ b/src/browser/ScriptManager.zig @@ -392,27 +392,38 @@ pub fn addFromElement(self: *ScriptManager, comptime from_parser: bool, script_e self.base.is_evaluating = true; defer self.base.endEvaluationWindow(was_evaluating); - errdefer self.base.scriptList(script).remove(&script.node); - try frame.makeRequest(.{ - .ctx = script, - .url = remote_url, - .method = .GET, - .frame_id = frame._frame_id, - .loader_id = frame._loader_id, - .cookie_jar = &frame._session.cookie_jar, - .cookie_origin = frame.url, - .resource_type = .script, - .notification = frame._session.notification, - .start_callback = if (log.enabled(.http, .debug)) Script.startCallback else null, - .header_callback = Script.headerCallback, - .data_callback = Script.dataCallback, - .done_callback = Script.doneCallback, - .error_callback = Script.errorCallback, - // Nothing holds the transfer; teardown cleanup runs through - // the manager's script lists. - .shutdown_callback = HttpClient.noopShutdown, - }); + const transfer = blk: { + errdefer self.base.scriptList(script).remove(&script.node); + const transfer = try frame.newRequest(.{ + .ctx = script, + .url = remote_url, + .method = .GET, + .frame_id = frame._frame_id, + .loader_id = frame._loader_id, + .cookie_jar = &frame._session.cookie_jar, + .cookie_origin = frame.url, + .resource_type = .script, + .notification = frame._session.notification, + .start_callback = if (log.enabled(.http, .debug)) Script.startCallback else null, + .header_callback = Script.headerCallback, + .data_callback = Script.dataCallback, + .done_callback = Script.doneCallback, + .error_callback = Script.errorCallback, + // Nothing holds the transfer; teardown cleanup runs through + // the manager's script lists. + .shutdown_callback = HttpClient.noopShutdown, + }); + errdefer transfer.deinit(); + try frame.headersForRequest(transfer); + break :blk transfer; + }; + + // Point of no return: submit() consumes the transfer, and on a synchronous + // failure fires Script.errorCallback, which removes the node and deinits + // the script (freeing our arena). Its error is already delivered there + // (same as Fetch), so there's nothing left for us to unwind. handover = true; + transfer.submit() catch {}; } // A + + + diff --git a/src/browser/webapi/net/WebSocket.zig b/src/browser/webapi/net/WebSocket.zig index 4c76aafdf..ad953a04f 100644 --- a/src/browser/webapi/net/WebSocket.zig +++ b/src/browser/webapi/net/WebSocket.zig @@ -522,7 +522,12 @@ fn queueMessage(self: *WebSocket, msg: Message) !void { if (was_empty) { // Unpause the send callback so libcurl will request data if (self._conn) |conn| { - try conn.pause(.{ .cont = true }); + conn.pause(.{ .cont = true }) catch |err| { + // our caller is doing `errdefer errdefer arena.release();` which + // will free msg. So we have to pop it out. + _ = self._send_queue.pop(); + return err; + }; } } } @@ -1054,3 +1059,32 @@ test "WebApi: WebSocket" { test "WebApi: WebSocket in worker" { try testing.htmlRunner("net/websocket_worker.html", .{}); } + +// Production crash (release overflow on unrelated pooled objects): send() +// released the message arena on a failed unpause while the message stayed in +// _send_queue, which released it again later — a pooled-arena double release. +test "WebApi: WebSocket send owns its message arena once when the unpause fails" { + const frame = try testing.createFrame(); + defer testing.test_session.closeAllPages(); + + var ls: js.Local.Scope = undefined; + frame.js.localScope(&ls); + defer ls.deinit(); + + var protocols: [0][]const u8 = .{}; + const ws = try WebSocket.init("ws://127.0.0.1:9582/ws", &protocols, &frame.js.execution); + try testing.expect(ws._conn != null); + + // connect() tracked the easy handle but no tick has performed it, so + // libcurl has no connection behind it and curl_easy_pause fails: the + // same state as a send() on a socket the peer already closed, before + // the close has been dispatched. + ws._ready_state = .open; + + const message = try ls.local.exec("'hello'", null); + try testing.expectError(error.BadFunctionArgument, ws.send(.{ .js_val = message })); + + // The queued message owns the arena. A failed send must not leave it + // queued with its arena already released. + try testing.expectEqual(0, ws._send_queue.items.len); +} diff --git a/src/browser/webapi/net/XMLHttpRequest.zig b/src/browser/webapi/net/XMLHttpRequest.zig index b4aa23a43..1804552fd 100644 --- a/src/browser/webapi/net/XMLHttpRequest.zig +++ b/src/browser/webapi/net/XMLHttpRequest.zig @@ -601,11 +601,9 @@ fn httpDoneCallback(ctx: *anyopaque) !void { fn httpErrorCallback(ctx: *anyopaque, err: anyerror) void { const self: *XMLHttpRequest = @ptrCast(@alignCast(ctx)); - // http client will close it after an error, it isn't safe to keep around + // handleError can execute JS, which could .send() again: clear this now. + self._http_transfer = null; self.handleError(err); - if (self._http_transfer != null) { - self._http_transfer = null; - } self.releaseSelfRef(); } diff --git a/src/network/HttpClient.zig b/src/network/HttpClient.zig index f1a50d102..04315ad0b 100644 --- a/src/network/HttpClient.zig +++ b/src/network/HttpClient.zig @@ -140,6 +140,11 @@ use_proxy: bool, // Current TLS verification state, applied per-connection in makeRequest. tls_verify: bool = true, +// Test-only fault injection: makes the next submit() fail synchronously from +// inside the pipeline, the shape where error_callback fires AND the error is +// returned to the caller (see Transfer.submit). +test_fail_submit: if (lp.IS_TEST) ?anyerror else void = if (lp.IS_TEST) null else {}, + // User agent override set via CDP Emulation.setUserAgentOverride. // When set, takes precedence over the config's http_headers value. // Allocated from self.allocator when set, null otherwise. @@ -914,6 +919,12 @@ const SubmitFrom = enum { start, after_intercept, network }; fn pipeline(self: *Client, transfer: *Transfer, from: SubmitFrom) !void { sw: switch (from) { .start => { + if (comptime lp.IS_TEST) { + if (self.test_fail_submit) |err| { + return err; + } + } + if (self.network.web_bot_auth) |wba| { const authority = URL.getHost(transfer.req.url); try wba.signRequest(transfer, authority); @@ -3672,6 +3683,7 @@ fn initTestClient(client: *Client, pool: *ArenaPool) void { .single_flight = .init(testing.allocator), }; client.url_blocklist = null; + client.test_fail_submit = null; // isUrlBlocked reaches through here for the adblocker; tests that want // one assign it to `client.network` after this returns. test_network.adblocker = null; diff --git a/src/testing.zig b/src/testing.zig index d3e0d8aff..cb5399bd9 100644 --- a/src/testing.zig +++ b/src/testing.zig @@ -635,6 +635,16 @@ fn testHTTPHandler(req: *std.http.Server.Request) !void { }); } + if (std.mem.eql(u8, path, "/xhr/slow")) { + // Long enough for a timer scheduled by the requester to fire first. + lp.io.sleep(.fromMilliseconds(100), .awake) catch {}; + return req.respond("slow", .{ + .extra_headers = &.{ + .{ .name = "Content-Type", .value = "text/plain" }, + }, + }); + } + if (std.mem.eql(u8, path, "/xhr_empty")) { return req.respond("", .{ .extra_headers = &.{