Repository navigation
Conversation
Load() in columns/lowcardinality.cpp kept the index column from the wire without checking its values against the dictionary it had just read, and GetItem() passed them straight to the dictionary column. With a FixedString(N) dictionary, index * N can wrap to an in-range offset, so At() returns a view that runs past the dictionary buffer. Load() now rejects any index at or above the dictionary size with a ProtocolError, fails when the index column does not load, and switches on the raw index type value instead of casting it to IndexType first.
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.
Load()incolumns/lowcardinality.cppkeeps the index column from the wire without checking its values against the dictionary it just read, andGetItem()passes them straight to the dictionary column. Two things go wrong. An index past the end of the dictionary throwsstd::out_of_rangefrom deep inside the column. Worse, with aFixedString(N)dictionary,index * Ncan wrap to an in-range offset, andAt()then returns a view that runs past the dictionary buffer: ASan reports a heap-buffer-overflow READ of N bytes.Fix:
Load()checks every index against the dictionary size in one pass over the typed index column and throwsProtocolErrorfor any index at or above it.Load()also fails when the index column itself doesn't load. This hunk overlaps Grow column bodies and wire strings as the data arrives #591, which makes the same change, so whichever lands second drops it.createIndexColumn()switches on the raw index-type value instead of casting values 4..255 to theIndexTypeenum first.Reproducer: a
LowCardinality(FixedString(1001))block with a 2-entry dictionary, a UInt64 index type, and an index whose product with 1001 wraps modulo 2^64 to byte offset 1500 of the 2002-byte dictionary. Simpler variants are a UInt8 index of 2 into a 2-entry dictionary, or a String dictionary with an out-of-range index.Tests:
ColumnLowCardinalityFixedString_LoadRejectsIndexOutOfRangeandColumnLowCardinalityString_LoadRejectsIndexOutOfRangebuild the wire bytes with UInt8 and UInt64 index widths. They cover the wrapping case, an index just past the end, and plain and Nullable String dictionaries. Valid controls, including a Nullable index 0, still load. Both tests fail on master.