Skip to content

fix(wire): reject over-long and over-large varints in ConsumeVarint - #31

Open
linhongyu510 wants to merge 1 commit into
cloudwego:mainfrom
linhongyu510:fix/varint-overflow-check
Open

linhongyu510 wants to merge 1 commit into
cloudwego:mainfrom
linhongyu510:fix/varint-overflow-check

Conversation

@linhongyu510

@linhongyu510 linhongyu510 commented Oct 8, 2026 •

Copy link
Copy Markdown

Problem

ConsumeVarint diverged from the reference google.golang.org/protobuf/encoding/protowire decoder:

  1. No 10-byte cap. The loop reads continuation bytes indefinitely. At i=10+, i*7 >= 70\], and Go's unsigned shift wraps modulo 64, so later bytes OR stray bits into already-populated positions of v`.

  2. No 10th-byte value check. The reference requires the 10th byte's lower 7 bits to be 0 or 1 (only bit 63 can be set in a 64-bit varint). FastPB accepted any value.

Reproduction (before fix)

Input fastpb protowire reference
[0x80×9, 0x02] (10B, b[9]=2) (v=0, n=10) accepted (v=0, n=-3) overflow
[0x80×9, 0x7f] (10B, b[9]=127) (v=2^63, n=10) wrong value (v=0, n=-3) overflow
[0x80×10, 0x01] (11B) (v=0, n=11) accepted (v=0, n=-3) overflow
[0x80×9, 0x01] (max uint64) (v=2^63, n=10) (v=2^63, n=10) ✓ match

Fix

Limit the scan loop to the first 9 bytes (the hot path for all field tags and most values), then check the 10th byte separately. The 10th byte can only contribute bit 63 (value 0 or 1); any larger value or a 10th continuation byte returns -3 (errCodeOverflow).

This matches the reference protowire structure: the short-path loop body is unchanged from the original (no extra per-iteration comparison).

Performance

Benchstat (3 runs each, Apple M5 Pro / arm64, 2s benchtime):

Input base (ns/op) fix (ns/op) delta
tag (1 byte) 0.645 0.605 -6.2%
tag (2 bytes) 0.993 0.929 -6.4%
varint (1 byte) 0.633 0.608 -4.0%
varint (5 bytes) 2.234 2.045 -8.5%
valid 10-byte max 4.268 3.784 -11.3%
invalid 10-byte overflow 4.215 3.688 -12.5%

No hot-path regression. The i < 9 loop bound gives the compiler a known small trip count, which improves codegen.

Verification

  • go build ./... clean (amd64, arm64, 386)
  • go vet clean
  • go test . all pass
  • go test -race . clean
  • Revert proof: restoring original wire.go fails all 3 overflow subcases
  • Pure Go, no assembly, no platform-specific code

@linhongyu510
linhongyu510 requested a review from a team as a code owner October 8, 2026 09:21
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@linhongyu510
linhongyu510 force-pushed the fix/varint-overflow-check branch from 17e129b to 47e6ca2 Compare October 8, 2026 09:25
ConsumeVarint previously had no length cap and no 10th-byte validation,
diverging from the reference google.golang.org/protobuf/encoding/protowire
decoder:

- A 10-byte varint whose 10th byte had value >= 2 was silently accepted
  with corrupted bits (shift-by >= 64 wraps modulo 64, ORing stray bits).
- An 11+ byte varint was silently parsed, with later bytes ORing into
  already-used bit positions.

The reference decoder returns errCodeOverflow (-3) in both cases.

Fix: limit the scan loop to the first 9 bytes (the hot path for all
field tags and most values), then check the 10th byte separately.
The 10th byte can only contribute bit 63 (value 0 or 1); any larger
value or a 10th continuation byte returns -3.

Performance: benchstat (3 runs each, arm64) shows no regression on
1-5 byte varints (within noise or slightly faster); the loop-bound
hint helps the compiler optimize the short path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants