Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@

### Bug Fixes

- **[client-v2]** Fixed binary varint decoding for length and count fields so overflowing or overlong values fail with an `IOException` instead of being decoded into corrupted or negative `int` values.

- **[jdbc-v2]** Fixed `Statement.cancel()` throwing `SESSION_IS_LOCKED` when the statement was running inside a ClickHouse session. The driver now accepts `session_id`, `session_check`, and `session_timeout` as first-class connection properties and correctly suppresses them when issuing a `KILL QUERY` during cancellation. This ensures the cancellation request runs outside the session and no longer contends with the running query for the session lock. (https://github.com/ClickHouse/clickhouse-java/issues/2690, https://github.com/ClickHouse/clickhouse-java/issues/2881)

- **[client-v2]** Fixed inconsistent use of `executionTimeout` parameter in `Client` component. The timeout was previously set in milliseconds but mistakenly retrieved and used in seconds in some places. Now it correctly uses milliseconds consistently. (https://github.com/ClickHouse/clickhouse-java/issues/2358)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -959,16 +959,20 @@ private double[][][][] readGeoMultiPolygon() throws IOException {
public static int readVarInt(InputStream input) throws IOException {
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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Please find more details in the issue: #2902.

The tests in this PR demonstrate two cases that are currently decoded incorrectly:

  • Overflow: 80 80 80 80 08,
  • Overlong encoding: 80 80 80 80 80 01.

I agree that if any of these readers are expected to decode protocol-level 64-bit varints, then introducing a dedicated long-returning reader (and updating the relevant call sites) would be a better long-term solution. My goal in this PR was to make the current int-returning implementation fail explicitly instead of silently returning a corrupted value.

byte b = (byte) readByteOrEOF(input);
if (i == 4 && (b & 0xF8) != 0) {
throw new IOException("VarInt is too large for int");
}

value |= (b & 0x7F) << (7 * i);

if ((b & 0x80) == 0) {
break;
return value;
}
}

return value;
throw new IOException("Malformed VarInt");
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,4 +204,26 @@ public void testReadNullVariantReturnsNull() throws Exception {

Assert.assertNull(reader.readValue(column));
}

@Test
public void testReadVarIntReadsMaxInt() throws IOException {
Assert.assertEquals(readVarInt((byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0x07),
Integer.MAX_VALUE);
}

@Test
public void testReadVarIntRejectsOverflow() {
Assert.assertThrows(IOException.class,
() -> readVarInt((byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x08));
}

@Test
public void testReadVarIntRejectsOverlongValue() {
Assert.assertThrows(IOException.class,
() -> readVarInt((byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x01));
}

private static int readVarInt(byte... bytes) throws IOException {
return BinaryStreamReader.readVarInt(new ByteArrayInputStream(bytes));
}
}
Loading