From 377de019b85b91a655f505edec24df0321c523cc Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 10 Jul 2026 08:22:28 +0800 Subject: [PATCH] fix uaf when aborting a robots-parked transfer --- src/network/HttpClient.zig | 71 ++++++++++++++++++++-- src/network/RobotsGate.zig | 121 +++++++++++++++++++++++++++---------- 2 files changed, 156 insertions(+), 36 deletions(-) diff --git a/src/network/HttpClient.zig b/src/network/HttpClient.zig index b8dc2070f..48d60a135 100644 --- a/src/network/HttpClient.zig +++ b/src/network/HttpClient.zig @@ -380,6 +380,9 @@ pub fn abort(self: *Client) void { std.debug.assert(self.in_use.first == null); std.debug.assert(self.ready_queue.first == null); std.debug.assert(self.dirty.first == null); + // - self.robots.pending : each robots fetch's shutdown_callback + // drops its entry; parked waiters unlink in their own deinit. + std.debug.assert(self.robots.pending.count() == 0); } } @@ -1881,10 +1884,7 @@ pub const Transfer = struct { self._conn = null; } - // Unlink from client.queue if we were waiting for a handle. - // Without this, deinit'ing a queued transfer (e.g. via owner-list - // abort during navigation) leaves a dangling _node in the queue - // that the next tick would pop and hand to libcurl → UAF. + // Unlink from client.pending_queue if we were waiting for a handle. if (self.state == .queued) { self.client.pending_queue.remove(&self._node); } @@ -1900,6 +1900,12 @@ pub const Transfer = struct { self.client.dispatch_count -= 1; } + // And for the robots gate: RobotsGate.pending holds a raw *Transfer + // while we're parked. + if (self.state == .parked and self.state.parked == .robots) { + self.client.robots.remove(self); + } + // A pending revalidation entry owns cache resources (possibly an // open file). It's resolved at completion; if we die first, release. switch (self._cache_intent) { @@ -2832,6 +2838,7 @@ fn initTestClient(client: *Client, pool: *ArenaPool) void { client.cache = null; client.serve_mode = false; client.obey_robots = false; + client.robots = .{ .allocator = testing.allocator, .network = undefined }; } test "HttpClient: fulfillIntercepted survives a done_callback that tears down the owner" { @@ -2911,6 +2918,62 @@ test "HttpClient: fulfillIntercepted survives a done_callback that tears down th try testing.expectEqual(null, owner.transfers.first); } +test "HttpClient: aborting a robots-parked transfer unlinks it from the gate" { + // Regression: RobotsGate.pending kept a raw *Transfer with nothing + // removing it when a parked transfer was aborted out-of-band + // (xhr.abort(), owner teardown). The robots.txt resolution would then + // unpark freed memory. + var pool = ArenaPool.init(testing.allocator, .{}); + defer pool.deinit(); + + var client: Client = undefined; + initTestClient(&client, &pool); + defer client.transfers.deinit(testing.allocator); + defer client.robots.deinit(); + + const robots_url = "http://example.com/robots.txt"; + + var waiting: std.ArrayList(*Transfer) = .empty; + for (0..2) |i| { + const arena = try pool.acquire(.small, "test"); + const transfer = try arena.create(Transfer); + transfer.* = .{ + .arena = arena, + .owner = null, + .req = .{ + .frame_id = 0, + .loader_id = 0, + .method = .GET, + .url = "http://example.com/", + .cookie_jar = null, + .cookie_origin = "", + .resource_type = .document, + .notification = undefined, + .shutdown_callback = noopShutdown, + }, + .client = &client, + .id = @intCast(i + 1), + .start_time = 0, + }; + try client.transfers.putNoClobber(testing.allocator, transfer.id, transfer); + try waiting.append(testing.allocator, transfer); + transfer.park(.robots); + } + try client.robots.pending.putNoClobber(testing.allocator, robots_url, waiting); + + const t1 = client.robots.pending.get(robots_url).?.items[0]; + const t2 = client.robots.pending.get(robots_url).?.items[1]; + + t1.abort(error.Abort); + try testing.expectEqual(1, client.robots.pending.get(robots_url).?.items.len); + try testing.expect(client.robots.pending.get(robots_url).?.items[0] == t2); + try testing.expectEqual(1, client.transfers.count()); + + t2.abort(error.Abort); + try testing.expectEqual(0, client.robots.pending.get(robots_url).?.items.len); + try testing.expectEqual(0, client.transfers.count()); +} + test "HttpClient: fulfillIntercepted follows a 3xx redirect" { // Regression for #2828: a CDP Fetch.fulfillRequest with a 3xx status + a // Location header must be followed like a real network redirect (re-issued diff --git a/src/network/RobotsGate.zig b/src/network/RobotsGate.zig index 0fec17b3a..27bbf7068 100644 --- a/src/network/RobotsGate.zig +++ b/src/network/RobotsGate.zig @@ -25,6 +25,7 @@ const std = @import("std"); const lp = @import("lightpanda"); const URL = @import("../browser/URL.zig"); +const ArenaPool = @import("../ArenaPool.zig"); const Robots = @import("Robots.zig"); const Network = @import("Network.zig"); @@ -71,40 +72,66 @@ pub fn check(self: *RobotsGate, transfer: *Transfer) !Result { return .pending; } -fn fetchThenResume(self: *RobotsGate, robots_url: [:0]const u8, transfer: *Transfer) !void { - const entry = try self.pending.getOrPut(self.allocator, robots_url); +// A parked transfer is dying out-of-band (abort, owner teardown) — unlink +// it so the robots.txt resolution doesn't touch freed memory. The map entry +// stays: the in-flight fetch owns it (the key lives on the fetch's context +// arena) and still resolves the remaining waiters. +pub fn remove(self: *RobotsGate, transfer: *Transfer) void { + var it = self.pending.valueIterator(); + while (it.next()) |waiting| { + for (waiting.items, 0..) |t, i| { + if (t == transfer) { + _ = waiting.swapRemove(i); + return; + } + } + } +} - if (entry.found_existing) { +fn fetchThenResume(self: *RobotsGate, robots_url: [:0]const u8, transfer: *Transfer) !void { + if (self.pending.getPtr(robots_url)) |waiting| { // A fetch for this robots.txt is already in flight, queue behind it. - try entry.value_ptr.append(self.allocator, transfer); + try waiting.append(self.allocator, transfer); transfer.park(.robots); return; } - errdefer _ = self.pending.remove(robots_url); - entry.value_ptr.* = .empty; + const client = transfer.client; - try entry.value_ptr.append(self.allocator, transfer); - transfer.park(.robots); - errdefer { - entry.value_ptr.deinit(self.allocator); - transfer.unpark(); - } + // The context, the response buffer and the pending-map key live on + // their own pooled arena, NOT on transfer.arena — any waiter (this one + // included) can be aborted while the fetch is still in flight, and the + // fetch's callbacks must survive that. The arena is released by + // whichever terminal callback fires (done / error / shutdown). + const arena = try client.arena_pool.acquire(.small, "RobotsGate.RobotsContext"); + errdefer client.arena_pool.release(arena); - const robots_ctx = try transfer.arena.create(RobotsContext); + const owned_url = try arena.dupeZ(u8, robots_url); + const robots_ctx = try arena.create(RobotsContext); robots_ctx.* = .{ .gate = self, .buffer = .empty, - .arena = transfer.arena, - .robots_url = robots_url, + .arena = arena, + .arena_pool = client.arena_pool, + .robots_url = owned_url, }; - log.debug(.browser, "fetching robots.txt", .{ .robots_url = robots_url }); + var waiting: std.ArrayList(*Transfer) = .empty; + try waiting.append(self.allocator, transfer); + errdefer waiting.deinit(self.allocator); + + try self.pending.putNoClobber(self.allocator, owned_url, waiting); + errdefer _ = self.pending.remove(owned_url); + + transfer.park(.robots); + errdefer transfer.unpark(); + + log.debug(.browser, "fetching robots.txt", .{ .robots_url = owned_url }); // Only the parent's frame/loader ids (CDP correlation) and notification // carry over — no cookies, credentials, headers, or timeout. - try transfer.client.request(.{ - .url = robots_url, + const fetch_transfer = try client.newRequest(.{ + .url = owned_url, .method = .GET, .internal = true, .resource_type = .fetch, @@ -112,7 +139,7 @@ fn fetchThenResume(self: *RobotsGate, robots_url: [:0]const u8, transfer: *Trans .loader_id = transfer.req.loader_id, .notification = transfer.req.notification, .cookie_jar = null, - .cookie_origin = robots_url, + .cookie_origin = owned_url, .ctx = robots_ctx, .header_callback = RobotsContext.headerCallback, .data_callback = RobotsContext.dataCallback, @@ -120,14 +147,31 @@ fn fetchThenResume(self: *RobotsGate, robots_url: [:0]const u8, transfer: *Trans .error_callback = RobotsContext.errorCallback, .shutdown_callback = RobotsContext.shutdownCallback, }, transfer.owner); + + // From here the fetch owns the pending entry and the context arena. If + // submit fails it fires error_callback — possibly synchronously, right + // here — which resolves the waiters (fail-open, may already have resumed + // `transfer`) and releases the arena. So there is nothing to unwind + // locally and the errdefers above must not run: swallow the error. + fetch_transfer.submit() catch {}; } -fn flushPending(self: *RobotsGate, robots_url: [:0]const u8, allowed: bool) void { +// The robots.txt fetch resolved: hand every waiter back to the pipeline, +// each judged against its own path. No store entry (fetch failed, or a 200 +// whose body never got parsed) fails open. +fn flushPending(self: *RobotsGate, robots_url: []const u8) void { var queued = self.pending.fetchRemove(robots_url) orelse return; defer queued.value.deinit(self.allocator); + const robot_entry = self.network.robot_store.get(robots_url); for (queued.value.items) |transfer| { transfer.unpark(); + + const allowed = if (robot_entry) |entry| switch (entry) { + .absent => true, + .present => |robots| robots.isAllowed(URL.getPathname(transfer.req.url)), + } else true; + if (!allowed) { log.warn(.http, "blocked by robots", .{ .url = transfer.req.url }); transfer.failAsync(error.RobotsBlocked); @@ -143,9 +187,12 @@ fn flushPending(self: *RobotsGate, robots_url: [:0]const u8, allowed: bool) void } } -// shutdown_callback is only called on owner shutdown. And if this robot's fetch -// is being shutdown, than any transfers waiting for it will be shutdown too. -fn flushPendingShutdown(self: *RobotsGate, robots_url: [:0]const u8) void { +// shutdown_callback fires on owner shutdown. Drop the entry without +// resuming anyone. Waiters from the same owner are being kill()'d by the +// same teardown loop; their deinit finds no gate entry left and no-ops. +// A waiter parked by a different owner stays parked until its own owner +// tears it down — it is never resumed (known limitation). +fn flushPendingShutdown(self: *RobotsGate, robots_url: []const u8) void { var pending = self.pending.fetchRemove(robots_url) orelse return; pending.value.deinit(self.allocator); } @@ -153,6 +200,7 @@ fn flushPendingShutdown(self: *RobotsGate, robots_url: [:0]const u8) void { const RobotsContext = struct { gate: *RobotsGate, arena: Allocator, + arena_pool: *ArenaPool, robots_url: [:0]const u8, buffer: std.ArrayList(u8), status: u16 = 0, @@ -176,11 +224,8 @@ const RobotsContext = struct { fn doneCallback(ctx_ptr: *anyopaque) anyerror!void { const self: *RobotsContext = @ptrCast(@alignCast(ctx_ptr)); - const gate = self.gate; const robots_url = self.robots_url; - - var allowed = true; - const network = gate.network; + const network = self.gate.network; switch (self.status) { 200 => { @@ -195,8 +240,6 @@ const RobotsContext = struct { }; if (robots) |r| { try network.robot_store.put(robots_url, r); - const path = URL.getPathname(gate.pending.get(robots_url).?.items[0].req.url); - allowed = r.isAllowed(path); } } }, @@ -213,20 +256,34 @@ const RobotsContext = struct { }, } - gate.flushPending(robots_url, allowed); + // If anything above threw, error_callback fires next and resolves + // instead — resolve() must run exactly once. + self.resolve(); } fn errorCallback(ctx_ptr: *anyopaque, err: anyerror) void { const self: *RobotsContext = @ptrCast(@alignCast(ctx_ptr)); log.warn(.http, "robots fetch failed", .{ .err = err }); - self.gate.flushPending(self.robots_url, true); + self.resolve(); } fn shutdownCallback(ctx_ptr: *anyopaque) void { const self: *RobotsContext = @ptrCast(@alignCast(ctx_ptr)); log.debug(.http, "robots fetch shutdown", .{}); - self.gate.flushPendingShutdown(self.robots_url); + const gate = self.gate; + const pool = self.arena_pool; + const arena = self.arena; + gate.flushPendingShutdown(self.robots_url); + pool.release(arena); + } + + fn resolve(self: *RobotsContext) void { + const gate = self.gate; + const pool = self.arena_pool; + const arena = self.arena; + gate.flushPending(self.robots_url); + pool.release(arena); } };