-
Notifications
You must be signed in to change notification settings - Fork 66
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
V2: Check for matching field numbers in StartGroup / EndGroup tags #810
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be ported to main / v1.
@@ -108,12 +108,12 @@ function readMessage( | |||
const unknownFields = message.getUnknown() ?? []; | |||
while (reader.pos < end) { | |||
[fieldNo, wireType] = reader.tag(); | |||
if (wireType == WireType.EndGroup) { | |||
if (delimited && wireType == WireType.EndGroup) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only break the loop for EndGroup if we're currently parsing a group (a.k.a. delimited message encoding).
Previously, this would behave incorrectly for a rogue EndGroup tag.
break; | ||
} | ||
const field = message.findNumber(fieldNo); | ||
if (!field) { | ||
const data = reader.skip(wireType); | ||
const data = reader.skip(wireType, fieldNo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to pass the field number, so that the EndGroup tag's field number can be checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
let t: WireType; | ||
while ((t = this.tag()[1]) !== WireType.EndGroup) { | ||
this.skip(t); | ||
for (;;) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting syntax. Is this idiomatic for JS/TS? In languages with while
keywords, I am used to instead seeing while (true)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be idiomatic to avoid loops, which is not feasible here. I would prefer while (true)
as well, but the linter objects the constant condition.
It's possible that the linter can be configured to accept it, but touching the linter config is always very involved. Since we have to change the linter config to replace make
with turborepo
, it's better to punt on it for now. Our usage of make
in this repo is much more unidiomatic than this loop construct.
This is a port of #810 from the v2 branch into the main (v1) branch. --------- Co-authored-by: Timo Stamm <ts@timostamm.de>
When an unknown field is encountered while parsing binary, the tag is skipped. This adds a check for skipping groups, verifying that the field number of StartGroup and EndGroup matches.