Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves memory safety in clonePayload by ensuring that partially cloned arrays and maps are properly cleaned up if an error occurs during the cloning process. It also updates pointer-to-integer conversions in tests to use @intFromPtr. Feedback was provided regarding the new tests, noting that FixedBufferAllocator may not reliably detect double-free issues, and suggesting the use of an allocator with double-free detection or address sanitizers for more robust verification.
| 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); | ||
| } |
There was a problem hiding this comment.
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).
1 participant
No description provided.