fix: memory leak when decoding Lists of Bytes #68
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Messages that include a `repeated` field of type `bytes` are not properly freed due to a missing condition `deinit_field()` which only cares for list of other message types or strings, but not bytes. This commit introduces a new unit test that fails if decoding a message with a list of bytes leaks and fix, that makes the unit test pass.
|
Thanks! I'll look into the details ASAP. It seems I introduced this bug myself recently with bytes. Seems OK at first glance, but I will look at it properly soon. |
Arwalk
requested changes
Sep 14, 2024
src/protobuf.zig
Outdated
| @@ -586,7 +586,7 @@ fn deinit_field(result: anytype, comptime field_name: []const u8, comptime ftype | |||
| } | |||
| }, | |||
| .List => |list_type| { | |||
| if (list_type == .SubMessage or list_type == .String) { | |||
| if (list_type == .SubMessage or list_type == .String or list_type == .Bytes) { | |||
There was a problem hiding this comment.
Let's use switch here so we have the exhaustive check at compile time and avoid this kind of error in the future.
diff --git a/src/protobuf.zig b/src/protobuf.zig
index dbe0c48..ac7e811 100644
--- a/src/protobuf.zig
+++ b/src/protobuf.zig
@@ -586,10 +586,13 @@ fn deinit_field(result: anytype, comptime field_name: []const u8, comptime ftype
}
},
.List => |list_type| {
- if (list_type == .SubMessage or list_type == .String or list_type == .Bytes) {
- for (@field(result, field_name).items) |item| {
- item.deinit();
- }
+ switch (list_type) {
+ .SubMessage, .String, .Bytes => {
+ for (@field(result, field_name).items) |item| {
+ item.deinit();
+ }
+ },
+ .Varint, .FixedInt => {},
}
@field(result, field_name).deinit();
},There was a problem hiding this comment.
Ok, I'll change it. Thanks for the improvement. I'm currently taking my first baby steps with zig, so I'm grateful for each learning along the way :)
In `deinit_field()` we previously used an if condition that checked for different types within a list, but unfortunately this check was not exhaustive. By replacing this check with an switch statement, we now make sure that we have to handle all possible cases, otherwise we'll get a compiler error.
|
I changed the check to use |
Arwalk
approved these changes
Sep 15, 2024
|
Thanks a lot for your contribution ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Messages that include a
repeatedfield of typebytesare not properly freed due to a missing condition indeinit_field()which only cares for list of other message types or strings, but not bytes.This commit introduces a new unit test that fails if decoding a message with a list of bytes leaks and fix, that makes the unit test pass.