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

CLAassistant commented Jun 25, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@chernser

Copy link
Copy Markdown
Contributor

@romka

Thank you for the contribution! We will review the PR.
Please create an issue so this PR is not orphan.

Thanks!

int value = 0;

for (int i = 0; i < 10; i++) {
for (int i = 0; i < 5; i++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what var int could be in data from server?
problem may be we need to adjust using 64-bit varint.

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.

3 participants