Reject zero, oversized, and malformed FixedString sizes

clickhouse/columns/factory.cpp

@@ -46,6 +46,16 @@ const auto& GetASTChildElement(const TypeAst & ast, int position) {
     return ast.elements[static_cast<size_t>(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_t>(size);
+}
+
 static ColumnRef CreateTerminalColumn(const TypeAst& ast) {
     switch (ast.code) {
     case Type::Void:
@@ -92,7 +102,7 @@ static ColumnRef CreateTerminalColumn(const TypeAst& ast) {
     case Type::String:
         return std::make_shared<ColumnString>();
     case Type::FixedString:
-        return std::make_shared<ColumnFixedString>(GetASTChildElement(ast, 0).value);
+        return std::make_shared<ColumnFixedString>(GetFixedStringSize(ast));
 
     case Type::DateTime:
         if (ast.elements.empty()) {
@@ -225,7 +235,7 @@ static ColumnRef CreateColumnFromAst(const TypeAst& ast, CreateColumnByTypeSetti
                     case Type::String:
                         return std::make_shared<LowCardinalitySerializationAdaptor<ColumnString>>();
                     case Type::FixedString:
-                        return std::make_shared<LowCardinalitySerializationAdaptor<ColumnFixedString>>(GetASTChildElement(nested, 0).value);
+                        return std::make_shared<LowCardinalitySerializationAdaptor<ColumnFixedString>>(GetFixedStringSize(nested));
                     case Type::Nullable:
                         throw UnimplementedError("LowCardinality(" + nested.name + ") is not supported with LowCardinalityAsWrappedColumn on");
                     default:

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) {

clickhouse/types/type_parser.cpp

@@ -16,6 +16,7 @@
 #include <strings.h>
 #endif
 
+#include <charconv>
 
 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 &ast;
+    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;
 }
 
 }

ut/CreateColumnByType_ut.cpp

@@ -64,6 +64,53 @@ 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<ColumnFixedString>()->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<ColumnFixedString>()->FixedSize());
+
+    EXPECT_THROW(CreateColumnByType("FixedString(16777216)"), ValidationError);
+}
+
+TEST(CreateColumnByType, LowCardinalityFixedStringSizeLimit) {
+    CreateColumnByTypeSettings wrapped;
+    wrapped.low_cardinality_as_wrapped_column = true;
+
+    const std::pair<CreateColumnByTypeSettings, std::string> 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) {
+        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 +310,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))",

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<ColumnFixedString>(0);
+TEST(ColumnsCase, FixedString_Size_Eq0_Throws) {
+    EXPECT_THROW(ColumnFixedString(0), ValidationError);
+}
+
+TEST(ColumnsCase, FixedString_Type_Size_Eq1) {
+    const auto col = std::make_shared<ColumnFixedString>(1);
     ASSERT_EQ(col->FixedSize(), col->Type()->As<FixedStringType>()->GetSize());
 }
 

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;

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)))",