Skip to content

Fix memory leak in clonePayload#21

Merged
jinzhongjia merged 7 commits intozigcc:mainfrom
mstdokumaci:fix
Apr 2, 2026
Merged

Fix memory leak in clonePayload#21
jinzhongjia merged 7 commits intozigcc:mainfrom
mstdokumaci:fix

Conversation

@mstdokumaci
Copy link
Copy Markdown
Contributor

@mstdokumaci mstdokumaci commented Mar 31, 2026

  • Fixed potential memory leaks on error paths in .arr and .map branches of clonePayload. Fixes Potential memory leak #20
  • Added two new tests for clonePayload error paths
  • Replaced ptrToInt with intFromPtr in tests

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves memory safety in src/msgpack.zig by implementing manual cleanup for partially cloned arrays and maps when an error occurs during clonePayload. It also updates src/test.zig to use @intFromPtr and adds regression tests for these failure paths. The review feedback identifies opportunities to improve the cleanup logic by iterating in reverse for array elements to better support stack-like allocators and by reusing the Payload.free method to simplify map cleanup.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves memory safety in clonePayload by ensuring that partially cloned arrays and maps are correctly freed if an error occurs during the cloning process. It also updates deprecated @ptrToInt calls to @intFromPtr in the test suite and adds new test cases to verify the error-path cleanup logic. The review feedback suggests simplifying the array cleanup loop to reduce complexity and streamlining the map cleanup by calling deinit directly instead of wrapping the map in a temporary Payload object.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces memory leak protection in the clonePayload function for arrays by implementing an errdefer block that cleans up partially cloned elements. It also updates test cases to use @intFromPtr and adds new tests for failure paths. The review feedback highlights a similar unaddressed memory leak in the map cloning logic, suggests using a more idiomatic for loop for array cleanup, and points out that the new map failure test may be unreliable due to the fixed buffer size.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves memory safety in clonePayload by adding cleanup logic for partially cloned arrays and maps during error conditions. It also updates pointer-to-integer conversions in tests for compatibility with newer Zig versions. However, the refactoring of the map cloning logic is noted to be unnecessarily complex and still leaks entries from previous iterations if a later one fails. Additionally, the new regression tests are unreliable because they use FixedBufferAllocator, which does not reclaim memory on free, thus failing to properly verify that the cleanup logic works as intended.

@mstdokumaci
Copy link
Copy Markdown
Contributor Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves memory safety in src/msgpack.zig by ensuring that partially cloned arrays and maps are properly freed if an error occurs during the cloning process. It also updates pointer-to-int conversions in tests to use @intFromPtr and adds new test cases to verify these error-handling paths. A review comment suggests that the buffer size in the map partial failure test might be too small to allow the initial HashMap bucket allocation, which is necessary to correctly simulate a partial clone.

@jinzhongjia jinzhongjia merged commit 2795481 into zigcc:main Apr 2, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Potential memory leak

2 participants