Merge pull request #3098 from lightpanda-io/page-double-free

uaf: Fix page double-free on navigate failure.

src/browser/Page.zig

@@ -138,6 +138,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

src/browser/Session.zig

@@ -282,6 +282,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");
 }
 
@@ -290,6 +295,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| {
@@ -387,10 +396,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();
@@ -843,7 +851,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 lp.IS_DEBUG) {
         log.debug(.browser, "initiate root navigation", .{ .url = url });
@@ -974,3 +983,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();
+}