Repository navigation
Conversation
A server-supplied FixedString(0) creates a ColumnFixedString with a zero width, and ColumnFixedString::Size() divides by it as soon as Block::AppendColumn() sees the zero-row header block. A malformed width such as FixedString(-) makes std::stol throw out of TypeParser::Parse(). ParseTypeName() inserts the cache entry before parsing and erases it only when Parse() returns false, so the throw leaves a half-built AST cached and the next query with the same type hits the same division. Number tokens now parse with std::from_chars, and invalid or out-of-range values fail the parse instead of throwing, which lets the existing erase path run. ParseTypeName() also parses into a local and caches only on success, so an exception from the parser can no longer leave an entry behind. The factory rejects sizes outside 1..0xFFFFFF, the server's own DataTypeFixedString limit (MAX_FIXEDSTRING_SIZE), and the ColumnFixedString constructor rejects 0. The server never accepts FixedString(0), so the tests that built one now use FixedString(1).
The 1..0xFFFFFF FixedString check only ran in CreateTerminalColumn(). With low_cardinality_as_wrapped_column set, CreateColumnFromAst() builds LowCardinality(FixedString(N)) straight from the AST value, so a server could declare any width and the client would allocate it on the first load. I moved the check into GetFixedStringSize() and call it from both paths.
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
1 participant
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.
A server that sends a column typed
FixedString(0)crashes the client with SIGFPE, even in an empty result.ColumnFixedString::Size()divides by the width, andBlock::AppendColumn()calls it on the zero-row header block.A malformed width is worse because it poisons the type cache. For
FixedString(-),std::stolthrows out ofTypeParser::Parse().ParseTypeName()intypes/type_parser.cppinserts the cache entry before parsing and only erases it whenParse()returns false, so the half-built AST stays cached. The first query throwsstol; the second identical query hits the zero division.Fix:
std::from_chars. Invalid or out-of-range values fail the parse instead of throwing, so the existing erase path runs. It also removes the 32-bitlonglimit on Windows, wherestd::stolthrew for widths above 2^31.ParseTypeName()parses into a local and caches only on success, so an exception can't leave an entry behind.low_cardinality_as_wrapped_columnset,CreateColumnFromAst()builtLowCardinality(FixedString(N))straight from the AST value and skipped the width bound, so the first load allocated whatever width the server declared. The check now lives inGetFixedStringSize(), which both paths call.MAX_FIXEDSTRING_SIZE, withValidationError. TheColumnFixedStringconstructor rejects 0.The server never accepts
FixedString(0), so the existing tests that built one now useFixedString(1).Reproducer:
CreateColumnByType("FixedString(0)")->Size(), or a server Data block whose column type isFixedString(0). For the cache case, callCreateColumnByType("FixedString(-)")twice.Tests:
TypeParser.InvalidNumber,ParseTypeName.MalformedNumberReturnsNull,CreateColumnByType.FixedStringSize,CreateColumnByType.MalformedFixedStringSize,CreateColumnByType.FixedStringSizeLimit,CreateColumnByType.LowCardinalityFixedStringSizeLimitandColumnsCase.FixedString_Size_Eq0_Throws. On master,FixedStringSizedies with SIGFPE and the others fail.