From 523efbd85a4f13885c5c698d635d4184ad034e9f Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Tue, 3 Mar 2026 18:02:06 +0800 Subject: [PATCH] Fix a few issues in Client Most significantly, if removing from the multi fails, the connection is added to a "dirty" list for the removal to be retried later. Looking at the curl source code, remove fails on a recursive call, and we've struggled with recursive calls before, so I _think_ this might be happening (it fails in other cases, but I suspect if it _is_ happening, it's for this reason). The retry happens _after_ `perform`, so it cannot fail for due to recursiveness. If it fails at this point, we @panic. This is harsh, but it isn't easily recoverable and before putting effort into it, I'd like to know that it's actually happening. Fix potential use of undefined when a 401-407 request is received, but no 'WWW-Authenticate' or 'Proxy-Authenticate' header is received. Don't call `curl_multi_remove_handle` on an easy that hasn't been added yet do to error. Specifically, if `makeRequest` fails during setup, transfer_conn is nulled so that `transfer.deinit()` doesn't try to remove the connection. And the conn is removed from the `in_use` queue and made `available` again. On Abort, if getting the private fails (extremely unlikely), we now still try to remove the connection from the multi. Added a few more fields to the famous "ScriptManager.Header recall" assertion. --- src/Net.zig | 55 +++++++++++++++++++++++++---------- src/browser/ScriptManager.zig | 8 +++++ src/cdp/domains/fetch.zig | 6 ++-- src/http/Client.zig | 39 +++++++++++++++---------- 4 files changed, 74 insertions(+), 34 deletions(-) diff --git a/src/Net.zig b/src/Net.zig index 2c27dabd..bd723be6 100644 --- a/src/Net.zig +++ b/src/Net.zig @@ -174,16 +174,16 @@ const HeaderValue = struct { pub const AuthChallenge = struct { status: u16, - source: enum { server, proxy }, - scheme: enum { basic, digest }, - realm: []const u8, + source: ?enum { server, proxy }, + scheme: ?enum { basic, digest }, + realm: ?[]const u8, pub fn parse(status: u16, header: []const u8) !AuthChallenge { var ac: AuthChallenge = .{ .status = status, - .source = undefined, - .realm = "TODO", // TODO parser and set realm - .scheme = undefined, + .source = null, + .realm = null, + .scheme = null, }; const sep = std.mem.indexOfPos(u8, header, 0, ": ") orelse return error.InvalidHeader; @@ -471,6 +471,7 @@ pub const Connection = struct { pub const Handles = struct { connections: []Connection, + dirty: HandleList, in_use: HandleList, available: HandleList, multi: *libcurl.CurlM, @@ -501,6 +502,7 @@ pub const Handles = struct { } return .{ + .dirty = .{}, .in_use = .{}, .connections = connections, .available = available, @@ -522,8 +524,6 @@ pub const Handles = struct { pub fn get(self: *Handles) ?*Connection { if (self.available.popFirst()) |node| { - node.prev = null; - node.next = null; self.in_use.append(node); return @as(*Connection, @fieldParentPtr("node", node)); } @@ -535,21 +535,46 @@ pub const Handles = struct { } pub fn remove(self: *Handles, conn: *Connection) void { - libcurl.curl_multi_remove_handle(self.multi, conn.easy) catch |err| { - log.fatal(.http, "multi remove handle", .{ .err = err }); - }; - var node = &conn.node; + if (libcurl.curl_multi_remove_handle(self.multi, conn.easy)) { + self.isAvailable(conn); + } else |err| { + // can happen if we're in a perform() call, so we'll queue this + // for cleanup later. + const node = &conn.node; + self.in_use.remove(node); + self.dirty.append(node); + log.warn(.http, "multi remove handle", .{ .err = err }); + } + } + + pub fn isAvailable(self: *Handles, conn: *Connection) void { + const node = &conn.node; self.in_use.remove(node); - node.prev = null; - node.next = null; self.available.append(node); } pub fn perform(self: *Handles) !c_int { - var running: c_int = undefined; self.performing = true; defer self.performing = false; + + const multi = self.multi; + var running: c_int = undefined; try libcurl.curl_multi_perform(self.multi, &running); + + { + const list = &self.dirty; + while (list.first) |node| { + list.remove(node); + const conn: *Connection = @fieldParentPtr("node", node); + if (libcurl.curl_multi_remove_handle(multi, conn.easy)) { + self.available.append(node); + } else |err| { + log.fatal(.http, "multi remove handle", .{ .err = err, .src = "perform" }); + @panic("multi_remove_handle"); + } + } + } + return running; } diff --git a/src/browser/ScriptManager.zig b/src/browser/ScriptManager.zig index 4b5316e7..0466f125 100644 --- a/src/browser/ScriptManager.zig +++ b/src/browser/ScriptManager.zig @@ -634,6 +634,8 @@ pub const Script = struct { debug_transfer_notified_fail: bool = false, debug_transfer_redirecting: bool = false, debug_transfer_intercept_state: u8 = 0, + debug_transfer_auth_challenge: bool = false, + debug_transfer_easy_id: usize = 0, const Kind = enum { module, @@ -711,6 +713,8 @@ pub const Script = struct { .a5 = self.debug_transfer_notified_fail, .a6 = self.debug_transfer_redirecting, .a7 = self.debug_transfer_intercept_state, + .a8 = self.debug_transfer_auth_challenge, + .a9 = self.debug_transfer_easy_id, .b1 = transfer.id, .b2 = transfer._tries, .b3 = transfer.aborted, @@ -718,6 +722,8 @@ pub const Script = struct { .b5 = transfer._notified_fail, .b6 = transfer._redirecting, .b7 = @intFromEnum(transfer._intercept_state), + .b8 = transfer._auth_challenge != null, + .b9 = if (transfer._conn) |c| @intFromPtr(c.easy) else 0, }); self.header_callback_called = true; self.debug_transfer_id = transfer.id; @@ -727,6 +733,8 @@ pub const Script = struct { self.debug_transfer_notified_fail = transfer._notified_fail; self.debug_transfer_redirecting = transfer._redirecting; self.debug_transfer_intercept_state = @intFromEnum(transfer._intercept_state); + self.debug_transfer_auth_challenge = transfer._auth_challenge != null; + self.debug_transfer_easy_id = if (transfer._conn) |c| @intFromPtr(c.easy) else 0; } lp.assert(self.source.remote.capacity == 0, "ScriptManager.Header buffer", .{ .capacity = self.source.remote.capacity }); diff --git a/src/cdp/domains/fetch.zig b/src/cdp/domains/fetch.zig index 8a54881b..beed6d76 100644 --- a/src/cdp/domains/fetch.zig +++ b/src/cdp/domains/fetch.zig @@ -406,10 +406,10 @@ pub fn requestAuthRequired(bc: anytype, intercept: *const Notification.RequestAu .fetch => "Fetch", }, .authChallenge = .{ - .source = if (challenge.source == .server) "Server" else "Proxy", .origin = "", // TODO get origin, could be the proxy address for example. - .scheme = if (challenge.scheme == .digest) "digest" else "basic", - .realm = challenge.realm, + .source = if (challenge.source) |s| (if (s == .server) "Server" else "Proxy") else "", + .scheme = if (challenge.scheme) |s| (if (s == .digest) "digest" else "basic") else "", + .realm = challenge.realm orelse "", }, .networkId = &id.toRequestId(transfer.id), }, .{ .session_id = session_id }); diff --git a/src/http/Client.zig b/src/http/Client.zig index 26284b8a..326635f4 100644 --- a/src/http/Client.zig +++ b/src/http/Client.zig @@ -66,7 +66,7 @@ active: usize, intercepted: usize, // Our easy handles, managed by a curl multi. -handles: Handles, +handles: Net.Handles, // Use to generate the next request ID next_request_id: u32 = 0, @@ -128,7 +128,7 @@ pub fn init(allocator: Allocator, ca_blob: ?Net.Blob, robot_store: *RobotStore, const client = try allocator.create(Client); errdefer allocator.destroy(client); - var handles = try Handles.init(allocator, ca_blob, config); + var handles = try Net.Handles.init(allocator, ca_blob, config); errdefer handles.deinit(allocator); // Set transfer callbacks on each connection. @@ -191,6 +191,8 @@ fn _abort(self: *Client, comptime abort_all: bool, frame_id: u32) void { n = node.next; const conn: *Net.Connection = @fieldParentPtr("node", node); var transfer = Transfer.fromConnection(conn) catch |err| { + // Let's cleanup what we can + self.handles.remove(conn); log.err(.http, "get private info", .{ .err = err, .source = "abort" }); continue; }; @@ -665,7 +667,7 @@ pub fn restoreOriginalProxy(self: *Client) !void { } // Enable TLS verification on all connections. -pub fn enableTlsVerify(self: *const Client) !void { +pub fn enableTlsVerify(self: *Client) !void { // Remove inflight connections check on enable TLS b/c chromiumoxide calls // the command during navigate and Curl seems to accept it... @@ -675,7 +677,7 @@ pub fn enableTlsVerify(self: *const Client) !void { } // Disable TLS verification on all connections. -pub fn disableTlsVerify(self: *const Client) !void { +pub fn disableTlsVerify(self: *Client) !void { // Remove inflight connections check on disable TLS b/c chromiumoxide calls // the command during navigate and Curl seems to accept it... @@ -689,7 +691,11 @@ fn makeRequest(self: *Client, conn: *Net.Connection, transfer: *Transfer) anyerr { transfer._conn = conn; - errdefer transfer.deinit(); + errdefer { + transfer._conn = null; + transfer.deinit(); + self.handles.isAvailable(conn); + } try conn.setURL(req.url); try conn.setMethod(req.method); @@ -716,17 +722,20 @@ fn makeRequest(self: *Client, conn: *Net.Connection, transfer: *Transfer) anyerr } } - // Once soon as this is called, our "perform" loop is responsible for + // As soon as this is called, our "perform" loop is responsible for // cleaning things up. That's why the above code is in a block. If anything - // fails BEFORE `curl_multi_add_handle` suceeds, the we still need to do + // fails BEFORE `curl_multi_add_handle` succeeds, the we still need to do // cleanup. But if things fail after `curl_multi_add_handle`, we expect // perfom to pickup the failure and cleanup. - try self.handles.add(conn); + self.handles.add(conn) catch |err| { + transfer._conn = null; + transfer.deinit(); + self.handles.isAvailable(conn); + return err; + }; if (req.start_callback) |cb| { cb(transfer) catch |err| { - self.handles.remove(conn); - transfer._conn = null; transfer.deinit(); return err; }; @@ -834,7 +843,7 @@ fn processMessages(self: *Client) !bool { // In case of request w/o data, we need to call the header done // callback now. const proceed = transfer.headerDoneCallback(&msg.conn) catch |err| { - log.err(.http, "header_done_callback", .{ .err = err }); + log.err(.http, "header_done_callback2", .{ .err = err }); requestFailed(transfer, err, true); continue; }; @@ -872,8 +881,6 @@ fn ensureNoActiveConnection(self: *const Client) !void { } } -const Handles = Net.Handles; - pub const RequestCookie = struct { is_http: bool, jar: *CookieJar, @@ -1300,9 +1307,9 @@ pub const Transfer = struct { // WWW-Authenticate or Proxy-Authenticate header. transfer._auth_challenge = .{ .status = status, - .source = undefined, - .scheme = undefined, - .realm = undefined, + .source = null, + .scheme = null, + .realm = null, }; return buf_len; }