From ad203ddc79d5c5d836593f39ea540c8416534af1 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Mon, 29 Apr 2024 10:31:01 -0400 Subject: [PATCH] Check for matching field numbers in StartGroup / EndGroup tags (#816) This is a port of https://github.com/bufbuild/protobuf-es/pull/810 from the v2 branch into the main (v1) branch. --------- Co-authored-by: Timo Stamm --- packages/protobuf-bench/README.md | 2 +- .../src/binary-reader-writer.test.ts | 77 ++++++++++++++++++- packages/protobuf/src/binary-encoding.ts | 23 ++++-- packages/protobuf/src/extension-accessor.ts | 2 +- .../protobuf/src/private/binary-format.ts | 7 +- 5 files changed, 98 insertions(+), 13 deletions(-) diff --git a/packages/protobuf-bench/README.md b/packages/protobuf-bench/README.md index 628feebb7..7102fc411 100644 --- a/packages/protobuf-bench/README.md +++ b/packages/protobuf-bench/README.md @@ -10,5 +10,5 @@ server would usually do. | code generator | bundle size | minified | compressed | |---------------------|------------------------:|-----------------------:|-------------------:| -| protobuf-es | 97,764 b | 41,777 b | 10,850 b | +| protobuf-es | 98,070 b | 41,868 b | 10,882 b | | protobuf-javascript | 394,384 b | 288,654 b | 45,122 b | diff --git a/packages/protobuf-test/src/binary-reader-writer.test.ts b/packages/protobuf-test/src/binary-reader-writer.test.ts index 91a649e96..32b798c2f 100644 --- a/packages/protobuf-test/src/binary-reader-writer.test.ts +++ b/packages/protobuf-test/src/binary-reader-writer.test.ts @@ -13,7 +13,7 @@ // limitations under the License. import { describe, expect, it } from "@jest/globals"; -import { BinaryWriter, WireType } from "@bufbuild/protobuf"; +import { BinaryReader, BinaryWriter, WireType } from "@bufbuild/protobuf"; import { User } from "./gen/ts/extra/example_pb.js"; describe("BinaryWriter example", () => { @@ -31,3 +31,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); + } + }); + }); +}); diff --git a/packages/protobuf/src/binary-encoding.ts b/packages/protobuf/src/binary-encoding.ts index 683c9af28..5b9dcdbc4 100644 --- a/packages/protobuf/src/binary-encoding.ts +++ b/packages/protobuf/src/binary-encoding.ts @@ -92,7 +92,7 @@ export interface IBinaryReader { /** * Skip one element on the wire and return the skipped data. */ - skip(wireType: WireType): Uint8Array; + skip(wireType: WireType, fieldNo?: number): Uint8Array; /** * Read a `uint32` field, an unsigned 32 bit varint. @@ -582,10 +582,12 @@ export class BinaryReader implements IBinaryReader { } /** - * 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: @@ -607,10 +609,15 @@ export class BinaryReader implements IBinaryReader { 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 (;;) { + 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: diff --git a/packages/protobuf/src/extension-accessor.ts b/packages/protobuf/src/extension-accessor.ts index f45debc4d..946d376a2 100644 --- a/packages/protobuf/src/extension-accessor.ts +++ b/packages/protobuf/src/extension-accessor.ts @@ -98,7 +98,7 @@ export function setExtension, V>( const reader = readOpt.readerFactory(writer.finish()); while (reader.pos < reader.len) { const [no, wireType] = reader.tag(); - const data = reader.skip(wireType); + const data = reader.skip(wireType, no); message.getType().runtime.bin.onUnknownField(message, no, wireType, data); } } diff --git a/packages/protobuf/src/private/binary-format.ts b/packages/protobuf/src/private/binary-format.ts index 069f700c4..5e113d0f3 100644 --- a/packages/protobuf/src/private/binary-format.ts +++ b/packages/protobuf/src/private/binary-format.ts @@ -105,12 +105,15 @@ export function makeBinaryFormat(): BinaryFormat { let fieldNo: number | undefined, wireType: WireType | undefined; while (reader.pos < end) { [fieldNo, wireType] = reader.tag(); - if (wireType == WireType.EndGroup) { + if ( + delimitedMessageEncoding === true && + wireType == WireType.EndGroup + ) { break; } const field = type.fields.find(fieldNo); if (!field) { - const data = reader.skip(wireType); + const data = reader.skip(wireType, fieldNo); if (options.readUnknownFields) { this.onUnknownField(message, fieldNo, wireType, data); }