Repository navigation
Conversation
ColumnVector, ColumnEnum and ColumnFixedString resized to the declared row count before reading, ColumnString reserved the declared row count and allocated each declared string length up front, and WireFormat::ReadString resized to the declared length, so a packet of a few dozen bytes could make the client allocate gigabytes. I added WireFormat::ReadPODArray, which starts at 1 MiB, reserves eightfold ahead and zero-fills only what it is about to read, and use it for these bodies and for ReadString. ColumnString caps its initial row reservation and reads strings above 1 MiB into a block of their own that grows the same way. Default 64K-row blocks fit the first step and cost nothing extra; a single 80 MB column body copies 73 MiB while growing. LowCardinality's Load also ignored a short index column, which with incremental loading would deliver fewer rows than declared, so it now throws ProtocolError.
Client::Impl::ReadBlock() reads the block's column and row counts as uint64_t and passes the row count to Column::Load(), which takes a size_t. On 32-bit, a server that declares 2^41 rows has the count truncated to 0, and the client returns an empty block instead of an error. Counts that do not survive the conversion now throw ProtocolError. The test only runs where size_t is narrower than 64 bits and skips elsewhere.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Several column loaders size their buffers from server-declared counts before any data has arrived. A packet of a few dozen bytes can therefore make the client allocate gigabytes:
ColumnVector,ColumnEnumandColumnFixedStringLoadBodyresize to the declared row count.ColumnString::LoadBodyreserves the declared row count and allocates each declared string length up front.WireFormat::ReadStringzero-fills up to the declared length.For example, a UInt64 Data block that declares 2^28 rows and then sends nothing reaches 2 GiB of resident memory in under a second. So does a single 2^31-byte String length or
FixedString(2147483648)with one row.Fix: new
WireFormat::ReadPODArraystarts at 1 MiB, reserves eightfold ahead as data arrives, and zero-fills only the chunk it's about to read. The column bodies andReadStringuse it.ColumnStringcaps its initial row reservation and reads strings above 1 MiB into a dedicated block that grows the same way. The growth arithmetic (WireFormat::NextReadCapacity) can't overflow a 32-bitsize_t.Cost: default 64K-row blocks fit the first step and pay nothing extra. A single 80 MB column body copies 73 MiB while growing (1 + 8 + 64 MiB).
ColumnLowCardinality::Loadalso ignored the result of loading the index column. With incremental loading a truncated index would come back shorter than the declared row count, so it now throwsProtocolError.On 32-bit,
Client::Impl::ReadBlock()truncated theuint64_tcolumn and row counts tosize_t, so a block declaring 2^41 rows came back empty instead of failing. Counts that don't fit now throwProtocolError.The FixedString width × rows overflow check is left to #579.
Tests:
ut/allocation_tracker.cpprecords the largest allocation inside a scope. It self-checks that it sees library allocations, and the tests that depend on it skip if it doesn't (for example with a DLL build).actual: 134217728 vs 8388608.ColumnLowCardinalityString_Load_TruncatedIndexcovers the short index.CodedStreamCase.NextReadCapacityDoesNotOverflowchecks the growth arithmetic inuint32_t.ClientProtocol.RowCountBeyondSizeTThrowsProtocolErrorruns only wheresize_tis narrower than 64 bits and skips elsewhere. I ran it in an i386 build (with-Werroroff, since master doesn't build warning-clean there): it fails on master and passes with the fix.