Do not allocate from sizes a peer declares in the Native protocol - #117011
Do not allocate from sizes a peer declares in the Native protocol#117011alexey-milovidov wants to merge 1 commit into
Conversation
A client allocates from sizes that the server puts on the wire, and the client has no memory limit to stop it, so a small packet could make it allocate gigabytes: - The strings of an exception, which is the first thing a client can receive during the handshake, were resized to their declared size before the text arrived. Read them as the bytes arrive instead. - The decompressed size of a compressed block was not bounded, while the compressed size was. Bound it the same way. - The row count of a block was only checked against a limit of a thousand billion rows, and the bulk deserialization resizes the column to it before reading. Lower the limit to a billion rows, which is already far above anything ClickHouse produces, and stop preallocating the columns from a row count that has not been backed by any data yet.
|
Workflow [PR], commit [fb3a21b] Summary: ✅
AI ReviewSummaryThis PR hardens a few client-side Findings❌ Blockers
Final Verdict
LLVM Coverage ReportMeasured on commit fb3a21b.
Changed lines: Changed C/C++ lines covered: 36/40 (90.00%) · Uncovered code |
| * declares a huge decompressed size makes the reader allocate that much from a tiny payload. | ||
| * Blocks are written with `max_compress_block_size`, which is 1 MB by default. | ||
| */ | ||
| constexpr uint64_t DBMS_MAX_DECOMPRESSED_SIZE = 0x40000000ULL; /// 1GB |
There was a problem hiding this comment.
This still leaves the original allocation bug in place at the new boundary. CompressedReadBuffer::nextImpl and CompressedReadBufferFromFile::nextImpl both call memory.resize(size_decompressed + ...) immediately after readCompressedData, so a block that declares exactly 0x40000000 bytes here still forces a ~1 GiB allocation before any real payload is validated. That only moves the cliff from 4 GiB to 1 GiB; it does not satisfy the PR contract that a peer-declared size should not become an allocation on its own.
I think this needs either a much smaller byte budget that matches supported writers, or a decompression path that does not pre-size the destination buffer from the header alone.
| /// Not `uz`: the value does not fit into `size_t` on 32-bit platforms, and it is compared against | ||
| /// a row count read from the wire as `UInt64`. | ||
| static constexpr auto DEFAULT_NATIVE_BINARY_MAX_NUM_ROWS = 1'000'000'000'000ULL; | ||
| static constexpr auto DEFAULT_NATIVE_BINARY_MAX_NUM_ROWS = 1'000'000'000ULL; |
There was a problem hiding this comment.
Lowering the cap to 1'000'000'000 does not actually close the NativeReader side of the bug. NativeReader::readData still forwards rows into deserializeBinaryBulkWithMultipleStreams, and SerializationNumber<T>::deserializeBinaryBulk starts with x.resize(initial_size + limit) in src/DataTypes/Serializations/SerializationNumber.cpp. So a Native block that declares exactly 1e9 rows for a single UInt64 column still allocates about 8 GiB before any column bytes are read. The new test only exercises 1e12, but the header/payload asymmetry remains at the new boundary.
This probably needs a byte-budget-based cap, or the bulk deserializers need to grow from bytes actually read instead of trusting rows.
| throw Exception(ErrorCodes::TOO_LARGE_SIZE_COMPRESSED, "Too large size_compressed_without_checksum: {}. " | ||
| "Most likely corrupted data.", size_compressed_without_checksum); | ||
|
|
||
| if (size_decompressed > DBMS_MAX_DECOMPRESSED_SIZE) |
There was a problem hiding this comment.
Putting this cap in the shared compressed-reader path is a backward-compatibility break: current writers still allow blocks larger than 1 GiB. CompressedWriteBuffer::nextImpl accepts any offset() <= INT_MAX, StorageLog passes max_compress_block_size through without clamping, and clickhouse-compressor --block-size is also an unconstrained size_t. That means previously readable compressed data can start failing after upgrade even though the format itself did not change.
If the goal is to harden untrusted Native or network reads, I think the tighter bound has to live on those untrusted entrypoints, or this needs a compatibility knob or migration story for generic compressed files.
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Compile time of recompiled translation units3967 translation units recompiled, 18580 s compile time in total, 3966 of them have a recent master baseline. |
Closes: https://github.com/ClickHouse/clickhouse-private/issues/71254
A client allocates from sizes that the server puts on the wire, before the corresponding payload arrives, and the client has no memory limit to stop it.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Sizes declared by a server in the Native protocol no longer make the client allocate memory for data that has not arrived: exception strings are read as they arrive, the decompressed size of a block is bounded, and the maximum number of rows in a
Nativeblock is now a billion.Workflow [PR]
Sync PR [sync-upstream/pr/117011]