Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Convert OOB enums from panics to runtime errors #76

Merged
merged 3 commits into from
Oct 29, 2024

Conversation

hans-tvs
Copy link
Contributor

No description provided.

@Arwalk Arwalk requested review from Arwalk and menduz October 28, 2024 19:30
Copy link
Owner

@Arwalk Arwalk left a comment

Choose a reason for hiding this comment

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

Excellent! Some minor changes requested. Thank you for catching this problem and fixing it.

src/protobuf.zig Outdated
@@ -808,7 +808,7 @@ pub const WireDecoderIterator = struct {
};

/// Get a real varint of type T from a raw u64 data.
fn decode_varint_value(comptime T: type, comptime varint_type: VarintType, raw: u64) T {
fn decode_varint_value(comptime T: type, comptime varint_type: VarintType, raw: u64) !T {
Copy link
Owner

Choose a reason for hiding this comment

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

polish: Can we make the error type explicit instead of inferred? I'd like to make a pass on all the method to make them explicits in the near future.

src/protobuf.zig Outdated
@@ -825,7 +825,10 @@ fn decode_varint_value(comptime T: type, comptime varint_type: VarintType, raw:
else => @compileError("Invalid type " ++ @typeName(T) ++ " passed"),
},
.Bool => raw != 0,
.Enum => @as(T, @enumFromInt(@as(i32, @intCast(raw)))),
.Enum => if ((raw >> 32) != 0)
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: wouldn't it be clearer to use std.math.maxInt(i32)?

.Enum => if(raw > std.math.maxInt(i32))

.Enum => if ((raw >> 32) != 0)
error.InvalidInput
else
@as(T, @enumFromInt(@as(i32, @bitCast(@as(u32, @intCast(raw)))))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is there a new bitCast?

Copy link
Collaborator

@menduz menduz left a comment

Choose a reason for hiding this comment

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

Could you add some comments about the motivations for the changeset? the motivation is not evident in the tests

@hans-tvs
Copy link
Contributor Author

hans-tvs commented Oct 29, 2024

Thanks for the suggestions. I took another pass at this.

explicit error type

I used that particular error literal because it fit the problem well and appeared many times throughout the code. Does re-using the DecodingError you already have defined work here?

std.math.maxInt(i32)
Why is there a new bitcast?

These are related. It's valid for protobuf enums to have negative values, and the previous @intCast didn't suffice for those. The solution here uses an @intCast, as before, to drop from 64 to 32 bits (cost varies from architecture to architecture, but it's cheap), and a @bitCast to assert that those bits are actually signed (free at runtime except in some esoteric microcontrollers and whatnot).

Regarding your suggestion, comparing to std.math.maxInt(u32) (not the signed version) seems reasonable.

Can you add some comments about the motivations for the changeset?

I added comments in code for one of the tests; the other test was just a new test for existing behavior (mainly for my benefit, ensuring I hadn't made any egregious errors, but it usually doesn't hurt to add those tests upstream). I'm not totally sure what you're looking for, so would you mind taking a peek and explaining why that comment does or does not work?

Copy link
Collaborator

@menduz menduz left a comment

Choose a reason for hiding this comment

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

Many thanks for the revisions

@Arwalk Arwalk changed the title Convert OOB enums from panics to runtime errors fix: Convert OOB enums from panics to runtime errors Oct 29, 2024
@Arwalk Arwalk merged commit eed464e into Arwalk:master Oct 29, 2024
5 checks passed
@Arwalk
Copy link
Owner

Arwalk commented Oct 29, 2024

Thanks again for your contribution.

@hans-tvs
Copy link
Contributor Author

Thank you so much for the fast review and for building this project in the first place :)

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.

3 participants