From 01b8bb305b88f0a9cc37f5c284dc0e2ac845477e Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Tue, 26 May 2026 16:43:28 +0800 Subject: [PATCH] Move FinalizerCallbackIdentity lifetime from Session to Browser A v8 Finalizer can fire at any point, including after the underlying Zig object has been freed. Even after the corresponding Frame and Page have been freed. To deal with this, v8 Finalizers are tied to a special FinalizerCallback.Identity (fci) which tracks whether the underlying Page still exists (fci.done == true). This ensures that, if v8 calls an finalizer much later, we don't UAF. All we have to do is cleanup the fci itself and exit - everything else is already gone. The problem is, we tied the fci's to the session, but the v8 Finalizer can outlive the Session (which is meaningful to v8). We made this work by trying to force the isolate to reclaim memory on session deinit. The problem with this is that it still isn't guaranteed to work (though, practically speaking, if this _has_ resulted in any UAF, it's been rare). PLUS it puts load on the v8 worker threads that can compound over short-lived sessions. The fix is to move the fci to the browser. This is safe because Browser == Env == Isolate, so when the browser is torn down, the isolate is torn down, and at THAT point, we're sure no Finalizers can fire. With this change, the call to `memoryPressureNotification(.critical)` in Session.deinit has been removed. --- src/browser/Browser.zig | 25 ++++++++++++----- src/browser/Page.zig | 6 ++-- src/browser/Session.zig | 58 --------------------------------------- src/browser/js/Local.zig | 33 ++++++++++++++-------- src/browser/js/bridge.zig | 6 ++-- src/browser/js/js.zig | 58 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 103 insertions(+), 83 deletions(-) diff --git a/src/browser/Browser.zig b/src/browser/Browser.zig index cf14fb8f..1c6177ee 100644 --- a/src/browser/Browser.zig +++ b/src/browser/Browser.zig @@ -18,18 +18,17 @@ const std = @import("std"); -const Allocator = std.mem.Allocator; +const App = @import("../App.zig"); +const CDP = @import("../cdp/CDP.zig"); +const Notification = @import("../Notification.zig"); const js = @import("js/js.zig"); -const App = @import("../App.zig"); +const Page = @import("Page.zig"); +const Session = @import("Session.zig"); const HttpClient = @import("HttpClient.zig"); -const CDP = @import("../cdp/CDP.zig"); const ArenaPool = App.ArenaPool; - -const Session = @import("Session.zig"); -const Page = @import("Page.zig"); -const Notification = @import("../Notification.zig"); +const Allocator = std.mem.Allocator; // Browser is an instance of the browser. // You can create multiple browser instances. @@ -46,6 +45,14 @@ http_client: HttpClient, // used by sessions to allocate pages. page_pool: std.heap.MemoryPool(Page), +// Pool for FinalizerCallback.Identity structs — the records V8 weak-callback +// parameters point at. Scoped to the Browser (i.e. the V8 Isolate's lifetime) +// rather than the Session: V8 can run a weak finalizer arbitrarily late, any +// time up until the Isolate is torn down, so these must outlive every Session. +// Freed in deinit *after* env.deinit() tears down the Isolate — the point past +// which no finalizer can fire. +fc_identity_pool: std.heap.MemoryPool(js.FinalizerCallback.Identity), + // Monotonic frame-ID generator scoped to this Browser (one per CDP // connection). Lives here, not on Session, because CDP target IDs // (encoded as `FID-{d:0>10}`) must be unique for the lifetime of the @@ -84,6 +91,7 @@ pub fn init(self: *Browser, app: *App, opts: InitOpts, cdp: ?*CDP) !void { .arena_pool = &app.arena_pool, .http_client = undefined, .page_pool = std.heap.MemoryPool(Page).init(allocator), + .fc_identity_pool = .init(allocator), }; try self.http_client.init(allocator, &app.network, cdp); } @@ -91,6 +99,9 @@ pub fn init(self: *Browser, app: *App, opts: InitOpts, cdp: ?*CDP) !void { pub fn deinit(self: *Browser) void { self.closeSession(); self.env.deinit(); + // After env.deinit() the Isolate is gone, so no further weak finalizer can + // fire — only now is it safe to free the pool backing their parameters. + self.fc_identity_pool.deinit(); self.page_pool.deinit(); self.http_client.deinit(); } diff --git a/src/browser/Page.zig b/src/browser/Page.zig index f7d0bebc..9a3f764d 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -75,9 +75,9 @@ identity: js.Identity = .{}, // Finalizer callbacks for Zig instances exposed to v8 in this Page. Keyed by // Zig instance ptr. The backing FinalizerCallback.Identity structs come from -// Session.fc_identity_pool so they outlive the Page for v8 weak-callback -// safety. -finalizer_callbacks: std.AutoHashMapUnmanaged(usize, *Session.FinalizerCallback) = .empty, +// Browser.fc_identity_pool so they outlive the Page (and the Session) for v8 +// weak-callback safety. +finalizer_callbacks: std.AutoHashMapUnmanaged(usize, *js.FinalizerCallback) = .empty, // Tracked global v8 objects that need to be released when the Page tears down. globals: std.ArrayList(v8.Global) = .empty, diff --git a/src/browser/Session.zig b/src/browser/Session.zig index 5f9ede3e..4bd4259f 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -63,10 +63,6 @@ inject_scripts: []const []const u8 = &.{}, // Shared allocator. Used by Session itself and borrowed by Pages. arena_pool: *ArenaPool, -// Pool for FinalizerCallback.Identity structs. These must survive Page -// teardowns so V8 weak callbacks can validate the FC before dereferencing it. -fc_identity_pool: std.heap.MemoryPool(FinalizerCallback.Identity), - // The currently-active Page // flips this pointer. _active: ?*Page = null, @@ -116,7 +112,6 @@ pub fn init(self: *Session, browser: *Browser, notification: *Notification) !voi .storage_shed = .{}, .browser = browser, .notification = notification, - .fc_identity_pool = .init(allocator), .cookie_jar = storage.Cookie.Jar.init(allocator), // CLI defaults; LP.configureLoading can flip these per-session. .subframe_loading_enabled = !browser.app.config.disableSubframes(), @@ -136,13 +131,6 @@ pub fn deinit(self: *Session) void { self.cookie_jar.deinit(); - // Force V8 to flush any remaining weak callbacks while - // fc_identity_pool is still alive. Identity structs allocated from - // this pool back V8 weak-callback parameters; freeing the pool first - // would leave dangling pointers that segfault on the next GC. - self.browser.env.memoryPressureNotification(.critical); - self.fc_identity_pool.deinit(); - self.storage_shed.deinit(self.browser.app.allocator); self.arena_pool.release(self.arena); } @@ -647,49 +635,3 @@ pub fn nextLoaderId(self: *Session) u32 { self.loader_id_gen = id; return id; } - -// Every finalizable instance of Zig gets 1 FinalizerCallback registered in the -// Page. This is to ensure that, if v8 doesn't finalize the value, we can -// release on Page teardown. -pub const FinalizerCallback = struct { - page: *Page, - arena: Allocator, - resolved_ptr_id: usize, - finalizer_ptr_id: usize, - release_ref: *const fn (ptr_id: usize, page: *Page) void, - - // Linked list of Identities referencing this FC. - identities: ?*Identity = null, - // Count of active identities (for knowing when to clean up FC). - identity_count: u8 = 0, - - // For every FinalizerCallback we'll have 1+ FinalizerCallback.Identity: one - // for every identity that gets the instance. In most cases, that'll be 1. - // Allocated from Session.fc_identity_pool so it survives Page teardowns and - // allows the weak callback to safely check the done flag. - pub const Identity = struct { - session: *Session, - // The Page that owns the FinalizerCallback this Identity references. - // Only safe to dereference when `done == false`. When done is true, - // the Page may have been torn down and this pointer is stale. - page: *Page, - identity: *js.Identity, - finalizer_ptr_id: usize, - resolved_ptr_id: usize, - next: ?*Identity = null, - done: bool = false, - }; - - // Called during Page teardown to force cleanup regardless of identities. - pub fn deinit(self: *FinalizerCallback, page: *Page) void { - // Mark all identities as done so stale V8 weak callbacks - // won't find the wrong FC if resolved_ptr_id is reused. - var id = self.identities; - while (id) |identity| { - identity.done = true; - id = identity.next; - } - self.release_ref(self.finalizer_ptr_id, page); - page.releaseArena(self.arena); - } -}; diff --git a/src/browser/js/Local.zig b/src/browser/js/Local.zig index e1ec6be3..900b0b59 100644 --- a/src/browser/js/Local.zig +++ b/src/browser/js/Local.zig @@ -18,10 +18,10 @@ const std = @import("std"); const lp = @import("lightpanda"); + const string = @import("../../string.zig"); const Page = @import("../Page.zig"); -const FinalizerCallback = @import("../Session.zig").FinalizerCallback; const js = @import("js.zig"); const bridge = @import("bridge.zig"); @@ -33,6 +33,7 @@ const TaggedOpaque = @import("TaggedOpaque.zig"); const v8 = js.v8; const log = lp.log; const CallOpts = Caller.CallOpts; +const FinalizerCallback = js.FinalizerCallback; // Where js.Context has a lifetime tied to the frame, and holds the // v8::Global, this has a much shorter lifetime and holds a @@ -293,10 +294,11 @@ pub fn mapZigInstanceToJs(self: *const Local, js_obj_handle: ?*const v8.Object, finalizer_gop.value_ptr.* = try self.createFinalizerCallback(resolved_ptr_id, finalizer_ptr_id, finalizer.release_ref_from_zig); } const fc = finalizer_gop.value_ptr.*; - const identity_finalizer = try session.fc_identity_pool.create(); + const browser = session.browser; + const identity_finalizer = try browser.fc_identity_pool.create(); identity_finalizer.* = .{ + .browser = browser, .page = page, - .session = session, .identity = ctx.identity, .finalizer_ptr_id = finalizer_ptr_id, .resolved_ptr_id = resolved_ptr_id, @@ -1248,23 +1250,30 @@ fn resolveT(comptime T: type, value: *T) Resolved { const ptr = v8.v8__WeakCallbackInfo__GetParameter(handle.?).?; const identity_finalizer: *FinalizerCallback.Identity = @ptrCast(@alignCast(ptr)); - // Identity is allocated from pool, so it's valid even after frame reset. + // The Identity lives in browser.fc_identity_pool, which outlives + // the page and the session, so freeing ourselves is always safe. + // `browser` is the only field we may touch unconditionally. + defer identity_finalizer.browser.fc_identity_pool.destroy(identity_finalizer); + + // If done, the owning scope (page / isolated world) was already + // torn down: its identity.deinit() reset our Global and the FC + // was already released. The page, identity_map and finalizer_ptr_id + // are all stale (the Page may even be reused by a later Session), + // so we must not dereference them — just free ourselves (defer). + if (identity_finalizer.done) { + return; + } + const page = identity_finalizer.page; const resolved_ptr_id = identity_finalizer.resolved_ptr_id; - defer page.session.fc_identity_pool.destroy(identity_finalizer); - // Always clean up the identity map entry + // Clean up the identity map entry: the object is being collected, + // so our Global to it is dead. if (identity_finalizer.identity.identity_map.fetchRemove(resolved_ptr_id)) |kv| { var global = kv.value; v8.v8__Global__Reset(&global); } - // If done, FC was already cleaned up during Page teardown. - // The finalizer_ptr_id may have been reused for a new object, - // so we must not look it up in the map. It's also unsafe to - // dereference identity_finalizer.page after done is true. - if (identity_finalizer.done) return; - const finalizer_ptr_id = identity_finalizer.finalizer_ptr_id; const fc = page.finalizer_callbacks.get(finalizer_ptr_id) orelse return; diff --git a/src/browser/js/bridge.zig b/src/browser/js/bridge.zig index 5de1d1b1..4754f488 100644 --- a/src/browser/js/bridge.zig +++ b/src/browser/js/bridge.zig @@ -19,14 +19,14 @@ const std = @import("std"); const lp = @import("lightpanda"); -const js = @import("js.zig"); const Frame = @import("../Frame.zig"); -const v8 = js.v8; - +const js = @import("js.zig"); const Caller = @import("Caller.zig"); const Context = @import("Context.zig"); +const v8 = js.v8; +const Allocator = std.mem.Allocator; const IS_DEBUG = @import("builtin").mode == .Debug; pub fn Builder(comptime T: type) type { diff --git a/src/browser/js/js.zig b/src/browser/js/js.zig index adc36255..1d4140fe 100644 --- a/src/browser/js/js.zig +++ b/src/browser/js/js.zig @@ -50,6 +50,7 @@ pub const Integer = @import("Integer.zig"); pub const PromiseResolver = @import("PromiseResolver.zig"); pub const PromiseRejection = @import("PromiseRejection.zig"); +const js = @This(); const Allocator = std.mem.Allocator; pub fn Bridge(comptime T: type) type { @@ -359,3 +360,60 @@ test "TaggedAnyOpaque" { // If we grow this, fine, but it should be a conscious decision try std.testing.expectEqual(24, @sizeOf(@import("TaggedOpaque.zig"))); } + +// Every finalizable instance of Zig gets 1 FinalizerCallback registered in the +// Page. This is to ensure that, if v8 doesn't finalize the value, we can +// release on Page teardown. +pub const FinalizerCallback = struct { + page: *Page, + arena: Allocator, + resolved_ptr_id: usize, + finalizer_ptr_id: usize, + release_ref: *const fn (ptr_id: usize, page: *Page) void, + + // Linked list of Identities referencing this FC. + identities: ?*FinalizerCallback.Identity = null, + // Count of active identities (for knowing when to clean up FC). + identity_count: u8 = 0, + + const Page = @import("../Page.zig"); + const Browser = @import("../Browser.zig"); + + // For every FinalizerCallback we'll have 1+ FinalizerCallback.Identity: one + // for every identity that gets the instance. In most cases, that'll be 1. + // Allocated from Browser.fc_identity_pool so it survives Page *and* Session + // teardowns — V8 may fire the weak callback any time before the Isolate is + // torn down — and lets the callback safely check the done flag. + pub const Identity = struct { + // The Page that owns the FinalizerCallback this Identity references. + // Only safe to dereference when `done == false`. When done is true, + // the Page may have been torn down and this pointer is stale. + page: *Page, + + // Stable handle to the pool this struct came from. The weak callback + // reaches the pool through here (not via page/session) so it stays + // valid to self-destruct even when `done` and the page/session are gone. + browser: *Browser, + + // The world's identity map. Only safe to dereference when `done == false` + // (see `browser` above) — its teardown already reset every Global. + identity: *js.Identity, + finalizer_ptr_id: usize, + resolved_ptr_id: usize, + next: ?*FinalizerCallback.Identity = null, + done: bool = false, + }; + + // Called during Page teardown to force cleanup regardless of identities. + pub fn deinit(self: *FinalizerCallback, page: *Page) void { + // Mark all identities as done so stale V8 weak callbacks + // won't find the wrong FC if resolved_ptr_id is reused. + var id = self.identities; + while (id) |identity| { + identity.done = true; + id = identity.next; + } + self.release_ref(self.finalizer_ptr_id, page); + page.releaseArena(self.arena); + } +};