Skip to content
Closed

Fix #22

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/msgpack.zig
Original file line number Diff line number Diff line change
Expand Up @@ -865,16 +865,24 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload {

.arr => |arr| {
const new_arr = try allocator.alloc(Payload, arr.len);
errdefer allocator.free(new_arr);
var cloned_count: usize = 0;
errdefer {
// cleanup partial clones on error
for (new_arr[0..cloned_count]) |item| {
item.free(allocator);
}
allocator.free(new_arr);
}
for (arr, 0..) |item, i| {
new_arr[i] = try clonePayload(item, allocator);
cloned_count += 1;
}
return Payload{ .arr = new_arr };
},

.map => |m| {
var new_map = Map.init(allocator);
errdefer new_map.deinit();
errdefer (Payload{ .map = new_map }).free(allocator);

// Clone all entries
var it = m.map.iterator();
Expand Down
50 changes: 49 additions & 1 deletion src/test.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1187,7 +1187,7 @@ test "payload utility methods" {
const cloned_str_payload = try str_payload.deepClone(allocator);
defer cloned_str_payload.free(allocator);
try expect(u8eql("test", cloned_str_payload.str.value()));
try expect(@ptrToInt(str_payload.str.value().ptr) != @ptrToInt(cloned_str_payload.str.value().ptr));
try expect(@intFromPtr(str_payload.str.value().ptr) != @intFromPtr(cloned_str_payload.str.value().ptr));

const arr_payload = try Payload.arrPayload(3, allocator);
defer arr_payload.free(allocator);
Expand Down Expand Up @@ -3613,6 +3613,54 @@ test "iterative parser: deep nested maps" {
try expect(decoded == .map);
}

test "clonePayload arr partial fail path frees partially cloned elements" {
var src = try Payload.arrPayload(2, std.heap.page_allocator);
defer src.free(std.heap.page_allocator);

try src.setArrElement(0, try Payload.strToPayload("a", std.heap.page_allocator));
try src.setArrElement(1, try Payload.strToPayload("this-is-a-very-large-string-that-will-always-fail-because-it-is-much-longer-than-the-fixed-buffer-can-handle-and-should-cause-out-of-memory-error-during-cloning-process", std.heap.page_allocator));

var buffer: [@sizeOf(Payload) * 2 + 8]u8 = undefined;
var pool = std.heap.FixedBufferAllocator.init(&buffer);
const clone_alloc = pool.allocator();

const result = src.deepClone(clone_alloc);
if (result) |cloned| {
cloned.free(clone_alloc);
try std.testing.expect(false);
} else |err| {
try std.testing.expect(err == error.OutOfMemory);
}

// Verify post-failure allocator state is valid: next alloc must succeed.
const next_alloc = try clone_alloc.alloc(u8, 16);
clone_alloc.free(next_alloc);
}

test "clonePayload map partial fail path frees partially cloned entries" {
var src = Payload.mapPayload(std.heap.page_allocator);
defer src.free(std.heap.page_allocator);

try src.mapPut("k1", try Payload.strToPayload("v1", std.heap.page_allocator));
try src.mapPut("k2", try Payload.strToPayload("very-long-string-to-force-out-of-memory-on-clone-because-the-fixed-buffer-is-too-small-to-allocate-this-large-string-during-the-cloning-operation", std.heap.page_allocator));

var buffer: [@sizeOf(Payload) * 2 + 16]u8 = undefined;
var pool = std.heap.FixedBufferAllocator.init(&buffer);
const clone_alloc = pool.allocator();

const result = src.deepClone(clone_alloc);
if (result) |cloned| {
cloned.free(clone_alloc);
try std.testing.expect(false);
} else |err| {
try std.testing.expect(err == error.OutOfMemory);
}

// Verify post-failure allocator state is valid: next alloc must succeed.
const next_alloc = try clone_alloc.alloc(u8, 16);
clone_alloc.free(next_alloc);
}
Comment on lines +3640 to +3662
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This test may not be sufficient to detect the double-free issue present in the clonePayload implementation for maps. The std.heap.FixedBufferAllocator does not detect double-frees, so it's possible for the double-free to occur without causing the subsequent allocation to fail, leading to a false positive test result.

To reliably catch this kind of memory safety issue, you would typically need to run tests with an allocator that has double-free detection or use tools like the address sanitizer (-fsanitize=address).


test "iterative free: deeply nested payload" {
// Create a deeply nested structure in memory
var root = try Payload.arrPayload(1, allocator);
Expand Down
Loading