Fix leaks and double-frees in Kym VM environment

source/coral/io.zig

@@ -7,6 +7,7 @@ pub const AllocationError = error {
 };
 
 pub const AllocationOptions = struct {
+	return_address: usize,
 	allocation: ?[]u8 = null,
 	size: usize,
 };
@@ -181,9 +182,10 @@ pub fn allocate_many(allocator: Allocator, amount: usize, comptime Type: type) A
 		@compileError("Cannot allocate memory for 0-byte type " ++ @typeName(Type));
 	}
 
-	return @ptrCast([*]Type, @alignCast(@alignOf(Type), allocator.invoke(.{.size = @sizeOf(Type) * amount}) orelse {
-		return error.OutOfMemory;
-	}))[0 .. amount];
+	return @ptrCast([*]Type, @alignCast(@alignOf(Type), allocator.invoke(.{
+		.size = @sizeOf(Type) * amount,
+		.return_address = @returnAddress(),
+	}) orelse return error.OutOfMemory))[0 .. amount];
 }
 
 pub fn allocate_one(allocator: Allocator, value: anytype) AllocationError!*@TypeOf(value) {
@@ -193,9 +195,10 @@ pub fn allocate_one(allocator: Allocator, value: anytype) AllocationError!*@Type
 		@compileError("Cannot allocate memory for 0-byte type " ++ @typeName(Type));
 	}
 
-	const allocation = @ptrCast(*Type, @alignCast(@alignOf(Type), allocator.invoke(.{.size = @sizeOf(Type)}) orelse {
-		return error.OutOfMemory;
-	}));
+	const allocation = @ptrCast(*Type, @alignCast(@alignOf(Type), allocator.invoke(.{
+		.size = @sizeOf(Type),
+		.return_address = @returnAddress(),
+	}) orelse return error.OutOfMemory));
 
 	allocation.* = value;
 
@@ -241,6 +244,7 @@ pub fn deallocate(allocator: Allocator, allocation: anytype) void {
 					.Many, .C => @compileError("length of allocation must be known to deallocate"),
 				},
 
+				.return_address = @returnAddress(),
 				.size = 0,
 			});
 		},

source/coral/list.zig

@@ -52,7 +52,7 @@ pub fn Stack(comptime Value: type) type {
 				return;
 			}
 
-			io.deallocate(allocator, self.values);
+			io.deallocate(allocator, self.values.ptr[0 .. self.capacity]);
 
 			self.values = &.{};
 			self.capacity = 0;
@@ -132,7 +132,7 @@ pub fn Stack(comptime Value: type) type {
 		pub fn push_all(self: *Self, allocator: io.Allocator, values: []const Value) io.AllocationError!void {
 			const new_length = self.values.len + values.len;
 
-			if (new_length >= self.capacity) {
+			if (new_length > self.capacity) {
 				try self.grow(allocator, values.len + values.len);
 			}
 

source/ona/heap.zig

@@ -7,7 +7,11 @@ const std = @import("std");
 ///
 ///
 ///
-const stack_trace_frame_size = if (std.debug.sys_can_stack_trace) 4 else 0;
+const AllocationInfo = struct {
+	trace: std.debug.Trace,
+	next_info: ?*AllocationInfo,
+	size: usize,
+};
 
 ///
 ///
@@ -16,19 +20,16 @@ const Context = struct {
 	allocation_info_head: ?*AllocationInfo = null,
 
 	///
+	/// Attempts to allocate a buffer of `size` length from `self`, with `return_address` as the location of the
+	/// allocation request origin.
 	///
+	/// A reference to the allocated buffer is returned via a slice if the allocation was successful, otherwise `null`
+	/// is returned.
 	///
-	const AllocationInfo = struct {
-		stack_frames: [stack_trace_frame_size]usize,
-		stack_trace: std.builtin.StackTrace,
-		next_info: ?*AllocationInfo,
-		size: usize,
-	};
-
+	/// *Note* the returned buffer must be deallocated with [deallocate] before program exit or it will cause a memory
+	/// leak.
 	///
-	///
-	///
-	fn allocate(self: *Context, size: usize) ?[]u8 {
+	fn allocate(self: *Context, size: usize, return_address: usize) ?[]u8 {
 		const allocation_info_size = @sizeOf(AllocationInfo);
 		const total_allocation_size = allocation_info_size + size;
 		const allocation = ext.SDL_malloc(total_allocation_size) orelse return null;
@@ -37,15 +38,10 @@ const Context = struct {
 		allocation_info.* = .{
 			.size = size,
 			.next_info = self.allocation_info_head,
-			.stack_frames = [_]usize{0} ** stack_trace_frame_size,
-
-			.stack_trace = .{
-				.instruction_addresses = &allocation_info.stack_frames,
-				.index = 0,
-			},
+			.trace = .{},
 		};
 
-		std.debug.captureStackTrace(null, &allocation_info.stack_trace);
+		allocation_info.trace.addAddr(return_address, "");
 
 		self.allocation_info_head = allocation_info;
 
@@ -53,7 +49,10 @@ const Context = struct {
 	}
 
 	///
+	/// Deallocates a the allocation buffer referenced by `allocation`.
 	///
+	/// *Note* the pointer and length of `allocation` must match valid values known to `allocator` otherwise safety-
+	/// checked behavior will occur.
 	///
 	fn deallocate(self: *Context, allocation: []u8) void {
 		const target_allocation_info = @intToPtr(*AllocationInfo, @ptrToInt(allocation.ptr) - @sizeOf(AllocationInfo));
@@ -92,7 +91,19 @@ const Context = struct {
 	}
 
 	///
+	/// Attempts to reallocate the buffer referenced by `allocation` to be `size` length from `self`.
 	///
+	/// A reference to the reallocated buffer is returned via a slice if the reallocation was successful, otherwise
+	/// `null` is returned.
+	///
+	/// *Note* the returned buffer must be deallocated with [deallocate] before program exit or it will cause a memory
+	/// leak.
+	///
+	/// *Note* the pointer and length of `allocation` must match valid values known to `allocator` otherwise safety-
+	/// checked behavior will occur.
+	///
+	/// *Note* the allocation referenced by `allocation` should be considered invalid once the function returns,
+	/// discarding it in favor of the return value.
 	///
 	fn reallocate(self: *Context, allocation: []u8, size: usize) ?[]u8 {
 		const allocation_info_size = @sizeOf(AllocationInfo);
@@ -133,7 +144,7 @@ const Context = struct {
 };
 
 ///
-///
+/// Heap context.
 ///
 var context = Context{};
 
@@ -149,24 +160,30 @@ pub const allocator = coral.io.Allocator.bind(Context, &context, struct {
 				return null;
 			}
 
-			return self.allocate(0);
+			return self.allocate(0, options.return_address);
 		}
 
 		if (options.allocation) |allocation| {
 			return self.reallocate(allocation, options.size);
 		}
 
-		return self.allocate(options.size);
+		return self.allocate(options.size, options.return_address);
 	}
 }.reallocate);
 
 ///
+/// Checks for any allocations belonging to the process heap allocated through the [allocator] interface that are still
+/// alive and reports the stack traces of any detected allocations to stderr along with the allocation address and
+/// length.
 ///
-///
-pub fn trace_allocations() void {
+pub fn trace_leaks() void {
 	var current_allocation_info = context.allocation_info_head;
 
-	while (current_allocation_info) |allocation_info| : (current_allocation_info = allocation_info) {
-		std.debug.dumpStackTrace(allocation_info.stack_trace);
+	while (current_allocation_info) |allocation_info| : (current_allocation_info = allocation_info.next_info) {
+		std.debug.print("{d} byte leak at 0x{x} detected: {}", .{
+			allocation_info.size,
+			@ptrToInt(allocation_info) + @sizeOf(AllocationInfo),
+			allocation_info.trace
+		});
 	}
 }

source/ona/kym/Environment.zig

@@ -69,21 +69,6 @@ const Object = struct {
 
 		self.ref_count += 1;
 	}
-
-	pub fn release(self: *Object, env: *Self) bool {
-		coral.debug.assert(self.ref_count != 0);
-
-		self.ref_count -= 1;
-
-		if (self.ref_count == 0) {
-			coral.io.deallocate(env.allocator, self.state.userdata);
-			self.state.fields.deinit(env.allocator);
-
-			return false;
-		}
-
-		return true;
-	}
 };
 
 pub const ObjectInfo = struct {
@@ -185,8 +170,10 @@ pub fn check(self: *Self, condition: bool, failure_message: []const u8) !void {
 }
 
 pub fn deinit(self: *Self) void {
+	self.discard(.{.object = self.global_object});
 	self.values.deinit(self.allocator);
 	self.calls.deinit(self.allocator);
+	self.heap.deinit(self.allocator);
 }
 
 pub fn discard(self: *Self, val: types.Val) void {
@@ -194,7 +181,19 @@ pub fn discard(self: *Self, val: types.Val) void {
 		.object => |object| {
 			var data = self.heap.fetch(object);
 
-			if (data.release(self)) {
+			coral.debug.assert(data.ref_count != 0);
+
+			data.ref_count -= 1;
+
+			if (data.ref_count == 0) {
+				data.state.info.deinitializer(.{
+					.env = self,
+					.obj = val.as_ref(),
+				});
+
+				coral.io.deallocate(self.allocator, data.state.userdata);
+				data.state.fields.deinit(self.allocator);
+
 				self.heap.remove(object);
 			} else {
 				self.heap.assign(object, data);
@@ -214,17 +213,21 @@ pub fn execute_data(self: *Self, source: DataSource) types.RuntimeError!types.Va
 		}
 	};
 
-	var chunk = try Chunk.init(self, source.name);
+	var compiled_chunk = init_compiled_chunk: {
+		var chunk = try Chunk.init(self, source.name);
 
-	errdefer chunk.deinit();
+		errdefer chunk.deinit();
 
-	chunk.compile(source.data) catch |compile_error| {
-		self.reporter.invoke(chunk.error_details());
+		chunk.compile(source.data) catch |compile_error| {
+			self.reporter.invoke(chunk.error_details());
 
-		return compile_error;
+			return compile_error;
+		};
+
+		break: init_compiled_chunk chunk;
 	};
 
-	const script = try self.new_object(coral.io.bytes_of(&chunk), .{
+	const script = try self.new_object(coral.io.bytes_of(&compiled_chunk), .{
 		.identity = typeid,
 		.deinitializer = Behaviors.deinitialize,
 	});
@@ -346,12 +349,11 @@ pub fn native_cast(self: *Self, castable: types.Ref, id: *const anyopaque, compt
 	try self.check(castable == .object, "invalid type conversion: object");
 
 	const object = self.heap.fetch(castable.object);
-	const alignment = @alignOf(Type);
-	const is_expected_type = (object.state.info.identity == id) and (object.state.userdata.len == alignment);
+	const is_expected_type = (object.state.info.identity == id) and (object.state.userdata.len == @sizeOf(Type));
 
 	try self.check(is_expected_type, "invalid object cast: native type");
 
-	return @ptrCast(*Type, @alignCast(alignment, object.state.userdata));
+	return @ptrCast(*Type, @alignCast(@alignOf(Type), object.state.userdata));
 }
 
 pub fn new_array(self: *Self) coral.io.AllocationError!types.Val {

source/ona/ona.zig

@@ -56,6 +56,8 @@ const AppManifest = struct {
 };
 
 pub fn run_app(base_file_system: file.System) void {
+	defer heap.trace_leaks();
+
 	const Logger = struct {
 		const Self = @This();
 
@@ -135,6 +137,4 @@ pub fn run_app(base_file_system: file.System) void {
 			ext.SDL_RenderPresent(renderer);
 		}
 	}
-
-	heap.trace_allocations();
 }