diff --git a/src/ArenaPool.zig b/src/ArenaPool.zig index 36f7c71d..29d2a0da 100644 --- a/src/ArenaPool.zig +++ b/src/ArenaPool.zig @@ -28,6 +28,9 @@ const ArenaPool = @This(); const IS_DEBUG = builtin.mode == .Debug; +// In Debug, disable pooling to better catch UAF. +const SAFETY = IS_DEBUG == true and builtin.is_test == false; + pub const BucketSize = enum { tiny, small, medium, large }; const Bucket = struct { @@ -187,7 +190,8 @@ pub fn release(self: *ArenaPool, allocator: Allocator) void { } } - if (bucket.free_list_len >= bucket.free_list_max) { + if ((comptime SAFETY) or bucket.free_list_len >= bucket.free_list_max) { + // In Debug, we never pool. It can mask UAF bugs. arena.deinit(); self.entry_pool.destroy(entry); return; @@ -200,12 +204,14 @@ pub fn release(self: *ArenaPool, allocator: Allocator) void { pub fn reset(_: *const ArenaPool, allocator: Allocator, retain: usize) void { const arena: *ArenaAllocator = @ptrCast(@alignCast(allocator.ptr)); - _ = arena.reset(.{ .retain_with_limit = retain }); + // In Debug, free_all, it's less likely to hide things + _ = arena.reset(if (comptime SAFETY) .free_all else .{ .retain_with_limit = retain }); } pub fn resetRetain(_: *const ArenaPool, allocator: Allocator) void { const arena: *ArenaAllocator = @ptrCast(@alignCast(allocator.ptr)); - _ = arena.reset(.retain_capacity); + // In Debug, free_all, it's less likely to hide things + _ = arena.reset(if (comptime SAFETY) .free_all else .retain_capacity); } const testing = std.testing; diff --git a/src/browser/Frame.zig b/src/browser/Frame.zig index 15c65bf4..884a5f88 100644 --- a/src/browser/Frame.zig +++ b/src/browser/Frame.zig @@ -2592,10 +2592,10 @@ pub fn createElementNS(self: *Frame, namespace: Element.Namespace, name: []const .{ ._proto = undefined }, ), asUint("marquee") => return self.createHtmlElementT( - Element.Html.Generic, + Element.Html.Marquee, namespace, attribute_iterator, - .{ ._proto = undefined, ._tag_name = comptime .wrap("marquee"), ._tag = .marquee }, + .{ ._proto = undefined }, ), asUint("address") => return self.createHtmlElementT( Element.Html.Generic, diff --git a/src/browser/js/Env.zig b/src/browser/js/Env.zig index a034952b..69858758 100644 --- a/src/browser/js/Env.zig +++ b/src/browser/js/Env.zig @@ -38,6 +38,8 @@ const JsApis = bridge.JsApis; const Allocator = std.mem.Allocator; const IS_DEBUG = builtin.mode == .Debug; +const MAX_CONTEXTS = if (lp.build_config.wpt_extensions) 8192 else 128; + fn initClassIds() void { inline for (JsApis, 0..) |JsApi, i| { JsApi.Meta.class_id = i; @@ -63,8 +65,7 @@ platform: *const Platform, // the global isolate isolate: js.Isolate, -contexts: [64]*Context, -context_count: usize, +contexts: std.ArrayList(*Context), // just kept around because we need to free it on deinit isolate_params: *v8.CreateParams, @@ -184,8 +185,7 @@ pub fn init(app: *App, opts: InitOpts) !Env { .app = app, .context_id = 0, .allocator = allocator, - .contexts = undefined, - .context_count = 0, + .contexts = .empty, .isolate = isolate, .platform = &app.platform, .templates = templates, @@ -199,11 +199,12 @@ pub fn init(app: *App, opts: InitOpts) !Env { pub fn deinit(self: *Env) void { if (comptime IS_DEBUG) { - std.debug.assert(self.context_count == 0); + std.debug.assert(self.contexts.items.len == 0); } - for (self.contexts[0..self.context_count]) |ctx| { + for (self.contexts.items) |ctx| { ctx.deinit(); } + self.contexts.deinit(self.allocator); const app = self.app; const allocator = app.allocator; @@ -338,22 +339,18 @@ fn _createContext(self: *Env, global: anytype, params: ContextParams) !*Context // a v8 context, we can get our context out v8.v8__Context__SetAlignedPointerInEmbedderData(v8_context, 1, @ptrCast(context)); - const count = self.context_count; - if (count >= self.contexts.len) { + if (self.contexts.items.len >= MAX_CONTEXTS) { return error.TooManyContexts; } - self.contexts[count] = context; - self.context_count = count + 1; + try self.contexts.append(self.allocator, context); return context; } pub fn destroyContext(self: *Env, context: *Context) void { - for (self.contexts[0..self.context_count], 0..) |ctx, i| { + for (self.contexts.items, 0..) |ctx, i| { if (ctx == context) { - // Swap with last element and decrement count - self.context_count -= 1; - self.contexts[i] = self.contexts[self.context_count]; + _ = self.contexts.swapRemove(i); break; } } else { @@ -387,9 +384,11 @@ pub fn runMicrotasks(self: *Env) void { self.microtask_queues_are_running = true; defer self.microtask_queues_are_running = false; + // Re-read len/items each iteration: a checkpoint can run JS that creates + // a new context (e.g. an iframe), appending to (and reallocating) the list. var i: usize = 0; - while (i < self.context_count) : (i += 1) { - const ctx = self.contexts[i]; + while (i < self.contexts.items.len) : (i += 1) { + const ctx = self.contexts.items[i]; v8.v8__MicrotaskQueue__PerformCheckpoint(ctx.microtask_queue, v8_isolate); } } @@ -400,7 +399,11 @@ pub fn runMacrotasks(self: *Env) !void { return; } - for (self.contexts[0..self.context_count]) |ctx| { + // Re-read len/items each iteration: scheduler.run() can create a new context + // (e.g. an iframe), appending to (and reallocating) the list. + var i: usize = 0; + while (i < self.contexts.items.len) : (i += 1) { + const ctx = self.contexts.items[i]; if (comptime builtin.is_test == false) { // I hate this comptime check as much as you do. But we have tests // which rely on short execution before shutdown. In real world, it's @@ -420,7 +423,7 @@ pub fn runMacrotasks(self: *Env) !void { pub fn msToNextMacrotask(self: *Env) ?u64 { var next_task: u64 = std.math.maxInt(u64); - for (self.contexts[0..self.context_count]) |ctx| { + for (self.contexts.items) |ctx| { const candidate = ctx.scheduler.msToNextHigh() orelse continue; next_task = @min(candidate, next_task); } diff --git a/src/browser/js/Local.zig b/src/browser/js/Local.zig index ef15c4a1..15fc3d1e 100644 --- a/src/browser/js/Local.zig +++ b/src/browser/js/Local.zig @@ -34,6 +34,7 @@ const v8 = js.v8; const log = lp.log; const CallOpts = Caller.CallOpts; const FinalizerCallback = js.FinalizerCallback; +const IS_DEBUG = @import("builtin").mode == .Debug; // Where js.Context has a lifetime tied to the frame, and holds the // v8::Global, this has a much shorter lifetime and holds a @@ -1271,15 +1272,35 @@ fn resolveT(comptime T: type, value: *T) Resolved { const finalizer_ptr_id = identity_finalizer.finalizer_ptr_id; const fc = page.finalizer_callbacks.get(finalizer_ptr_id) orelse return; - const identity_count = fc.identity_count; - if (identity_count == 1) { + { + // Unlink this identity from the FC's intrusive list + var prev: ?*FinalizerCallback.Identity = null; + var node = fc.identities; + while (node) |n| { + if (n == identity_finalizer) { + if (prev) |p| { + p.next = n.next; + } else { + fc.identities = n.next; + } + fc.identity_count -= 1; + break; + } + prev = n; + node = n.next; + } else { + if (comptime IS_DEBUG) { + std.debug.assert(false); + } + } + } + + if (fc.identity_count == 0) { // Last identity - clean up the FC. // Remove from map before releaseRef to prevent address reuse issues. _ = page.finalizer_callbacks.remove(finalizer_ptr_id); FT.releaseRef(@ptrFromInt(finalizer_ptr_id), page); page.releaseArena(fc.arena); - } else { - fc.identity_count = identity_count - 1; } } diff --git a/src/browser/js/bridge.zig b/src/browser/js/bridge.zig index 2335d89a..ef215554 100644 --- a/src/browser/js/bridge.zig +++ b/src/browser/js/bridge.zig @@ -866,6 +866,7 @@ pub const PageJsApis = flattenTypes(&.{ @import("../webapi/element/html/LI.zig"), @import("../webapi/element/html/Link.zig"), @import("../webapi/element/html/Map.zig"), + @import("../webapi/element/html/Marquee.zig"), @import("../webapi/element/html/Media.zig"), @import("../webapi/element/html/Meta.zig"), @import("../webapi/element/html/Meter.zig"), diff --git a/src/browser/tests/custom_elements/reentrant_document_close.html b/src/browser/tests/custom_elements/reentrant_document_close.html new file mode 100644 index 00000000..02fc5491 --- /dev/null +++ b/src/browser/tests/custom_elements/reentrant_document_close.html @@ -0,0 +1,58 @@ + + + + + + + + + diff --git a/src/browser/tests/element/html/font.html b/src/browser/tests/element/html/font.html new file mode 100644 index 00000000..bd3d6d31 --- /dev/null +++ b/src/browser/tests/element/html/font.html @@ -0,0 +1,47 @@ + + + + + + + + diff --git a/src/browser/tests/element/html/htmlelement-props.html b/src/browser/tests/element/html/htmlelement-props.html index a008adc5..6e625d4f 100644 --- a/src/browser/tests/element/html/htmlelement-props.html +++ b/src/browser/tests/element/html/htmlelement-props.html @@ -52,5 +52,33 @@ const textarea = document.createElement('textarea'); testing.expectEqual(0, textarea.tabIndex); + + // tabIndex follows the HTML "rules for parsing integers": skip leading + // ASCII whitespace, take an optional sign and the leading run of digits, + // ignoring any trailing junk. + d3.setAttribute('tabindex', ' 7'); + testing.expectEqual(7, d3.tabIndex); + + d3.setAttribute('tabindex', '\t\n\f\r9'); + testing.expectEqual(9, d3.tabIndex); + + d3.setAttribute('tabindex', '7%'); + testing.expectEqual(7, d3.tabIndex); + + d3.setAttribute('tabindex', '1.5'); + testing.expectEqual(1, d3.tabIndex); + + d3.setAttribute('tabindex', '-3'); + testing.expectEqual(-3, d3.tabIndex); + + d3.setAttribute('tabindex', '+4'); + testing.expectEqual(4, d3.tabIndex); + + // Non-numeric values fall back to the element's default (-1 here). + d3.setAttribute('tabindex', 'foo'); + testing.expectEqual(-1, d3.tabIndex); + + d3.setAttribute('tabindex', ''); + testing.expectEqual(-1, d3.tabIndex); } diff --git a/src/browser/tests/element/html/marquee.html b/src/browser/tests/element/html/marquee.html new file mode 100644 index 00000000..12fb4d99 --- /dev/null +++ b/src/browser/tests/element/html/marquee.html @@ -0,0 +1,100 @@ + + + + + + + + + + + + diff --git a/src/browser/tests/element/html/param.html b/src/browser/tests/element/html/param.html new file mode 100644 index 00000000..12ef80b7 --- /dev/null +++ b/src/browser/tests/element/html/param.html @@ -0,0 +1,36 @@ + + + + + + + + diff --git a/src/browser/tests/element/html/style.html b/src/browser/tests/element/html/style.html index 5a5cb4be..45aeed4e 100644 --- a/src/browser/tests/element/html/style.html +++ b/src/browser/tests/element/html/style.html @@ -49,7 +49,7 @@