Skip to content

Fix client v2 varint overflow#2894

Open
romka wants to merge 3 commits into
ClickHouse:mainfrom
romka:fix-client-v2-varint-overflow
Open

Fix client v2 varint overflow#2894
romka wants to merge 3 commits into
ClickHouse:mainfrom
romka:fix-client-v2-varint-overflow

Conversation

@romka

@romka romka commented Jun 25, 2026

Copy link
Copy Markdown

Summary

Fixes BinaryStreamReader.readVarInt in client-v2 so malformed or overflowing varints are rejected instead of being decoded into corrupted or negative int values.

The previous implementation accumulated into an int while looping up to 10 bytes. For bytes after the 5th, Java masks int shift distances modulo 32, so overlong varints could corrupt lower bits. It also allowed 5-byte values above Integer.MAX_VALUE, which can become negative sizes/counts.

There is no related issue.

Changes

  • Limit readVarInt decoding to 5 bytes.
  • Reject fifth-byte payloads that exceed non-negative int range or continue past 5 bytes.
  • Add focused tests for:
    • Integer.MAX_VALUE
    • overflow above Integer.MAX_VALUE
    • overlong varint encoding

Compatibility

This changes handling of malformed/overflowing binary varints from silent decoding to IOException. Valid non-negative int varints are unchanged.

Testing

  • mvn -Duser.timezone=UTC -pl client-v2 -am -Dtest=BinaryStreamReaderTests -Dsurefire.failIfNoSpecifiedTests=false test
  • mvn -Duser.timezone=UTC -DskipITs clean verify

Checklist

  • Unit tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG

@romka romka requested review from chernser and mzitnik as code owners June 25, 2026 17:25
@github-actions

Copy link
Copy Markdown

Repository collaborators can run the JMH benchmark suite against this PR by commenting:

/benchmark

Optional regression threshold override (Δ% on Time or Alloc/op; defaults to 10%):

/benchmark threshold=15

Only one benchmark run per PR is active at a time — issuing a new /benchmark comment cancels the previous run. After the run finishes a separate comment will be posted comparing it against the latest scheduled run on main; the PR check fails if any benchmark regresses by more than the threshold.

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


romka seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

2 participants