From 6c422b7247385e391f6f2184c84dfb4b2f544387 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Sun, 18 Oct 2026 11:31:18 -0400 Subject: [PATCH 1/3] 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/3] 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) { From cf7ea8cf525a27c0fe16b2f8b4e152613ef97f51 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Sun, 18 Oct 2026 11:47:04 -0400 Subject: [PATCH 3/3] Bound the parsed type AST cache ParseTypeName() keeps every distinct type name it parses in a process-wide map with no eviction, so a server that sends many distinct type names, one zero-row block each, grows client memory without bound. The cache now holds at most kTypeAstCacheMaxEntries entries and kTypeAstCacheMaxBytes of type-name text. An insert that would exceed either limit clears the cache first, and a name longer than the byte budget is parsed on every call without being cached. The cleared map is destroyed after the mutex is released. Clearing would invalidate ASTs that callers are still using, so ParseTypeName() now returns std::shared_ptr instead of a pointer into the map. This changes a function declared in an installed header; the in-tree caller compiles unchanged. --- clickhouse/types/type_parser.cpp | 29 ++++++++++------- clickhouse/types/type_parser.h | 11 +++++-- ut/type_parser_ut.cpp | 56 +++++++++++++++++++++++++++++++- 3 files changed, 81 insertions(+), 15 deletions(-) diff --git a/clickhouse/types/type_parser.cpp b/clickhouse/types/type_parser.cpp index c103ea05..406e84ff 100644 --- a/clickhouse/types/type_parser.cpp +++ b/clickhouse/types/type_parser.cpp @@ -358,25 +358,32 @@ TypeParser::Token TypeParser::NextToken() { return Token{Token::EOS, StringView()}; } - -const TypeAst* ParseTypeName(const std::string& type_name) { - // Cache for type_name. - // Usually we won't have too many type names in the cache, so do not try to - // limit cache size. - static std::map ast_cache; +std::shared_ptr ParseTypeName(const std::string& type_name) { + static std::map> ast_cache; + static size_t ast_cache_bytes = 0; static std::mutex lock; + // Declared before the guard so a cleared cache is destroyed after the mutex is released. + std::map> evicted; std::lock_guard guard(lock); auto it = ast_cache.find(type_name); if (it != ast_cache.end()) { - return &it->second; + return it->second; } - TypeAst ast; - if (!TypeParser(type_name).Parse(&ast)) { + auto ast = std::make_shared(); + if (!TypeParser(type_name).Parse(ast.get())) { return nullptr; } - return &ast_cache.emplace(type_name, std::move(ast)).first->second; -} + if (type_name.size() <= kTypeAstCacheMaxBytes) { + if (ast_cache.size() >= kTypeAstCacheMaxEntries || ast_cache_bytes + type_name.size() > kTypeAstCacheMaxBytes) { + evicted.swap(ast_cache); + ast_cache_bytes = 0; + } + ast_cache.emplace(type_name, ast); + ast_cache_bytes += type_name.size(); + } + return ast; +} } diff --git a/clickhouse/types/type_parser.h b/clickhouse/types/type_parser.h index cdef18ee..c1046281 100644 --- a/clickhouse/types/type_parser.h +++ b/clickhouse/types/type_parser.h @@ -94,7 +94,12 @@ class TypeParser { std::string scratch_; }; - -const TypeAst* ParseTypeName(const std::string& type_name); - +/// ParseTypeName() keeps parsed ASTs in a process-wide cache. When adding an +/// entry would exceed either limit, the cache is cleared first. A type name +/// longer than kTypeAstCacheMaxBytes is parsed on every call and never cached. +inline constexpr size_t kTypeAstCacheMaxEntries = 4096; +inline constexpr size_t kTypeAstCacheMaxBytes = 256 * 1024; + +/// Returns nullptr if type_name is not a valid type. +std::shared_ptr ParseTypeName(const std::string& type_name); } diff --git a/ut/type_parser_ut.cpp b/ut/type_parser_ut.cpp index fa55a456..df1406b1 100644 --- a/ut/type_parser_ut.cpp +++ b/ut/type_parser_ut.cpp @@ -558,13 +558,67 @@ TEST(ParseTypeName, MalformedNumberReturnsNull) { EXPECT_EQ(nullptr, ParseTypeName("FixedString(-)")); EXPECT_EQ(nullptr, ParseTypeName("FixedString(-)")); - const TypeAst* ast = ParseTypeName("FixedString(16)"); + const auto 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); } +namespace { + +size_t CountAlive(const std::vector>& asts) { + size_t alive = 0; + for (const auto& ast : asts) alive += !ast.expired(); + return alive; +} + +} // namespace + +TEST(ParseTypeName, CacheReusesAst) { + const auto ast = ParseTypeName("Array(Nullable(String))"); + ASSERT_NE(nullptr, ast); + EXPECT_EQ(ast, ParseTypeName("Array(Nullable(String))")); +} + +TEST(ParseTypeName, CacheEntryCountIsBounded) { + std::vector> asts; + for (size_t i = 0; i < 3 * kTypeAstCacheMaxEntries; ++i) { + asts.push_back(ParseTypeName("FixedString(" + std::to_string(i + 1) + ")")); + ASSERT_FALSE(asts.back().expired()); + } + EXPECT_LE(CountAlive(asts), kTypeAstCacheMaxEntries); + EXPECT_GT(CountAlive(asts), 0u); +} + +TEST(ParseTypeName, CacheByteBudgetIsBounded) { + const std::string label(1024, 'x'); + const std::string first_name = "Enum8('" + label + "' = 0)"; + const auto first = ParseTypeName(first_name); + ASSERT_NE(nullptr, first); + + std::vector> asts; + for (size_t i = 1; i <= 3 * kTypeAstCacheMaxBytes / label.size(); ++i) { + asts.push_back(ParseTypeName("Enum8('" + label + std::to_string(i) + "' = 0)")); + ASSERT_FALSE(asts.back().expired()); + } + EXPECT_LE(CountAlive(asts), kTypeAstCacheMaxBytes / label.size()); + + // An AST handed out before the cache was cleared stays valid. + EXPECT_EQ(first->elements[0].value_string, label); + EXPECT_NE(first, ParseTypeName(first_name)); +} + +TEST(ParseTypeName, OversizedNameIsNotCached) { + const std::string name = "Enum8('" + std::string(kTypeAstCacheMaxBytes, 'x') + "' = 0)"; + std::weak_ptr weak = ParseTypeName(name); + EXPECT_TRUE(weak.expired()); + + const auto ast = ParseTypeName(name); + ASSERT_NE(nullptr, ast); + EXPECT_EQ(ast->elements[0].value_string.size(), kTypeAstCacheMaxBytes); +} + TEST(TypeParser, AggregateFunction) { { TypeAst ast;