From 634e27c9c67cc22b76b88f094e1b81f0ed893f12 Mon Sep 17 00:00:00 2001
From: Karl Seguin <k@openmymind.io>
Date: Sat, 1 Aug 2026 17:41:19 +0800
Subject: [PATCH] uaf: Fix page double-free on navigate failure.

This is a classic (for us) issue. Session.initiateRootNavigation errdefer a
number of steps BUT, if the frame.navigate() call reaches libcurl's event-loop,
then those errdefer can conflict with the `frameErrorCallback`. Who's
responsible for cleaning up? It depends where the failure happened.

So the cleanup is now more defensive. Page.destroying guards against multiple
calls to Session.queuePageDestruction and initiateRootNavigation no longer
assumes that the new Page is in the pages collection.

I believe this is the cause of the "release overflow" for `Selection` that is
infrequent but has been seen for a long time.
---
 src/browser/Page.zig    |  7 ++++++
 src/browser/Session.zig | 48 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/src/browser/Page.zig b/src/browser/Page.zig
index 597c0ba47d..e8046b739e 100644
--- a/src/browser/Page.zig
+++ b/src/browser/Page.zig
@@ -140,6 +140,13 @@ replaces: ?*Page = null,
 // code. The two are kept in sync.
 replacement: ?*Page = null,
 
+// Prevents double entry in session._page_destruction_queue. Can happen since
+// various paths can enter this, and there isn't always a single clear owner
+// of who should errdefer, e.g. if this happens before a navigation's
+// transfer.submit(), then the caller needs to handle the failure. If it happens
+// after, then frameErrorCallback does.
+destroying: bool = false,
+
 // The viewport every consumer should read. The runtime override (set via
 // Emulation.setDeviceMetricsOverride) is stored on the Browser so it persists
 // across page navigations; delegate to it here, keeping a single read path for
diff --git a/src/browser/Session.zig b/src/browser/Session.zig
index 9b318e2171..be8fe3aaea 100644
--- a/src/browser/Session.zig
+++ b/src/browser/Session.zig
@@ -284,6 +284,11 @@ fn allocatePage(self: *Session, frame_id: u32) !*Page {
 
 // Tear down and free a Page allocated via allocatePage.
 fn queuePageDestruction(self: *Session, page: *Page) void {
+    if (page.destroying) {
+        // see Page.destroying field for why this is necessary.
+        return;
+    }
+    page.destroying = true;
     self._page_destruction_queue.append(self.arena.allocator(), page) catch @panic("OOM");
 }
 
@@ -292,6 +297,10 @@ fn retire(self: *Session, page: *Page) void {
         // page is being destroyed, if it was replacing a page, then update that
         // page's replacement to keep replaces<->replacement consistent.
         live.replacement = null;
+        // Might have retired while transfer was active and the Frame's
+        // header/error callback might still happen. Setting this null prevents
+        // those callbacks from using this page post-retirement
+        page.replaces = null;
     }
 
     if (page.replacement) |replacement| {
@@ -389,10 +398,9 @@ pub fn closePage(self: *Session, frame_id: u32) void {
 // We queue then process so that there is a single place (processDestroyQueues)
 // where pages get destroyed.
 pub fn closeAllPages(self: *Session) void {
-    self._page_destruction_queue.ensureUnusedCapacity(self.arena.allocator(), self.pages.items.len) catch @panic("OOM");
     for (self.pages.items) |page| {
         page.frame.abortTransfers();
-        self._page_destruction_queue.appendAssumeCapacity(page);
+        self.queuePageDestruction(page);
     }
     self.pages.clearRetainingCapacity();
     self.processDestroyQueues();
@@ -845,7 +853,8 @@ pub fn initiateRootNavigation(self: *Session, frame_id: u32, url: [:0]const u8,
 
     errdefer live.replacement = null;
     try self.pages.append(self.arena.allocator(), page);
-    errdefer _ = self.pages.pop();
+    // don't just pop, frame.navitate() can itself remove this page from the pages list
+    errdefer self.removePageFromList(page);
 
     if (comptime IS_DEBUG) {
         log.debug(.browser, "initiate root navigation", .{ .url = url });
@@ -976,3 +985,36 @@ pub const PageHandle = struct {
         return f.navigate(request_url, opts);
     }
 };
+
+const testing = @import("../testing.zig");
+test "Session: retiring a pending page destroys it once" {
+    const session = testing.test_session;
+    defer session.closeAllPages();
+
+    const handle = try session.createPage();
+    const live = handle.page().?;
+
+    // A pending root navigation, set up like initiateRootNavigation does.
+    const pending = try session.allocatePage(live.frame._frame_id);
+    pending.replaces = live;
+    live.replacement = pending;
+    try session.pages.append(session.arena.allocator(), pending);
+
+    // Its transfer failed inside submit(): the error_callback discards the
+    // pending page...
+    session.discardPendingPage(pending);
+    try testing.expectEqual(null, pending.replaces);
+    try testing.expectEqual(null, live.replacement);
+
+    // ...and then submit's error unwinds initiateRootNavigation's errdefers
+    // over a page that is already off the list and already queued.
+    session.removePageFromList(pending);
+    session.queuePageDestruction(pending);
+
+    try testing.expectEqual(1, session.pages.items.len);
+    try testing.expectEqual(live, session.pages.items[0]);
+    try testing.expectEqual(1, session._page_destruction_queue.items.len);
+
+    // Would deinit `pending` twice if it had been queued twice.
+    session.processDestroyQueues();
+}