macho: fix memory leaks when emptying TextBlocks

This happens on every call to `TextBlock.empty` by the `Module`.

3 files changed, 42 insertions, 29 deletions

diff --git a/src/link/MachO.zig b/src/link/MachO.zig
index 6481bfa847..d1d25a313c 100644
--- a/src/link/MachO.zig
+++ b/src/link/MachO.zig
@@ -1828,7 +1828,7 @@ fn writeTextBlocks(self: *MachO) !void {
                 });
 
                 try block.resolveRelocs(self);
-                mem.copy(u8, code[aligned_base_off..][0..block.size], block.code);
+                mem.copy(u8, code[aligned_base_off..][0..block.size], block.code.items);
 
                 // TODO NOP for machine code instead of just zeroing out
                 const padding_len = aligned_base_off - base_off;
@@ -2262,6 +2262,7 @@ fn resolveSymbols(self: *MachO) !void {
 
         const size = sym.n_value;
         const code = try self.base.allocator.alloc(u8, size);
+        defer self.base.allocator.free(code);
         mem.set(u8, code, 0);
         const alignment = (sym.n_desc >> 8) & 0x0f;
 
@@ -2287,11 +2288,12 @@ fn resolveSymbols(self: *MachO) !void {
         const block = try self.base.allocator.create(TextBlock);
         block.* = TextBlock.empty;
         block.local_sym_index = local_sym_index;
-        block.code = code;
         block.size = size;
         block.alignment = alignment;
         try self.managed_blocks.append(self.base.allocator, block);
 
+        try block.code.appendSlice(self.base.allocator, code);
+
         // Update target section's metadata
         // TODO should we update segment's size here too?
         // How does it tie with incremental space allocs?
@@ -2428,7 +2430,6 @@ fn resolveSymbols(self: *MachO) !void {
         const block = try self.base.allocator.create(TextBlock);
         block.* = TextBlock.empty;
         block.local_sym_index = local_sym_index;
-        block.code = try self.base.allocator.alloc(u8, 0);
         block.size = 0;
         block.alignment = 0;
         try self.managed_blocks.append(self.base.allocator, block);
@@ -3527,7 +3528,13 @@ pub fn updateFunc(self: *MachO, module: *Module, func: *Module.Fn, air: Air, liv
         try codegen.generateFunction(&self.base, decl.srcLoc(), func, air, liveness, &code_buffer, .none);
     switch (res) {
         .appended => {
-            decl.link.macho.code = code_buffer.toOwnedSlice();
+            // TODO clearing the code and relocs buffer should probably be orchestrated
+            // in a different, smarter, more automatic way somewhere else, in a more centralised
+            // way than this.
+            // If we don't clear the buffers here, we are up for some nasty surprises when
+            // this TextBlock is reused later on and was not freed by freeTextBlock().
+            decl.link.macho.code.clearAndFree(self.base.allocator);
+            try decl.link.macho.code.appendSlice(self.base.allocator, code_buffer.items);
         },
         .fail => |em| {
             decl.analysis = .codegen_failure;
@@ -3536,9 +3543,9 @@ pub fn updateFunc(self: *MachO, module: *Module, func: *Module.Fn, air: Air, liv
         },
     }
 
-    const symbol = try self.placeDecl(decl, decl.link.macho.code.len);
+    const symbol = try self.placeDecl(decl, decl.link.macho.code.items.len);
 
-    try self.writeCode(symbol, decl.link.macho.code);
+    try self.writeCode(symbol, decl.link.macho.code.items);
 
     if (debug_buffers) |db| {
         try self.d_sym.?.commitDeclDebugInfo(
@@ -3613,8 +3620,14 @@ pub fn updateDecl(self: *MachO, module: *Module, decl: *Module.Decl) !void {
         switch (res) {
             .externally_managed => |x| break :blk x,
             .appended => {
-                decl.link.macho.code = code_buffer.toOwnedSlice();
-                break :blk decl.link.macho.code;
+                // TODO clearing the code and relocs buffer should probably be orchestrated
+                // in a different, smarter, more automatic way somewhere else, in a more centralised
+                // way than this.
+                // If we don't clear the buffers here, we are up for some nasty surprises when
+                // this TextBlock is reused later on and was not freed by freeTextBlock().
+                decl.link.macho.code.clearAndFree(self.base.allocator);
+                try decl.link.macho.code.appendSlice(self.base.allocator, code_buffer.items);
+                break :blk decl.link.macho.code.items;
             },
             .fail => |em| {
                 decl.analysis = .codegen_failure;
@@ -3705,7 +3718,7 @@ fn placeDecl(self: *MachO, decl: *Module.Decl, code_len: usize) !*macho.nlist_64
     try decl.link.macho.resolveRelocs(self);
     // TODO this requires further investigation: should we dispose of resolved relocs, or keep them
     // so that we can reapply them when moving/growing sections?
-    decl.link.macho.relocs.clearRetainingCapacity();
+    decl.link.macho.relocs.clearAndFree(self.base.allocator);
 
     // Apply pending updates
     while (self.pending_updates.popOrNull()) |update| {
diff --git a/src/link/MachO/Object.zig b/src/link/MachO/Object.zig
index 7291e0929c..46d82dec31 100644
--- a/src/link/MachO/Object.zig
+++ b/src/link/MachO/Object.zig
@@ -726,11 +726,12 @@ pub fn parseTextBlocks(self: *Object, macho_file: *MachO) !void {
             const block = try macho_file.base.allocator.create(TextBlock);
             block.* = TextBlock.empty;
             block.local_sym_index = block_local_sym_index;
-            block.code = try self.allocator.dupe(u8, code);
             block.size = sect.size;
             block.alignment = sect.@"align";
             try macho_file.managed_blocks.append(macho_file.base.allocator, block);
 
+            try block.code.appendSlice(macho_file.base.allocator, code);
+
             try block.parseRelocsFromObject(self.allocator, relocs, self, .{
                 .base_addr = 0,
                 .macho_file = macho_file,
diff --git a/src/link/MachO/TextBlock.zig b/src/link/MachO/TextBlock.zig
index e7c0139623..9dc02d1f4d 100644
--- a/src/link/MachO/TextBlock.zig
+++ b/src/link/MachO/TextBlock.zig
@@ -30,7 +30,7 @@ aliases: std.ArrayListUnmanaged(u32) = .{},
 contained: std.ArrayListUnmanaged(SymbolAtOffset) = .{},
 
 /// Code (may be non-relocated) this block represents
-code: []u8,
+code: std.ArrayListUnmanaged(u8) = .{},
 
 /// Size and alignment of this text block
 /// Unlike in Elf, we need to store the size of this symbol as part of
@@ -196,9 +196,9 @@ pub const Relocation = struct {
             };
 
             if (self.is_64bit) {
-                mem.writeIntLittle(u64, args.block.code[args.offset..][0..8], @bitCast(u64, result));
+                mem.writeIntLittle(u64, args.block.code.items[args.offset..][0..8], @bitCast(u64, result));
             } else {
-                mem.writeIntLittle(u32, args.block.code[args.offset..][0..4], @truncate(u32, @bitCast(u64, result)));
+                mem.writeIntLittle(u32, args.block.code.items[args.offset..][0..4], @truncate(u32, @bitCast(u64, result)));
             }
         }
 
@@ -226,7 +226,7 @@ pub const Relocation = struct {
                         i28,
                         @intCast(i64, args.target_addr) - @intCast(i64, args.source_addr),
                     );
-                    const code = args.block.code[args.offset..][0..4];
+                    const code = args.block.code.items[args.offset..][0..4];
                     var inst = aarch64.Instruction{
                         .unconditional_branch_immediate = mem.bytesToValue(meta.TagPayload(
                             aarch64.Instruction,
@@ -241,7 +241,7 @@ pub const Relocation = struct {
                         i32,
                         @intCast(i64, args.target_addr) - @intCast(i64, args.source_addr) - 4,
                     );
-                    mem.writeIntLittle(u32, args.block.code[args.offset..][0..4], @bitCast(u32, displacement));
+                    mem.writeIntLittle(u32, args.block.code.items[args.offset..][0..4], @bitCast(u32, displacement));
                 },
                 else => return error.UnsupportedCpuArchitecture,
             }
@@ -269,7 +269,7 @@ pub const Relocation = struct {
             const target_page = @intCast(i32, target_addr >> 12);
             const pages = @bitCast(u21, @intCast(i21, target_page - source_page));
 
-            const code = args.block.code[args.offset..][0..4];
+            const code = args.block.code.items[args.offset..][0..4];
             var inst = aarch64.Instruction{
                 .pc_relative_address = mem.bytesToValue(meta.TagPayload(
                     aarch64.Instruction,
@@ -315,7 +315,7 @@ pub const Relocation = struct {
         };
 
         pub fn resolve(self: PageOff, args: ResolveArgs) !void {
-            const code = args.block.code[args.offset..][0..4];
+            const code = args.block.code.items[args.offset..][0..4];
 
             switch (self.kind) {
                 .page => {
@@ -445,7 +445,7 @@ pub const Relocation = struct {
     pub const PointerToGot = struct {
         pub fn resolve(_: PointerToGot, args: ResolveArgs) !void {
             const result = try math.cast(i32, @intCast(i64, args.target_addr) - @intCast(i64, args.source_addr));
-            mem.writeIntLittle(u32, args.block.code[args.offset..][0..4], @bitCast(u32, result));
+            mem.writeIntLittle(u32, args.block.code.items[args.offset..][0..4], @bitCast(u32, result));
         }
 
         pub fn format(self: PointerToGot, comptime fmt: []const u8, options: std.fmt.FormatOptions, writer: anytype) !void {
@@ -466,7 +466,7 @@ pub const Relocation = struct {
                 i32,
                 target_addr - @intCast(i64, args.source_addr) - self.correction - 4,
             );
-            mem.writeIntLittle(u32, args.block.code[args.offset..][0..4], @bitCast(u32, displacement));
+            mem.writeIntLittle(u32, args.block.code.items[args.offset..][0..4], @bitCast(u32, displacement));
         }
 
         pub fn format(self: Signed, comptime fmt: []const u8, options: std.fmt.FormatOptions, writer: anytype) !void {
@@ -489,13 +489,13 @@ pub const Relocation = struct {
         pub fn resolve(self: Load, args: ResolveArgs) !void {
             if (self.kind == .tlvp) {
                 // We need to rewrite the opcode from movq to leaq.
-                args.block.code[args.offset - 2] = 0x8d;
+                args.block.code.items[args.offset - 2] = 0x8d;
             }
             const displacement = try math.cast(
                 i32,
                 @intCast(i64, args.target_addr) - @intCast(i64, args.source_addr) - 4 + self.addend,
             );
-            mem.writeIntLittle(u32, args.block.code[args.offset..][0..4], @bitCast(u32, displacement));
+            mem.writeIntLittle(u32, args.block.code.items[args.offset..][0..4], @bitCast(u32, displacement));
         }
 
         pub fn format(self: Load, comptime fmt: []const u8, options: std.fmt.FormatOptions, writer: anytype) !void {
@@ -542,7 +542,6 @@ pub const Relocation = struct {
 
 pub const empty = TextBlock{
     .local_sym_index = 0,
-    .code = undefined,
     .size = 0,
     .alignment = 0,
     .prev = null,
@@ -560,7 +559,7 @@ pub fn deinit(self: *TextBlock, allocator: *Allocator) void {
     self.relocs.deinit(allocator);
     self.contained.deinit(allocator);
     self.aliases.deinit(allocator);
-    allocator.free(self.code);
+    self.code.deinit(allocator);
 }
 
 /// Returns how much room there is to grow in virtual address space.
@@ -914,9 +913,9 @@ fn parseUnsigned(
     };
 
     var addend: i64 = if (is_64bit)
-        mem.readIntLittle(i64, self.code[out.offset..][0..8])
+        mem.readIntLittle(i64, self.code.items[out.offset..][0..8])
     else
-        mem.readIntLittle(i32, self.code[out.offset..][0..4]);
+        mem.readIntLittle(i32, self.code.items[out.offset..][0..4]);
 
     if (rel.r_extern == 0) {
         assert(out.where == .local);
@@ -970,7 +969,7 @@ fn parsePageOff(self: TextBlock, rel: macho.relocation_info, out: *Relocation, a
     const rel_type = @intToEnum(macho.reloc_type_arm64, rel.r_type);
     const op_kind: ?Relocation.PageOff.OpKind = blk: {
         if (rel_type != .ARM64_RELOC_PAGEOFF12) break :blk null;
-        const op_kind: Relocation.PageOff.OpKind = if (isArithmeticOp(self.code[out.offset..][0..4]))
+        const op_kind: Relocation.PageOff.OpKind = if (isArithmeticOp(self.code.items[out.offset..][0..4]))
             .arithmetic
         else
             .load;
@@ -1013,7 +1012,7 @@ fn parseSigned(self: TextBlock, rel: macho.relocation_info, out: *Relocation, ct
         .X86_64_RELOC_SIGNED_4 => 4,
         else => unreachable,
     };
-    var addend: i64 = mem.readIntLittle(i32, self.code[out.offset..][0..4]) + correction;
+    var addend: i64 = mem.readIntLittle(i32, self.code.items[out.offset..][0..4]) + correction;
 
     if (rel.r_extern == 0) {
         const source_sym = ctx.macho_file.locals.items[self.local_sym_index];
@@ -1038,7 +1037,7 @@ fn parseLoad(self: TextBlock, rel: macho.relocation_info, out: *Relocation) void
 
     const rel_type = @intToEnum(macho.reloc_type_x86_64, rel.r_type);
     const addend: i32 = if (rel_type == .X86_64_RELOC_GOT)
-        mem.readIntLittle(i32, self.code[out.offset..][0..4])
+        mem.readIntLittle(i32, self.code.items[out.offset..][0..4])
     else
         0;
 
@@ -1173,7 +1172,7 @@ pub fn print_this(self: *const TextBlock, macho_file: MachO) void {
             }
         }
     }
-    log.warn("  code.len = {}", .{self.code.len});
+    log.warn("  code.len = {}", .{self.code.items.len});
     if (self.relocs.items.len > 0) {
         log.warn("  relocations:", .{});
         for (self.relocs.items) |rel| {