From 779ffa27fdea746a24012b73dd9581c1aacf5fdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mustafa=20Dokumac=C4=B1?= Date: Tue, 31 Mar 2026 06:47:38 +0000 Subject: [PATCH 1/6] fix(clonePayload): improve error handling for partial cloning of arrays and maps --- src/msgpack.zig | 30 +++++++++++++++++++++++++++--- src/test.zig | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/src/msgpack.zig b/src/msgpack.zig index b3a7106..14f4d1d 100644 --- a/src/msgpack.zig +++ b/src/msgpack.zig @@ -865,16 +865,38 @@ 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; + var success = false; + defer if (!success) { + // cleanup partial clones on error + for (0..cloned_count) |j| { + new_arr[j].free(allocator); + } + allocator.free(new_arr); + }; + for (arr, 0..) |item, i| { - new_arr[i] = try clonePayload(item, allocator); + const cloned_item = try clonePayload(item, allocator); + new_arr[i] = cloned_item; + cloned_count += 1; } + + success = true; return Payload{ .arr = new_arr }; }, .map => |m| { var new_map = Map.init(allocator); - errdefer new_map.deinit(); + var success = false; + defer if (!success) { + // Free any partially-inserted keys/values on error before deinit + var it2 = new_map.map.iterator(); + while (it2.next()) |entry| { + entry.key_ptr.*.free(allocator); + entry.value_ptr.*.free(allocator); + } + new_map.deinit(); + }; // Clone all entries var it = m.map.iterator(); @@ -887,6 +909,8 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { // Use putInternal to insert without additional cloning try new_map.putInternal(cloned_key, cloned_value); } + + success = true; return Payload{ .map = new_map }; }, }; diff --git a/src/test.zig b/src/test.zig index 69f9f51..42b0e39 100644 --- a/src/test.zig +++ b/src/test.zig @@ -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); @@ -3613,6 +3613,52 @@ 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-large-string-that-will-always-fail", 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); + } + + 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", 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); + } + + const next_alloc = try clone_alloc.alloc(u8, 16); + clone_alloc.free(next_alloc); +} + test "iterative free: deeply nested payload" { // Create a deeply nested structure in memory var root = try Payload.arrPayload(1, allocator); From f56062ee65647acc325ec532f853489cbb9e294c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mustafa=20Dokumac=C4=B1?= Date: Tue, 31 Mar 2026 06:59:39 +0000 Subject: [PATCH 2/6] chore: apply code review changes --- src/msgpack.zig | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/msgpack.zig b/src/msgpack.zig index 14f4d1d..a8d2b2d 100644 --- a/src/msgpack.zig +++ b/src/msgpack.zig @@ -869,7 +869,9 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { var success = false; defer if (!success) { // cleanup partial clones on error - for (0..cloned_count) |j| { + var j: usize = cloned_count; + while (j > 0) { + j -= 1; new_arr[j].free(allocator); } allocator.free(new_arr); @@ -889,13 +891,7 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { var new_map = Map.init(allocator); var success = false; defer if (!success) { - // Free any partially-inserted keys/values on error before deinit - var it2 = new_map.map.iterator(); - while (it2.next()) |entry| { - entry.key_ptr.*.free(allocator); - entry.value_ptr.*.free(allocator); - } - new_map.deinit(); + (Payload{ .map = new_map }).free(allocator); }; // Clone all entries From 2f87d9252c162f53e2475d9af383c73a9e301ece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mustafa=20Dokumac=C4=B1?= Date: Wed, 1 Apr 2026 20:21:12 +0000 Subject: [PATCH 3/6] fix(clonePayload): simplify error handling in clonePayload function --- src/msgpack.zig | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/msgpack.zig b/src/msgpack.zig index a8d2b2d..3fae48c 100644 --- a/src/msgpack.zig +++ b/src/msgpack.zig @@ -866,8 +866,7 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { .arr => |arr| { const new_arr = try allocator.alloc(Payload, arr.len); var cloned_count: usize = 0; - var success = false; - defer if (!success) { + errdefer { // cleanup partial clones on error var j: usize = cloned_count; while (j > 0) { @@ -875,7 +874,7 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { new_arr[j].free(allocator); } allocator.free(new_arr); - }; + } for (arr, 0..) |item, i| { const cloned_item = try clonePayload(item, allocator); @@ -883,16 +882,12 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { cloned_count += 1; } - success = true; return Payload{ .arr = new_arr }; }, .map => |m| { var new_map = Map.init(allocator); - var success = false; - defer if (!success) { - (Payload{ .map = new_map }).free(allocator); - }; + errdefer new_map.deinit(); // Clone all entries var it = m.map.iterator(); @@ -906,7 +901,6 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { try new_map.putInternal(cloned_key, cloned_value); } - success = true; return Payload{ .map = new_map }; }, }; From 731baa266581d57814a9430c52ed16760e5c3d3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mustafa=20Dokumac=C4=B1?= Date: Wed, 1 Apr 2026 20:25:52 +0000 Subject: [PATCH 4/6] lint --- src/msgpack.zig | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/msgpack.zig b/src/msgpack.zig index 3fae48c..cf727cd 100644 --- a/src/msgpack.zig +++ b/src/msgpack.zig @@ -875,13 +875,10 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { } allocator.free(new_arr); } - for (arr, 0..) |item, i| { - const cloned_item = try clonePayload(item, allocator); - new_arr[i] = cloned_item; + new_arr[i] = try clonePayload(item, allocator); cloned_count += 1; } - return Payload{ .arr = new_arr }; }, @@ -900,7 +897,6 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { // Use putInternal to insert without additional cloning try new_map.putInternal(cloned_key, cloned_value); } - return Payload{ .map = new_map }; }, }; From 79943da125e4cdcb30b05dbedd54d6f023abe0f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mustafa=20Dokumac=C4=B1?= Date: Wed, 1 Apr 2026 20:35:17 +0000 Subject: [PATCH 5/6] chore: apply suggested fixes --- src/msgpack.zig | 21 ++++++++++++--------- src/test.zig | 4 ++-- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/msgpack.zig b/src/msgpack.zig index cf727cd..163ba67 100644 --- a/src/msgpack.zig +++ b/src/msgpack.zig @@ -868,10 +868,8 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { var cloned_count: usize = 0; errdefer { // cleanup partial clones on error - var j: usize = cloned_count; - while (j > 0) { - j -= 1; - new_arr[j].free(allocator); + for (new_arr[0..cloned_count]) |item| { + item.free(allocator); } allocator.free(new_arr); } @@ -889,13 +887,18 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { // Clone all entries var it = m.map.iterator(); while (it.next()) |entry| { - const cloned_key = try clonePayload(entry.key_ptr.*, allocator); - errdefer cloned_key.free(allocator); - const cloned_value = try clonePayload(entry.value_ptr.*, allocator); - errdefer cloned_value.free(allocator); + var cloned_key: ?Payload = null; + var cloned_value: ?Payload = null; + errdefer { + if (cloned_value) |v| v.free(allocator); + if (cloned_key) |k| k.free(allocator); + } + + cloned_key = try clonePayload(entry.key_ptr.*, allocator); + cloned_value = try clonePayload(entry.value_ptr.*, allocator); // Use putInternal to insert without additional cloning - try new_map.putInternal(cloned_key, cloned_value); + try new_map.putInternal(cloned_key.?, cloned_value.?); } return Payload{ .map = new_map }; }, diff --git a/src/test.zig b/src/test.zig index 42b0e39..0f20172 100644 --- a/src/test.zig +++ b/src/test.zig @@ -3618,7 +3618,7 @@ test "clonePayload arr partial fail path frees partially cloned elements" { 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-large-string-that-will-always-fail", 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); @@ -3641,7 +3641,7 @@ test "clonePayload map partial fail path frees partially cloned entries" { 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", 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); From e7843ed684886dd0767cee4083898a2af706306b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mustafa=20Dokumac=C4=B1?= Date: Wed, 1 Apr 2026 20:59:05 +0000 Subject: [PATCH 6/6] chore: apply suggested fixes --- src/msgpack.zig | 17 ++++++----------- src/test.zig | 2 ++ 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/msgpack.zig b/src/msgpack.zig index 163ba67..7327751 100644 --- a/src/msgpack.zig +++ b/src/msgpack.zig @@ -882,23 +882,18 @@ fn clonePayload(payload: Payload, allocator: Allocator) !Payload { .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(); while (it.next()) |entry| { - var cloned_key: ?Payload = null; - var cloned_value: ?Payload = null; - errdefer { - if (cloned_value) |v| v.free(allocator); - if (cloned_key) |k| k.free(allocator); - } - - cloned_key = try clonePayload(entry.key_ptr.*, allocator); - cloned_value = try clonePayload(entry.value_ptr.*, allocator); + const cloned_key = try clonePayload(entry.key_ptr.*, allocator); + errdefer cloned_key.free(allocator); + const cloned_value = try clonePayload(entry.value_ptr.*, allocator); + errdefer cloned_value.free(allocator); // Use putInternal to insert without additional cloning - try new_map.putInternal(cloned_key.?, cloned_value.?); + try new_map.putInternal(cloned_key, cloned_value); } return Payload{ .map = new_map }; }, diff --git a/src/test.zig b/src/test.zig index 0f20172..19e5637 100644 --- a/src/test.zig +++ b/src/test.zig @@ -3632,6 +3632,7 @@ test "clonePayload arr partial fail path frees partially cloned elements" { 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); } @@ -3655,6 +3656,7 @@ test "clonePayload map partial fail path frees partially cloned entries" { 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); }