Skip to content

Reject LowCardinality indexes outside the dictionary - #593

Open
iliaal wants to merge 1 commit into
ClickHouse:masterfrom
iliaal:fix/lowcardinality-index-bounds
Open

iliaal wants to merge 1 commit into
ClickHouse:masterfrom
iliaal:fix/lowcardinality-index-bounds

Conversation

@iliaal

@iliaal iliaal commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Load() in columns/lowcardinality.cpp keeps the index column from the wire without checking its values against the dictionary it just read, and GetItem() passes them straight to the dictionary column. Two things go wrong. An index past the end of the dictionary throws std::out_of_range from deep inside the column. Worse, with a FixedString(N) dictionary, index * N can wrap to an in-range offset, and At() 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 throws ProtocolError for 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 the IndexType enum 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_LoadRejectsIndexOutOfRange and ColumnLowCardinalityString_LoadRejectsIndexOutOfRange build 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.

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.
@iliaal
iliaal requested review from mzitnik and slabko as code owners October 18, 2026 20:38
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.

1 participant