From 6c422b7247385e391f6f2184c84dfb4b2f544387 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Sun, 18 Oct 2026 11:31:18 -0400 Subject: [PATCH 1/2] Reject zero, oversized, and malformed FixedString sizes 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). --- clickhouse/columns/factory.cpp | 11 +++++++-- clickhouse/columns/string.cpp | 1 + clickhouse/types/type_parser.cpp | 17 ++++++++------ ut/CreateColumnByType_ut.cpp | 40 +++++++++++++++++++++++++------- ut/columns_ut.cpp | 8 +++++-- ut/type_parser_ut.cpp | 24 +++++++++++++++++++ ut/types_ut.cpp | 7 +++--- 7 files changed, 86 insertions(+), 22 deletions(-) diff --git a/clickhouse/columns/factory.cpp b/clickhouse/columns/factory.cpp index 2fb0d39f..d7a13973 100644 --- a/clickhouse/columns/factory.cpp +++ b/clickhouse/columns/factory.cpp @@ -91,8 +91,15 @@ static ColumnRef CreateTerminalColumn(const TypeAst& ast) { case Type::String: return std::make_shared(); - case Type::FixedString: - return std::make_shared(GetASTChildElement(ast, 0).value); + case Type::FixedString: { + // Same bounds as the server's DataTypeFixedString (MAX_FIXEDSTRING_SIZE). + constexpr int64_t kMaxFixedStringSize = 0xFFFFFF; + const auto size = GetASTChildElement(ast, 0).value; + if (size <= 0 || size > kMaxFixedStringSize) + throw ValidationError("FixedString size must be between 1 and " + std::to_string(kMaxFixedStringSize) + ", got " + + std::to_string(size)); + return std::make_shared(static_cast(size)); + } case Type::DateTime: if (ast.elements.empty()) { diff --git a/clickhouse/columns/string.cpp b/clickhouse/columns/string.cpp index e62a9b2b..9c7d739a 100644 --- a/clickhouse/columns/string.cpp +++ b/clickhouse/columns/string.cpp @@ -29,6 +29,7 @@ ColumnFixedString::ColumnFixedString(size_t n) : Column(Type::CreateString(n)) , string_size_(n) { + if (n == 0) throw ValidationError("FixedString size must be positive"); } void ColumnFixedString::Reserve(size_t new_cap) { diff --git a/clickhouse/types/type_parser.cpp b/clickhouse/types/type_parser.cpp index d305a398..c103ea05 100644 --- a/clickhouse/types/type_parser.cpp +++ b/clickhouse/types/type_parser.cpp @@ -16,6 +16,7 @@ #include #endif +#include namespace clickhouse { @@ -200,10 +201,13 @@ bool TypeParser::Parse(TypeAst* type) { type_->name = token.value.to_string(); type_->code = GetTypeCode(type_->name); break; - case Token::Number: + case Token::Number: { type_->meta = TypeAst::Number; - type_->value = std::stol(token.value.to_string()); + const char* last = token.value.data() + token.value.size(); + const auto [ptr, ec] = std::from_chars(token.value.data(), last, type_->value); + if (ec != std::errc() || ptr != last) return false; break; + } case Token::String: type_->meta = TypeAst::String; type_->value_string = std::string(token.value); @@ -368,12 +372,11 @@ const TypeAst* ParseTypeName(const std::string& type_name) { return &it->second; } - auto& ast = ast_cache[type_name]; - if (TypeParser(type_name).Parse(&ast)) { - return * + TypeAst ast; + if (!TypeParser(type_name).Parse(&ast)) { + return nullptr; } - ast_cache.erase(type_name); - return nullptr; + return &ast_cache.emplace(type_name, std::move(ast)).first->second; } } diff --git a/ut/CreateColumnByType_ut.cpp b/ut/CreateColumnByType_ut.cpp index 0f1fbc4f..d3e70a7d 100644 --- a/ut/CreateColumnByType_ut.cpp +++ b/ut/CreateColumnByType_ut.cpp @@ -64,6 +64,34 @@ TEST(CreateColumnByType, UnmatchedBrackets) { ASSERT_EQ(nullptr, CreateColumnByType("Array(LowCardinality(Nullable(FixedString(10000)))")); } +TEST(CreateColumnByType, FixedStringSize) { + const auto col = CreateColumnByType("FixedString(16)"); + ASSERT_NE(nullptr, col); + EXPECT_EQ(16u, col->As()->FixedSize()); + + EXPECT_THROW(CreateColumnByType("FixedString(0)"), ValidationError); + EXPECT_THROW(CreateColumnByType("FixedString(-1)"), ValidationError); + EXPECT_THROW(CreateColumnByType("Nullable(FixedString(0))"), ValidationError); + EXPECT_THROW(CreateColumnByType("LowCardinality(FixedString(0))"), ValidationError); +} + +TEST(CreateColumnByType, FixedStringSizeLimit) { + const auto col = CreateColumnByType("FixedString(16777215)"); + ASSERT_NE(nullptr, col); + EXPECT_EQ(16777215u, col->As()->FixedSize()); + + EXPECT_THROW(CreateColumnByType("FixedString(16777216)"), ValidationError); +} + +TEST(CreateColumnByType, MalformedFixedStringSize) { + // Repeated on purpose: a failed parse must not leave a cached AST behind. + for (int i = 0; i < 2; ++i) { + SCOPED_TRACE(i); + EXPECT_EQ(nullptr, CreateColumnByType("FixedString(-)")); + EXPECT_EQ(nullptr, CreateColumnByType("FixedString(99999999999999999999)")); + } +} + TEST(CreateColumnByType, LowCardinalityAsWrappedColumn) { CreateColumnByTypeSettings create_column_settings; create_column_settings.low_cardinality_as_wrapped_column = true; @@ -263,14 +291,10 @@ INSTANTIATE_TEST_SUITE_P(Basic, CreateColumnByTypeWithName, ::testing::Values( INSTANTIATE_TEST_SUITE_P(BasicBool, CreateColumnByTypeWithName, ::testing::Values("Bool")); #endif -INSTANTIATE_TEST_SUITE_P(Parametrized, CreateColumnByTypeWithName, ::testing::Values( - "FixedString(0)", "FixedString(10000)", - "DateTime('UTC')", "DateTime64(3, 'UTC')", - "Decimal(9,3)", "Decimal(18,3)", - "Enum8('ONE' = 1, 'TWO' = 2)", - "Enum16('ONE' = 1, 'TWO' = 2, 'THREE' = 3, 'FOUR' = 4)" -)); - +INSTANTIATE_TEST_SUITE_P(Parametrized, CreateColumnByTypeWithName, + ::testing::Values("FixedString(1)", "FixedString(10000)", "DateTime('UTC')", "DateTime64(3, 'UTC')", + "Decimal(9,3)", "Decimal(18,3)", "Enum8('ONE' = 1, 'TWO' = 2)", + "Enum16('ONE' = 1, 'TWO' = 2, 'THREE' = 3, 'FOUR' = 4)")); INSTANTIATE_TEST_SUITE_P(Nested, CreateColumnByTypeWithName, ::testing::Values( "Nullable(FixedString(10000))", diff --git a/ut/columns_ut.cpp b/ut/columns_ut.cpp index 57650ea9..c4b1e09d 100644 --- a/ut/columns_ut.cpp +++ b/ut/columns_ut.cpp @@ -303,8 +303,12 @@ TEST(ColumnsCase, FixedString_Append_LargeString) { EXPECT_ANY_THROW(col->Append("this is a long string")); } -TEST(ColumnsCase, FixedString_Type_Size_Eq0) { - const auto col = std::make_shared(0); +TEST(ColumnsCase, FixedString_Size_Eq0_Throws) { + EXPECT_THROW(ColumnFixedString(0), ValidationError); +} + +TEST(ColumnsCase, FixedString_Type_Size_Eq1) { + const auto col = std::make_shared(1); ASSERT_EQ(col->FixedSize(), col->Type()->As()->GetSize()); } diff --git a/ut/type_parser_ut.cpp b/ut/type_parser_ut.cpp index 76bedd7a..fa55a456 100644 --- a/ut/type_parser_ut.cpp +++ b/ut/type_parser_ut.cpp @@ -541,6 +541,30 @@ TEST(ParseTypeName, EmptyName) { EXPECT_EQ(nullptr, ParseTypeName(std::string(5, '\0'))); } +TEST(TypeParser, InvalidNumber) { + TypeAst ast; + EXPECT_FALSE(TypeParser("FixedString(-)").Parse(&ast)); + + TypeAst overflow; + EXPECT_FALSE(TypeParser("FixedString(99999999999999999999)").Parse(&overflow)); + + TypeAst negative; + ASSERT_TRUE(TypeParser("Enum8('a' = -128)").Parse(&negative)); + ASSERT_EQ(negative.elements.size(), 2u); + EXPECT_EQ(negative.elements[1].value, -128); +} + +TEST(ParseTypeName, MalformedNumberReturnsNull) { + EXPECT_EQ(nullptr, ParseTypeName("FixedString(-)")); + EXPECT_EQ(nullptr, ParseTypeName("FixedString(-)")); + + const TypeAst* ast = ParseTypeName("FixedString(16)"); + ASSERT_NE(nullptr, ast); + EXPECT_EQ(ast, ParseTypeName("FixedString(16)")); + ASSERT_EQ(ast->elements.size(), 1u); + EXPECT_EQ(ast->elements[0].value, 16); +} + TEST(TypeParser, AggregateFunction) { { TypeAst ast; diff --git a/ut/types_ut.cpp b/ut/types_ut.cpp index 51ee3aff..6dbe6408 100644 --- a/ut/types_ut.cpp +++ b/ut/types_ut.cpp @@ -184,9 +184,9 @@ TEST(TypesCase, IsEqual) { #endif "UInt8", "Int8", -// "UInt128", + // "UInt128", "String", - "FixedString(0)", + "FixedString(1)", "FixedString(10000)", "DateTime('UTC')", "DateTime64(3, 'UTC')", @@ -208,7 +208,8 @@ TEST(TypesCase, IsEqual) { "Array(Array(Nullable(Tuple(String, Int8, Date, DateTime))))", "Array(Array(Array(Nullable(Tuple(String, Int8, Date, DateTime)))))", "Array(Array(Array(Array(Nullable(Tuple(String, Int8, Date, DateTime('UTC')))))))" - "Array(Array(Array(Array(Nullable(Tuple(String, Int8, Date, DateTime('UTC'), Tuple(LowCardinality(String), Enum8('READ'=1, 'WRITE'=0))))))))", + "Array(Array(Array(Array(Nullable(Tuple(String, Int8, Date, DateTime('UTC'), Tuple(LowCardinality(String), Enum8('READ'=1, " + "'WRITE'=0))))))))", "Map(String, Int8)", "Map(String, Tuple(String, Int8, Date, DateTime))", "Map(UUID, Array(Tuple(String, Int8, Date, DateTime)))", From e3adba0ccbf856b5029a0d1fe3b24571204d4219 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Sun, 18 Oct 2026 16:54:00 -0400 Subject: [PATCH 2/2] Bound the FixedString size under LowCardinality as well 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. --- clickhouse/columns/factory.cpp | 23 +++++++++++++---------- ut/CreateColumnByType_ut.cpp | 19 +++++++++++++++++++ 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/clickhouse/columns/factory.cpp b/clickhouse/columns/factory.cpp index d7a13973..90ab1826 100644 --- a/clickhouse/columns/factory.cpp +++ b/clickhouse/columns/factory.cpp @@ -46,6 +46,16 @@ const auto& GetASTChildElement(const TypeAst & ast, int position) { return ast.elements[static_cast(position)]; } +size_t GetFixedStringSize(const TypeAst& ast) { + // Same bounds as the server's DataTypeFixedString (MAX_FIXEDSTRING_SIZE). + constexpr int64_t kMaxFixedStringSize = 0xFFFFFF; + const auto size = GetASTChildElement(ast, 0).value; + if (size <= 0 || size > kMaxFixedStringSize) + throw ValidationError("FixedString size must be between 1 and " + std::to_string(kMaxFixedStringSize) + ", got " + + std::to_string(size)); + return static_cast(size); +} + static ColumnRef CreateTerminalColumn(const TypeAst& ast) { switch (ast.code) { case Type::Void: @@ -91,15 +101,8 @@ static ColumnRef CreateTerminalColumn(const TypeAst& ast) { case Type::String: return std::make_shared(); - case Type::FixedString: { - // Same bounds as the server's DataTypeFixedString (MAX_FIXEDSTRING_SIZE). - constexpr int64_t kMaxFixedStringSize = 0xFFFFFF; - const auto size = GetASTChildElement(ast, 0).value; - if (size <= 0 || size > kMaxFixedStringSize) - throw ValidationError("FixedString size must be between 1 and " + std::to_string(kMaxFixedStringSize) + ", got " + - std::to_string(size)); - return std::make_shared(static_cast(size)); - } + case Type::FixedString: + return std::make_shared(GetFixedStringSize(ast)); case Type::DateTime: if (ast.elements.empty()) { @@ -232,7 +235,7 @@ static ColumnRef CreateColumnFromAst(const TypeAst& ast, CreateColumnByTypeSetti case Type::String: return std::make_shared>(); case Type::FixedString: - return std::make_shared>(GetASTChildElement(nested, 0).value); + return std::make_shared>(GetFixedStringSize(nested)); case Type::Nullable: throw UnimplementedError("LowCardinality(" + nested.name + ") is not supported with LowCardinalityAsWrappedColumn on"); default: diff --git a/ut/CreateColumnByType_ut.cpp b/ut/CreateColumnByType_ut.cpp index d3e70a7d..7c0b6614 100644 --- a/ut/CreateColumnByType_ut.cpp +++ b/ut/CreateColumnByType_ut.cpp @@ -83,6 +83,25 @@ TEST(CreateColumnByType, FixedStringSizeLimit) { EXPECT_THROW(CreateColumnByType("FixedString(16777216)"), ValidationError); } +TEST(CreateColumnByType, LowCardinalityFixedStringSizeLimit) { + CreateColumnByTypeSettings wrapped; + wrapped.low_cardinality_as_wrapped_column = true; + + const std::pair cases[] = { + {CreateColumnByTypeSettings{}, "LowCardinality(FixedString(16777215))"}, + {wrapped, "FixedString(16777215)"}, + }; + for (const auto& [settings, expected_type] : cases) { + SCOPED_TRACE(expected_type); + const auto col = CreateColumnByType("LowCardinality(FixedString(16777215))", settings); + ASSERT_NE(nullptr, col); + EXPECT_EQ(expected_type, col->GetType().GetName()); + + EXPECT_THROW(CreateColumnByType("LowCardinality(FixedString(16777216))", settings), ValidationError); + } + EXPECT_THROW(CreateColumnByType("LowCardinality(Nullable(FixedString(16777216)))"), ValidationError); +} + TEST(CreateColumnByType, MalformedFixedStringSize) { // Repeated on purpose: a failed parse must not leave a cached AST behind. for (int i = 0; i < 2; ++i) {