fix(clonePayload): improve error handling for partial cloning of arrays and maps

Author: mstdokumaci

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 };
         },
     };

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);