From a184cc7e04fbab6d604e20dd4f6d2e11768d71dd Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Sun, 18 Oct 2026 11:50:46 -0400 Subject: [PATCH 1/2] Reject unbalanced and overly deep type names TypeParser::Parse() calls top() and pop() on its element stack for every ')' and ',', so a type name with a surplus closing parenthesis such as "UInt8))" or ")," reads from an empty std::stack and crashes. It also has no nesting limit, while CreateColumnFromAst() and the TypeAst destructor recurse once per level, so a type nested 100000 levels deep, closed or not, overflows the stack. Parse() now fails on a ')' or ',' with no open parenthesis and on nesting deeper than 512 levels: current servers reject column types nested deeper than 499 levels, and 512 keeps the recursive paths to about 250 KiB of stack in an optimized build. --- clickhouse/types/type_parser.cpp | 3 ++ clickhouse/types/type_parser.h | 5 +++ ut/type_parser_ut.cpp | 73 ++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/clickhouse/types/type_parser.cpp b/clickhouse/types/type_parser.cpp index d305a398..a0caeeb1 100644 --- a/clickhouse/types/type_parser.cpp +++ b/clickhouse/types/type_parser.cpp @@ -209,16 +209,19 @@ bool TypeParser::Parse(TypeAst* type) { type_->value_string = std::string(token.value); break; case Token::LPar: + if (open_elements_.size() > kMaxNestingDepth) return false; type_->elements.emplace_back(TypeAst()); open_elements_.push(type_); type_ = &type_->elements.back(); break; case Token::RPar: + if (open_elements_.size() < 2) return false; type_ = open_elements_.top(); open_elements_.pop(); break; case Token::Assign: case Token::Comma: + if (open_elements_.size() < 2) return false; type_ = open_elements_.top(); open_elements_.pop(); type_->elements.emplace_back(TypeAst()); diff --git a/clickhouse/types/type_parser.h b/clickhouse/types/type_parser.h index cdef18ee..a37f5070 100644 --- a/clickhouse/types/type_parser.h +++ b/clickhouse/types/type_parser.h @@ -71,6 +71,11 @@ class TypeParser { }; public: + // Current servers reject column types nested deeper than 499 levels. The + // column factory and TypeAst destruction recurse once per level, so the + // limit also bounds their stack use. + static constexpr size_t kMaxNestingDepth = 512; + explicit TypeParser(const StringView& name); ~TypeParser(); diff --git a/ut/type_parser_ut.cpp b/ut/type_parser_ut.cpp index 76bedd7a..584c3da3 100644 --- a/ut/type_parser_ut.cpp +++ b/ut/type_parser_ut.cpp @@ -1,3 +1,4 @@ +#include #include #include @@ -552,3 +553,75 @@ TEST(TypeParser, AggregateFunction) { EXPECT_FALSE(TypeParser("AggregateFunction(argMax, LowCardinality(Nullable(FixedString(4))), DateTime(3, 'UTC'))").Parse(&ast)); } } + +TEST(TypeParser, UnbalancedClosingParenthesis) { + for (const std::string type_name : {")", "),", ")))", "UInt8)", "UInt8))", "UInt8),", "UInt8)(", "Array(UInt8))", "Array(UInt8)),", + "Tuple(UInt8, String))", "UInt8,", "UInt8, String", ","}) { + TypeAst ast; + EXPECT_FALSE(TypeParser(type_name).Parse(&ast)) << type_name; + EXPECT_EQ(nullptr, ParseTypeName(type_name)) << type_name; + } +} + +namespace { +std::string NestedArrayType(size_t depth, bool closed = true) { + std::string result; + for (size_t i = 0; i < depth; ++i) { + result += "Array("; + } + result += "UInt8"; + if (closed) { + result.append(depth, ')'); + } + return result; +} +} // namespace + +TEST(TypeParser, NestingDepthLimit) { + { + TypeAst ast; + ASSERT_TRUE(TypeParser(NestedArrayType(TypeParser::kMaxNestingDepth)).Parse(&ast)); + + size_t depth = 0; + const TypeAst* node = * + while (node->meta == TypeAst::Array) { + ASSERT_EQ(1u, node->elements.size()); + node = &node->elements.front(); + ++depth; + } + EXPECT_EQ(TypeParser::kMaxNestingDepth, depth); + EXPECT_EQ(Type::UInt8, node->code); + } + + for (size_t depth : {TypeParser::kMaxNestingDepth + 1, size_t{100000}}) { + TypeAst closed_ast; + EXPECT_FALSE(TypeParser(NestedArrayType(depth)).Parse(&closed_ast)) << depth; + EXPECT_EQ(nullptr, ParseTypeName(NestedArrayType(depth))) << depth; + + TypeAst open_ast; + EXPECT_FALSE(TypeParser(NestedArrayType(depth, false)).Parse(&open_ast)) << depth; + EXPECT_EQ(nullptr, ParseTypeName(NestedArrayType(depth, false))) << depth; + } +} + +TEST(TypeParser, NestingDepthLimitBoundsColumnFactory) { + auto col = CreateColumnByType(NestedArrayType(32)); + ASSERT_NE(nullptr, col); + EXPECT_EQ(NestedArrayType(32), col->Type()->GetName()); + + EXPECT_NE(nullptr, CreateColumnByType(NestedArrayType(499))); + EXPECT_NE(nullptr, CreateColumnByType(NestedArrayType(TypeParser::kMaxNestingDepth))); + EXPECT_EQ(nullptr, CreateColumnByType(NestedArrayType(TypeParser::kMaxNestingDepth + 1))); + EXPECT_EQ(nullptr, CreateColumnByType(NestedArrayType(100000))); + EXPECT_EQ(nullptr, CreateColumnByType(NestedArrayType(100000, false))); +} + +TEST(TypeParser, BalancedNestedControls) { + for (const std::string type_name : {"Map(String, Array(Nullable(UInt8)))", "Tuple(a UInt8, b String)", + "Array(Array(Array(Array(Array(Array(Array(Array(UInt8))))))))"}) { + TypeAst ast; + EXPECT_TRUE(TypeParser(type_name).Parse(&ast)) << type_name; + EXPECT_NE(nullptr, ParseTypeName(type_name)) << type_name; + EXPECT_NE(nullptr, CreateColumnByType(type_name)) << type_name; + } +} From 22426bd6ba25c2f52deb73f244d081dba934a04a Mon Sep 17 00:00:00 2001 From: Andrew Slabko Date: Wed, 28 Oct 2026 17:46:24 +0100 Subject: [PATCH 2/2] Keep the nesting depth limit out of the installed API TypeParser::kMaxNestingDepth was declared public in types/type_parser.h, which CMake installs, so the limit became part of the shipped API even though the only production reference is in TypeParser::Parse. Move it to a file-local constant in type_parser.cpp, matching the other internal constants there (kTypeCode, kUnescapeMap), so the value stays tunable without a breaking change. The tests referenced the constant to derive their boundaries, which made them tautological: asserting that depth N parses and N+1 does not holds for any limit, so lowering it to a value too small for the types real servers send would not have failed TypeParser.NestingDepthLimit. Pin 512 and 513 by value instead. --- clickhouse/types/type_parser.cpp | 6 ++++++ clickhouse/types/type_parser.h | 5 ----- ut/type_parser_ut.cpp | 14 +++++++++----- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/clickhouse/types/type_parser.cpp b/clickhouse/types/type_parser.cpp index a0caeeb1..51c3b30f 100644 --- a/clickhouse/types/type_parser.cpp +++ b/clickhouse/types/type_parser.cpp @@ -29,6 +29,12 @@ bool TypeAst::operator==(const TypeAst & other) const { && std::equal(elements.begin(), elements.end(), other.elements.begin(), other.elements.end()); } +// Current servers reject column types nested deeper than 499 levels. The column +// factory and TypeAst destruction recurse once per level, so the limit also +// bounds their stack use. Kept internal: this is a hardening detail, not part of +// the installed API. +static constexpr size_t kMaxNestingDepth = 512; + static const std::unordered_map kTypeCode = { { "Void", Type::Void }, { "Int8", Type::Int8 }, diff --git a/clickhouse/types/type_parser.h b/clickhouse/types/type_parser.h index a37f5070..cdef18ee 100644 --- a/clickhouse/types/type_parser.h +++ b/clickhouse/types/type_parser.h @@ -71,11 +71,6 @@ class TypeParser { }; public: - // Current servers reject column types nested deeper than 499 levels. The - // column factory and TypeAst destruction recurse once per level, so the - // limit also bounds their stack use. - static constexpr size_t kMaxNestingDepth = 512; - explicit TypeParser(const StringView& name); ~TypeParser(); diff --git a/ut/type_parser_ut.cpp b/ut/type_parser_ut.cpp index 584c3da3..c38e6e1c 100644 --- a/ut/type_parser_ut.cpp +++ b/ut/type_parser_ut.cpp @@ -577,10 +577,14 @@ std::string NestedArrayType(size_t depth, bool closed = true) { } } // namespace +// The depth limit is an internal detail of type_parser.cpp, so these tests pin +// it by value rather than by referring to the constant. Asserting against the +// constant would hold for any limit, including one too low to parse the types +// real servers send; changing the limit must fail here deliberately. TEST(TypeParser, NestingDepthLimit) { { TypeAst ast; - ASSERT_TRUE(TypeParser(NestedArrayType(TypeParser::kMaxNestingDepth)).Parse(&ast)); + ASSERT_TRUE(TypeParser(NestedArrayType(512)).Parse(&ast)); size_t depth = 0; const TypeAst* node = * @@ -589,11 +593,11 @@ TEST(TypeParser, NestingDepthLimit) { node = &node->elements.front(); ++depth; } - EXPECT_EQ(TypeParser::kMaxNestingDepth, depth); + EXPECT_EQ(512u, depth); EXPECT_EQ(Type::UInt8, node->code); } - for (size_t depth : {TypeParser::kMaxNestingDepth + 1, size_t{100000}}) { + for (size_t depth : {size_t{513}, size_t{100000}}) { TypeAst closed_ast; EXPECT_FALSE(TypeParser(NestedArrayType(depth)).Parse(&closed_ast)) << depth; EXPECT_EQ(nullptr, ParseTypeName(NestedArrayType(depth))) << depth; @@ -610,8 +614,8 @@ TEST(TypeParser, NestingDepthLimitBoundsColumnFactory) { EXPECT_EQ(NestedArrayType(32), col->Type()->GetName()); EXPECT_NE(nullptr, CreateColumnByType(NestedArrayType(499))); - EXPECT_NE(nullptr, CreateColumnByType(NestedArrayType(TypeParser::kMaxNestingDepth))); - EXPECT_EQ(nullptr, CreateColumnByType(NestedArrayType(TypeParser::kMaxNestingDepth + 1))); + EXPECT_NE(nullptr, CreateColumnByType(NestedArrayType(512))); + EXPECT_EQ(nullptr, CreateColumnByType(NestedArrayType(513))); EXPECT_EQ(nullptr, CreateColumnByType(NestedArrayType(100000))); EXPECT_EQ(nullptr, CreateColumnByType(NestedArrayType(100000, false))); }