From f61e9b013f2b881de15cdae6bf19aa08efc5eef4 Mon Sep 17 00:00:00 2001 From: Luna Date: Tue, 18 Aug 2020 20:40:06 -0300 Subject: [PATCH 1/5] make CommandList free its command pointers --- src/lang.zig | 24 +++++++++++++++++++++++- src/main.zig | 7 +++---- src/printer.zig | 2 +- src/runner.zig | 2 +- 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/lang.zig b/src/lang.zig index 3d33909..80450f3 100644 --- a/src/lang.zig +++ b/src/lang.zig @@ -474,7 +474,29 @@ pub const Command = struct { }); }; -pub const CommandList = std.ArrayList(*Command); +const CmdArrayList = std.ArrayList(*Command); + +pub const CommandList = struct { + list: CmdArrayList, + const Self = @This(); + + pub fn init(allocator: *std.mem.Allocator) Self { + return .{ + .list = CmdArrayList.init(allocator), + }; + } + + pub fn deinit(self: *Self) void { + for (self.list.items) |cmd_ptr| { + self.list.allocator.destroy(cmd_ptr); + } + self.list.deinit(); + } + + pub fn append(self: *Self, cmd: *Command) !void { + return try self.list.append(cmd); + } +}; /// A parser. pub const Lang = struct { diff --git a/src/main.zig b/src/main.zig index a53c1fb..119009e 100644 --- a/src/main.zig +++ b/src/main.zig @@ -65,7 +65,7 @@ pub fn doRepl(allocator: *std.mem.Allocator, args_it: anytype) !void { defer existing_cmds.deinit(); // copy the existing command list into the repl's command list - for (existing_cmds.items) |existing_cmd| { + for (existing_cmds.list.items) |existing_cmd| { try cmds.append(existing_cmd); } } else { @@ -210,9 +210,8 @@ pub fn doRepl(allocator: *std.mem.Allocator, args_it: anytype) !void { }; // no command? ignore! - if (cmds_parsed.items.len == 0) continue; - - current = cmds_parsed.items[0].*; + if (cmds_parsed.list.items.len == 0) continue; + current = cmds_parsed.list.items[0].*; // by cloning the parent runner, we can iteratively write // whatever command we want and only commit the good results diff --git a/src/printer.zig b/src/printer.zig index 69cc4b5..5ea5a7b 100644 --- a/src/printer.zig +++ b/src/printer.zig @@ -31,7 +31,7 @@ fn printCommand(stream: anytype, cmd: *langs.Command, comptime tag: langs.Comman } pub fn printList(list: langs.CommandList, stream: anytype) !void { - for (list.items) |cmd| { + for (list.list.items) |cmd| { const command = @tagName(cmd.tag); try stream.print("{}", .{command}); diff --git a/src/runner.zig b/src/runner.zig index a9ea3e3..37fade3 100644 --- a/src/runner.zig +++ b/src/runner.zig @@ -312,7 +312,7 @@ pub const Runner = struct { cmds: lang.CommandList, debug_flag: bool, ) !void { - for (cmds.items) |cmd| { + for (cmds.list.items) |cmd| { cmd.print(); try self.runCommand(cmd.*); } From 708ef45600b244fb8836000249848c3a561b1efb Mon Sep 17 00:00:00 2001 From: Luna Date: Tue, 18 Aug 2020 20:41:57 -0300 Subject: [PATCH 2/5] fix use-after-free on repl --- src/main.zig | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main.zig b/src/main.zig index 119009e..15e1524 100644 --- a/src/main.zig +++ b/src/main.zig @@ -60,9 +60,11 @@ pub fn doRepl(allocator: *std.mem.Allocator, args_it: anytype) !void { var scri_existing = try allocator.alloc(u8, total_bytes); _ = try file_read_opt.?.read(scri_existing); - // we can defer this because we copy the Command structs back to cmds + // we can't defer this directly because we copy the + // Command pointers to the cmds list. running deinit() directly + // would cause those pointers to be freed. var existing_cmds = try lang.parse(scri_existing); - defer existing_cmds.deinit(); + defer existing_cmds.list.deinit(); // copy the existing command list into the repl's command list for (existing_cmds.list.items) |existing_cmd| { From 0cd47be7ac9a398f3c9e4d44fb33132867644c2e Mon Sep 17 00:00:00 2001 From: Luna Date: Tue, 18 Aug 2020 20:53:54 -0300 Subject: [PATCH 3/5] remove need to dupe strings to command structs --- src/lang.zig | 7 +++---- src/main.zig | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/lang.zig b/src/lang.zig index 80450f3..c80af14 100644 --- a/src/lang.zig +++ b/src/lang.zig @@ -157,7 +157,7 @@ pub const Command = struct { pub const Load = struct { pub const base_tag = Tag.load; base: Command, - path: []u8, + path: []const u8, }; pub const Quicksave = struct { @@ -568,7 +568,7 @@ pub const Lang = struct { f32 => try std.fmt.parseFloat(f32, arg), u64 => try std.fmt.parseInt(u64, arg, 10), usize => try std.fmt.parseInt(usize, arg, 10), - []const u8 => try self.allocator.dupe(u8, arg), + []const u8 => arg, else => @compileError("parameter struct has unsupported type " ++ @typeName(cmd_field.field_type)), }; @@ -586,8 +586,7 @@ pub const Lang = struct { usize => try std.fmt.parseInt(usize, arg, 10), i32 => try std.fmt.parseInt(i32, arg, 10), f32 => try std.fmt.parseFloat(f32, arg), - []u8 => try self.allocator.dupe(u8, arg), - []const u8 => try self.allocator.dupe(u8, arg), + []const u8 => arg, else => @compileError("Invalid parameter type (" ++ @typeName(cmd_field.field_type) ++ ") left on command struct " ++ @typeName(command_struct) ++ "."), }; diff --git a/src/main.zig b/src/main.zig index 15e1524..4fd6525 100644 --- a/src/main.zig +++ b/src/main.zig @@ -77,7 +77,7 @@ pub fn doRepl(allocator: *std.mem.Allocator, args_it: anytype) !void { // TODO: deliberate memleak here. we only allocate this // command once, for the start of the file, so. var load_cmd = try allocator.create(langs.Command.Load); - std.mem.copy(u8, load_cmd.path, ":0"); + load_cmd.path = ":0"; load_cmd.base.tag = langs.Command.Tag.load; // taking address is fine, because load_cmd lives in the lifetime From 9527426d484d680f4db373c72f98fe1e2c35ef7d Mon Sep 17 00:00:00 2001 From: Luna Date: Tue, 18 Aug 2020 20:54:04 -0300 Subject: [PATCH 4/5] remove memleak when getting args --- src/main.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main.zig b/src/main.zig index 4fd6525..962684b 100644 --- a/src/main.zig +++ b/src/main.zig @@ -242,9 +242,9 @@ pub fn main() !void { var args_it = std.process.args(); // TODO print help - - _ = try (args_it.next(allocator) orelse @panic("expected exe name")); + _ = args_it.skip(); const scri_path = try (args_it.next(allocator) orelse @panic("expected scri path or 'repl'")); + defer allocator.free(scri_path); if (std.mem.eql(u8, scri_path, "repl")) { return try doRepl(allocator, &args_it); From 77cceab2886f2b117b4ee4683323067a12ef9fe5 Mon Sep 17 00:00:00 2001 From: Luna Date: Tue, 18 Aug 2020 20:57:33 -0300 Subject: [PATCH 5/5] make Image free its paths --- src/image.zig | 5 +++-- src/runner.zig | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/image.zig b/src/image.zig index 2d40b97..51c1eef 100644 --- a/src/image.zig +++ b/src/image.zig @@ -179,8 +179,9 @@ pub const Image = struct { } pub fn close(self: *Image) void { - //self.allocator.free(self.path); - //self.allocator.free(self.curpath); + self.allocator.free(self.path); + self.allocator.free(self.curpath); + var st: i32 = c.sf_close(self.sndfile); if (st != 0) { diff --git a/src/runner.zig b/src/runner.zig index 37fade3..47f2cab 100644 --- a/src/runner.zig +++ b/src/runner.zig @@ -78,6 +78,7 @@ pub const Runner = struct { } } + // Caller owns returned memory. fn resolveArgPath(self: *Runner, path_or_argidx: []const u8) ![]const u8 { const path = try self.resolveArg(path_or_argidx); const resolved_path = try std.fs.path.resolve( @@ -89,7 +90,7 @@ pub const Runner = struct { } fn loadCmd(self: *Runner, path_or_argidx: []const u8) !void { - var load_path = try self.resolveArgPath(path_or_argidx); + const load_path = try self.resolveArgPath(path_or_argidx); std.debug.warn("\tload path: {}\n", .{load_path}); // we could use ImageMagick to convert from X to BMP