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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 25, 2024
Merged

V2: Check for matching field numbers in StartGroup / EndGroup tags #810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
590607b
V2: Check for matching field numbers in StartGroup / EndGroup tags wh…
timostamm Apr 24, 2024
2dde13b
Merge branch 'v2' into tstamm/check-group-field-no-when-skipping
timostamm Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/protobuf-bench/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ server would usually do.

| code generator | bundle size | minified | compressed |
|---------------------|------------------------:|-----------------------:|-------------------:|
| protobuf-es | 126,666 b | 65,334 b | 15,946 b |
| protobuf-es | 126,944 b | 65,418 b | 15,948 b |
| protobuf-javascript | 394,384 b | 288,654 b | 45,122 b |
79 changes: 77 additions & 2 deletions packages/protobuf-test/src/wire/binary-encoding.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@
// limitations under the License.

import { describe, expect, it } from "@jest/globals";
import { BinaryWriter, WireType } from "@bufbuild/protobuf/wire";
import { UserDesc } from "../gen/ts/extra/example_pb.js";
import { fromBinary } from "@bufbuild/protobuf";
import { BinaryWriter, BinaryReader, WireType } from "@bufbuild/protobuf/wire";
import { UserDesc } from "../gen/ts/extra/example_pb.js";

describe("BinaryWriter example", () => {
it("should work as expected", () => {
Expand All @@ -32,3 +32,78 @@ describe("BinaryWriter example", () => {
expect(user.active).toBe(true);
});
});

describe("BinaryReader", () => {
describe("skip", () => {
it("should skip group", () => {
const reader = new BinaryReader(
new BinaryWriter()
.tag(1, WireType.StartGroup)
.tag(33, WireType.Varint)
.bool(true)
.tag(1, WireType.EndGroup)
.finish(),
);
const [fieldNo, wireType] = reader.tag();
expect(fieldNo).toBe(1);
expect(wireType).toBe(WireType.StartGroup);
reader.skip(WireType.StartGroup, 1);
expect(reader.pos).toBe(reader.len);
});
it("should skip nested group", () => {
const reader = new BinaryReader(
new BinaryWriter()
.tag(1, WireType.StartGroup)
.tag(1, WireType.StartGroup)
.tag(1, WireType.EndGroup)
.tag(1, WireType.EndGroup)
.finish(),
);
const [fieldNo, wireType] = reader.tag();
expect(fieldNo).toBe(1);
expect(wireType).toBe(WireType.StartGroup);
reader.skip(WireType.StartGroup, 1);
expect(reader.pos).toBe(reader.len);
});
it("should error on unexpected end group field number", () => {
const reader = new BinaryReader(
new BinaryWriter()
.tag(1, WireType.StartGroup)
.tag(2, WireType.EndGroup)
.finish(),
);
const [fieldNo, wireType] = reader.tag();
expect(fieldNo).toBe(1);
expect(wireType).toBe(WireType.StartGroup);
expect(() => {
reader.skip(WireType.StartGroup, 1);
}).toThrow(/^invalid end group tag$/);
});
it("should return skipped group data", () => {
const reader = new BinaryReader(
new BinaryWriter()
.tag(1, WireType.StartGroup)
.tag(33, WireType.Varint)
.bool(true)
.tag(1, WireType.EndGroup)
.finish(),
);
reader.tag();
const skipped = reader.skip(WireType.StartGroup, 1);
const sr = new BinaryReader(skipped);
{
const [fieldNo, wireType] = sr.tag();
expect(fieldNo).toBe(33);
expect(wireType).toBe(WireType.Varint);
const bool = sr.bool();
expect(bool).toBe(true);
}
{
const [fieldNo, wireType] = sr.tag();
expect(fieldNo).toBe(1);
expect(wireType).toBe(WireType.EndGroup);
expect(sr.pos).toBe(sr.len);
}
});
});
});
2 changes: 1 addition & 1 deletion packages/protobuf/src/extensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export function setExtension<Desc extends DescExtension>(
const reader = new BinaryReader(writer.finish());
while (reader.pos < reader.len) {
const [no, wireType] = reader.tag();
const data = reader.skip(wireType);
const data = reader.skip(wireType, no);
ufs.push({ no, wireType, data });
}
message.$unknown = ufs;
Expand Down
4 changes: 2 additions & 2 deletions packages/protobuf/src/from-binary.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member Author

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);
Copy link
Member Author

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.

if (options.readUnknownFields) {
unknownFields.push({ no: fieldNo, wireType, data });
}
Expand Down
21 changes: 14 additions & 7 deletions packages/protobuf/src/wire/binary-encoding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,10 +374,12 @@ export class BinaryReader {
}

/**
* Skip one element on the wire and return the skipped data.
* Supports WireType.StartGroup since v2.0.0-alpha.23.
* Skip one element and return the skipped data.
*
* When skipping StartGroup, provide the tags field number to check for
* matching field number in the EndGroup tag.
*/
skip(wireType: WireType): Uint8Array {
skip(wireType: WireType, fieldNo?: number): Uint8Array {
let start = this.pos;
switch (wireType) {
case WireType.Varint:
Expand All @@ -398,10 +400,15 @@ export class BinaryReader {
this.pos += len;
break;
case WireType.StartGroup:
// TODO check for matching field numbers in StartGroup / EndGroup tags
let t: WireType;
while ((t = this.tag()[1]) !== WireType.EndGroup) {
this.skip(t);
for (;;) {
Copy link
Member

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).

Copy link
Member Author

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.

const [fn, wt] = this.tag();
if (wt === WireType.EndGroup) {
if (fieldNo !== undefined && fn !== fieldNo) {
throw new Error("invalid end group tag");
}
break;
}
this.skip(wt, fn);
}
break;
default:
Expand Down
Loading