From ed3a464843f83caaf05cbe4d553026679d4fb492 Mon Sep 17 00:00:00 2001 From: Francis Bouvier Date: Sat, 9 Nov 2024 03:25:42 +0100 Subject: [PATCH 1/2] cdp: fix memory leak in msg parsing of the JSON Signed-off-by: Francis Bouvier --- src/cdp/browser.zig | 13 ++++++--- src/cdp/emulation.zig | 13 ++++++--- src/cdp/fetch.zig | 4 ++- src/cdp/log.zig | 4 ++- src/cdp/msg.zig | 62 +++++++++++++++++++++++++++++++++-------- src/cdp/network.zig | 7 +++-- src/cdp/page.zig | 19 +++++++++---- src/cdp/performance.zig | 4 ++- src/cdp/runtime.zig | 21 ++++++++++---- src/cdp/target.zig | 30 +++++++++++++------- 10 files changed, 131 insertions(+), 46 deletions(-) diff --git a/src/cdp/browser.zig b/src/cdp/browser.zig index 9a19c7a2..56017ac4 100644 --- a/src/cdp/browser.zig +++ b/src/cdp/browser.zig @@ -23,6 +23,7 @@ const Ctx = server.Ctx; const cdp = @import("cdp.zig"); const result = cdp.result; const IncomingMessage = @import("msg.zig").IncomingMessage; +const Input = @import("msg.zig").Input; const log = std.log.scoped(.cdp); @@ -62,7 +63,8 @@ fn getVersion( _: *Ctx, ) ![]const u8 { // input - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "browser.getVersion" }); // ouput @@ -89,7 +91,8 @@ fn setDownloadBehavior( downloadPath: ?[]const u8 = null, eventsEnabled: ?bool = null, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("REQ > id {d}, method {s}", .{ input.id, "browser.setDownloadBehavior" }); // output @@ -109,7 +112,8 @@ fn getWindowForTarget( const Params = struct { targetId: ?[]const u8 = null, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); std.debug.assert(input.sessionId != null); log.debug("Req > id {d}, method {s}", .{ input.id, "browser.getWindowForTarget" }); @@ -133,9 +137,10 @@ fn setWindowBounds( msg: *IncomingMessage, _: *Ctx, ) ![]const u8 { - const input = try msg.getInput(alloc, void); // input + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "browser.setWindowBounds" }); // output diff --git a/src/cdp/emulation.zig b/src/cdp/emulation.zig index c51e20f3..3fe75fb8 100644 --- a/src/cdp/emulation.zig +++ b/src/cdp/emulation.zig @@ -24,6 +24,7 @@ const cdp = @import("cdp.zig"); const result = cdp.result; const stringify = cdp.stringify; const IncomingMessage = @import("msg.zig").IncomingMessage; +const Input = @import("msg.zig").Input; const log = std.log.scoped(.cdp); @@ -67,7 +68,8 @@ fn setEmulatedMedia( media: ?[]const u8 = null, features: ?[]MediaFeature = null, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "emulation.setEmulatedMedia" }); // output @@ -84,7 +86,8 @@ fn setFocusEmulationEnabled( const Params = struct { enabled: bool, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "emulation.setFocusEmulationEnabled" }); // output @@ -98,7 +101,8 @@ fn setDeviceMetricsOverride( _: *Ctx, ) ![]const u8 { // input - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "emulation.setDeviceMetricsOverride" }); // output @@ -111,7 +115,8 @@ fn setTouchEmulationEnabled( msg: *IncomingMessage, _: *Ctx, ) ![]const u8 { - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "emulation.setTouchEmulationEnabled" }); return result(alloc, input.id, null, null, input.sessionId); diff --git a/src/cdp/fetch.zig b/src/cdp/fetch.zig index 5e54a13a..d56e4067 100644 --- a/src/cdp/fetch.zig +++ b/src/cdp/fetch.zig @@ -23,6 +23,7 @@ const Ctx = server.Ctx; const cdp = @import("cdp.zig"); const result = cdp.result; const IncomingMessage = @import("msg.zig").IncomingMessage; +const Input = @import("msg.zig").Input; const log = std.log.scoped(.cdp); @@ -50,7 +51,8 @@ fn disable( msg: *IncomingMessage, _: *Ctx, ) ![]const u8 { - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "fetch.disable" }); return result(alloc, input.id, null, null, input.sessionId); diff --git a/src/cdp/log.zig b/src/cdp/log.zig index 7ebcf241..d54487b2 100644 --- a/src/cdp/log.zig +++ b/src/cdp/log.zig @@ -23,6 +23,7 @@ const Ctx = server.Ctx; const cdp = @import("cdp.zig"); const result = cdp.result; const IncomingMessage = @import("msg.zig").IncomingMessage; +const Input = @import("msg.zig").Input; const stringify = cdp.stringify; const log_cdp = std.log.scoped(.cdp); @@ -50,7 +51,8 @@ fn enable( msg: *IncomingMessage, _: *Ctx, ) ![]const u8 { - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log_cdp.debug("Req > id {d}, method {s}", .{ input.id, "log.enable" }); return result(alloc, input.id, null, null, input.sessionId); diff --git a/src/cdp/msg.zig b/src/cdp/msg.zig index a50cfd4a..960c4b9b 100644 --- a/src/cdp/msg.zig +++ b/src/cdp/msg.zig @@ -136,8 +136,9 @@ pub const IncomingMessage = struct { } // getParams restart the JSON parsing - fn getParams(self: *IncomingMessage, alloc: std.mem.Allocator, T: type) !T { + fn getParams(self: *IncomingMessage, alloc: ?std.mem.Allocator, T: type) !T { if (T == void) return void{}; + std.debug.assert(alloc != null); // if T is not void, alloc should not be null if (self.params_skip) { // TODO if the params have been skipped, we have to retart the @@ -150,20 +151,56 @@ pub const IncomingMessage = struct { // parse "params" const options = std.json.ParseOptions{ .max_value_len = self.scanner.input.len, - .allocate = .alloc_if_needed, - }; - return try std.json.innerParse(T, alloc, &self.scanner, options); - } - - pub fn getInput(self: *IncomingMessage, alloc: std.mem.Allocator, T: type) !struct { id: u16, sessionId: ?[]const u8, params: T } { - return .{ - .params = try self.getParams(alloc, T), - .id = try self.getId(), - .sessionId = try self.getSessionId(), + .allocate = .alloc_always, }; + return try std.json.innerParse(T, alloc.?, &self.scanner, options); } }; +pub fn Input(T: type) type { + return struct { + arena: ?*std.heap.ArenaAllocator = null, + id: u16, + params: T, + sessionId: ?[]const u8, + + const Self = @This(); + + pub fn get(alloc: std.mem.Allocator, msg: *IncomingMessage) !Self { + var arena: ?*std.heap.ArenaAllocator = null; + var allocator: ?std.mem.Allocator = null; + + if (T != void) { + arena = try alloc.create(std.heap.ArenaAllocator); + arena.?.* = std.heap.ArenaAllocator.init(alloc); + allocator = arena.?.allocator(); + } + + errdefer { + if (arena) |_arena| { + _arena.deinit(); + alloc.destroy(_arena); + } + } + + return .{ + .arena = arena, + .params = try msg.getParams(allocator, T), + .id = try msg.getId(), + .sessionId = try msg.getSessionId(), + }; + } + + pub fn deinit(self: Self) void { + if (self.arena) |arena| { + const allocator = arena.child_allocator; + arena.deinit(); + allocator.destroy(arena); + } + } + }; +} + test "read incoming message" { const inputs = [_][]const u8{ \\{"id":1,"method":"foo","sessionId":"bar","params":{"bar":"baz"}} @@ -185,11 +222,12 @@ test "read incoming message" { try std.testing.expectEqualSlices(u8, "bar", (try msg.getSessionId()).?); const T = struct { bar: []const u8 }; - const in = msg.getInput(std.testing.allocator, T) catch |err| { + const in = Input(T).get(std.testing.allocator, &msg) catch |err| { if (err != error.SkippedParams) return err; // TODO remove this check when params in the beginning is handled. continue; }; + defer in.deinit(); try std.testing.expectEqualSlices(u8, "baz", in.params.bar); } } diff --git a/src/cdp/network.zig b/src/cdp/network.zig index 7a7ffe79..c7c599a6 100644 --- a/src/cdp/network.zig +++ b/src/cdp/network.zig @@ -23,6 +23,7 @@ const Ctx = server.Ctx; const cdp = @import("cdp.zig"); const result = cdp.result; const IncomingMessage = @import("msg.zig").IncomingMessage; +const Input = @import("msg.zig").Input; const log = std.log.scoped(.cdp); @@ -52,7 +53,8 @@ fn enable( _: *Ctx, ) ![]const u8 { // input - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "network.enable" }); return result(alloc, input.id, null, null, input.sessionId); @@ -65,7 +67,8 @@ fn setCacheDisabled( _: *Ctx, ) ![]const u8 { // input - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "network.setCacheDisabled" }); return result(alloc, input.id, null, null, input.sessionId); diff --git a/src/cdp/page.zig b/src/cdp/page.zig index 00870234..b2beb203 100644 --- a/src/cdp/page.zig +++ b/src/cdp/page.zig @@ -25,6 +25,7 @@ const result = cdp.result; const stringify = cdp.stringify; const sendEvent = cdp.sendEvent; const IncomingMessage = @import("msg.zig").IncomingMessage; +const Input = @import("msg.zig").Input; const log = std.log.scoped(.cdp); @@ -63,7 +64,8 @@ fn enable( _: *Ctx, ) ![]const u8 { // input - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "page.enable" }); return result(alloc, input.id, null, null, input.sessionId); @@ -90,7 +92,8 @@ fn getFrameTree( ctx: *Ctx, ) ![]const u8 { // input - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "page.getFrameTree" }); // output @@ -142,7 +145,8 @@ fn setLifecycleEventsEnabled( const Params = struct { enabled: bool, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "page.setLifecycleEventsEnabled" }); ctx.state.page_life_cycle_events = true; @@ -171,7 +175,8 @@ fn addScriptToEvaluateOnNewDocument( includeCommandLineAPI: bool = false, runImmediately: bool = false, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "page.addScriptToEvaluateOnNewDocument" }); // output @@ -205,7 +210,8 @@ fn createIsolatedWorld( worldName: []const u8, grantUniveralAccess: bool, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); std.debug.assert(input.sessionId != null); log.debug("Req > id {d}, method {s}", .{ input.id, "page.createIsolatedWorld" }); @@ -247,7 +253,8 @@ fn navigate( frameId: ?[]const u8 = null, referrerPolicy: ?[]const u8 = null, // TODO: enum }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); std.debug.assert(input.sessionId != null); log.debug("Req > id {d}, method {s}", .{ input.id, "page.navigate" }); diff --git a/src/cdp/performance.zig b/src/cdp/performance.zig index ebb602d5..bf210c36 100644 --- a/src/cdp/performance.zig +++ b/src/cdp/performance.zig @@ -23,6 +23,7 @@ const Ctx = server.Ctx; const cdp = @import("cdp.zig"); const result = cdp.result; const IncomingMessage = @import("msg.zig").IncomingMessage; +const Input = @import("msg.zig").Input; const log = std.log.scoped(.cdp); @@ -50,7 +51,8 @@ fn enable( _: *Ctx, ) ![]const u8 { // input - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "performance.enable" }); return result(alloc, input.id, null, null, input.sessionId); diff --git a/src/cdp/runtime.zig b/src/cdp/runtime.zig index 03f1e068..122d0a6d 100644 --- a/src/cdp/runtime.zig +++ b/src/cdp/runtime.zig @@ -26,6 +26,7 @@ const Ctx = server.Ctx; const cdp = @import("cdp.zig"); const result = cdp.result; const IncomingMessage = @import("msg.zig").IncomingMessage; +const Input = @import("msg.zig").Input; const stringify = cdp.stringify; const log = std.log.scoped(.cdp); @@ -77,9 +78,13 @@ fn sendInspector( userGesture: ?bool = null, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s} (script saved on cache)", .{ input.id, "runtime.evaluate" }); - script = input.params.expression; + const params = input.params; + const func = try alloc.alloc(u8, params.expression.len); + @memcpy(func, params.expression); + script = func; id = input.id; } else if (method == .callFunctionOn) { const Params = struct { @@ -95,14 +100,19 @@ fn sendInspector( userGesture: ?bool = null, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s} (script saved on cache)", .{ input.id, "runtime.callFunctionOn" }); - script = input.params.functionDeclaration; + const params = input.params; + const func = try alloc.alloc(u8, params.functionDeclaration.len); + @memcpy(func, params.functionDeclaration); + script = func; id = input.id; } if (script) |src| { try cdp.dumpFile(alloc, id, src); + alloc.free(src); } } @@ -163,7 +173,8 @@ fn runIfWaitingForDebugger( msg: *IncomingMessage, _: *Ctx, ) ![]const u8 { - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "runtime.runIfWaitingForDebugger" }); return result(alloc, input.id, null, null, input.sessionId); diff --git a/src/cdp/target.zig b/src/cdp/target.zig index 10263d52..f65c4ac3 100644 --- a/src/cdp/target.zig +++ b/src/cdp/target.zig @@ -24,6 +24,7 @@ const cdp = @import("cdp.zig"); const result = cdp.result; const stringify = cdp.stringify; const IncomingMessage = @import("msg.zig").IncomingMessage; +const Input = @import("msg.zig").Input; const log = std.log.scoped(.cdp); @@ -72,7 +73,8 @@ fn setDiscoverTargets( _: *Ctx, ) ![]const u8 { // input - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "target.setDiscoverTargets" }); // output @@ -111,7 +113,8 @@ fn setAutoAttach( flatten: bool = true, filter: ?[]TargetFilter = null, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "target.setAutoAttach" }); // attachedToTarget event @@ -144,7 +147,8 @@ fn attachToTarget( targetId: []const u8, flatten: bool = true, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "target.attachToTarget" }); // attachedToTarget event @@ -180,7 +184,8 @@ fn getTargetInfo( const Params = struct { targetId: ?[]const u8 = null, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "target.getTargetInfo" }); // output @@ -213,7 +218,8 @@ fn getBrowserContexts( ctx: *Ctx, ) ![]const u8 { // input - const input = try msg.getInput(alloc, void); + const input = try Input(void).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "target.getBrowserContexts" }); // ouptut @@ -246,7 +252,8 @@ fn createBrowserContext( proxyBypassList: ?[]const u8 = null, originsWithUniversalNetworkAccess: ?[][]const u8 = null, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "target.createBrowserContext" }); ctx.state.contextID = ContextID; @@ -279,7 +286,8 @@ fn disposeBrowserContext( const Params = struct { browserContextId: []const u8, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "target.disposeBrowserContext" }); // output @@ -309,7 +317,8 @@ fn createTarget( background: bool = false, forTab: ?bool = null, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "target.createTarget" }); // change CDP state @@ -326,7 +335,7 @@ fn createTarget( .targetId = ctx.state.frameID, .title = "", .url = ctx.state.url, - .browserContextId = msg.params.?.browserContextId orelse ContextID, + .browserContextId = input.params.browserContextId orelse ContextID, }, .waitingForDebugger = true, }; @@ -360,7 +369,8 @@ fn closeTarget( const Params = struct { targetId: []const u8, }; - const input = try msg.getInput(alloc, Params); + const input = try Input(Params).get(alloc, msg); + defer input.deinit(); log.debug("Req > id {d}, method {s}", .{ input.id, "target.closeTarget" }); // output From 8a25545cacf1682b1bdbe719da05ef614d8eeda1 Mon Sep 17 00:00:00 2001 From: Francis Bouvier Date: Sat, 9 Nov 2024 13:34:15 +0100 Subject: [PATCH 2/2] memory: use a GPA in Debug mode and a page allocator in Release Signed-off-by: Francis Bouvier --- src/main.zig | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/main.zig b/src/main.zig index 2bfe8c50..d7c3143d 100644 --- a/src/main.zig +++ b/src/main.zig @@ -158,12 +158,27 @@ fn printUsageExit(execname: []const u8, res: u8) void { pub fn main() !void { // allocator - var arena = std.heap.ArenaAllocator.init(std.heap.page_allocator); - defer arena.deinit(); - const alloc = arena.allocator(); + // - in Debug mode we use the General Purpose Allocator to detect memory leaks + // - in Release mode we use the page allocator + var alloc: std.mem.Allocator = undefined; + var _gpa: ?std.heap.GeneralPurposeAllocator(.{}) = null; + if (builtin.mode == .Debug) { + _gpa = std.heap.GeneralPurposeAllocator(.{}){}; + alloc = _gpa.?.allocator(); + } else { + alloc = std.heap.page_allocator; + } + defer { + if (_gpa) |*gpa| { + switch (gpa.deinit()) { + .ok => std.debug.print("No memory leaks\n", .{}), + .leak => @panic("Memory leak"), + } + } + } // args - var args = try std.process.argsWithAllocator(arena.allocator()); + var args = try std.process.argsWithAllocator(alloc); defer args.deinit(); const execname = args.next().?; @@ -263,7 +278,7 @@ pub fn main() !void { std.log.info("Listening on: {s}:{d}...", .{ host, port }); // loop - var loop = try jsruntime.Loop.init(arena.allocator()); + var loop = try jsruntime.Loop.init(alloc); defer loop.deinit(); // listen @@ -279,7 +294,7 @@ pub fn main() !void { const vm = jsruntime.VM.init(); defer vm.deinit(); - var loop = try jsruntime.Loop.init(arena.allocator()); + var loop = try jsruntime.Loop.init(alloc); defer loop.deinit(); var browser = Browser{};