From 6d5f091447a17d62e73e9dd06ef79807412fd5d9 Mon Sep 17 00:00:00 2001 From: Daniel Q Date: Sun, 28 Jun 2026 08:59:16 +0200 Subject: [PATCH] Validate ClickHouseVersion format instead of lenient suffix parsing The parser introduced in #1653 stopped at the first non-numeric token and treated the rest of the string as a build suffix. However, it didn't checked the suffix text, the number of components, or empty tokens, so malformed strings like "26..1", "26.1.", "26.x.1" and "26.altinityantalya" were silently accepted. Replace/update the scan with an explicit format check based on the shapes that actually occur, such as: - 2, 3, or 4 numeric groups with no suffix, or - exactly 4 numeric groups followed by the "altinityantalya" suffix - every group must be a non-empty integer anything else is rejected. (note: Three groups is the YY.M.minor form still used upstream, so it's a normal accepted shape) Closes #1831. --- src/Common/ClickHouseVersion.cpp | 43 +++++++++++++++-------- src/Common/tests/gtest_ch_version.cpp | 50 +++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 15 deletions(-) create mode 100644 src/Common/tests/gtest_ch_version.cpp diff --git a/src/Common/ClickHouseVersion.cpp b/src/Common/ClickHouseVersion.cpp index dd1d4d0fbb3c..56ab2ce5c4c6 100644 --- a/src/Common/ClickHouseVersion.cpp +++ b/src/Common/ClickHouseVersion.cpp @@ -4,7 +4,6 @@ #include #include -#include #include @@ -20,24 +19,38 @@ ClickHouseVersion::ClickHouseVersion(std::string_view version) { Strings split; boost::split(split, version, [](char c){ return c == '.'; }); - components.reserve(split.size()); if (split.empty()) - throw Exception{ErrorCodes::BAD_ARGUMENTS, "Cannot parse ClickHouse version here: {}", version}; + throw Exception{ErrorCodes::BAD_ARGUMENTS, "Cannot parse ClickHouse version: {}", version}; + + size_t last_component; + bool last_is_numeric = !split.back().empty(); + if (last_is_numeric) + { + ReadBufferFromString buf(split.back()); + last_is_numeric = tryReadIntText(last_component, buf) && buf.eof(); + } + + if (!last_is_numeric) + { + /// A trailing non-numeric token is a build suffix (e.g. "26.1.3.20001.altinityantalya"). + /// Only that exact suffix, preceded by exactly 4 numeric components, is accepted + suffix = split.back(); + split.pop_back(); + if (suffix != "altinityantalya" || split.size() != 4) + throw Exception{ErrorCodes::BAD_ARGUMENTS, "Cannot parse ClickHouse version: {}", version}; + } + else if (split.size() < 2 || split.size() > 4) + { + throw Exception{ErrorCodes::BAD_ARGUMENTS, "Cannot parse ClickHouse version: {}", version}; + } - for (size_t i = 0; i < split.size(); ++i) + components.reserve(split.size()); + for (const auto & token : split) { size_t component; - ReadBufferFromString buf(split[i]); - if (!tryReadIntText(component, buf) || !buf.eof()) - { - /// Non-numeric component (e.g. "altinityantalya"): treat this and remaining parts as suffix. - /// Valid version must have at least one numeric component (e.g. "26.1.3.20001.altinityantalya"). - if (components.empty()) - throw Exception{ErrorCodes::BAD_ARGUMENTS, "Cannot parse ClickHouse version here: {}", version}; - Strings suffix_parts(split.begin() + i, split.end()); - suffix = boost::algorithm::join(suffix_parts, "."); - break; - } + ReadBufferFromString buf(token); + if (token.empty() || !tryReadIntText(component, buf) || !buf.eof()) + throw Exception{ErrorCodes::BAD_ARGUMENTS, "Cannot parse ClickHouse version: {}", version}; components.push_back(component); } } diff --git a/src/Common/tests/gtest_ch_version.cpp b/src/Common/tests/gtest_ch_version.cpp new file mode 100644 index 000000000000..ea6f7b66fa2c --- /dev/null +++ b/src/Common/tests/gtest_ch_version.cpp @@ -0,0 +1,50 @@ +#include +#include +#include +#include + +using namespace DB; + +TEST(ClickHouseVersion, AcceptsValidFormats) +{ + EXPECT_EQ(ClickHouseVersion("26.3").toString(), "26.3"); + EXPECT_EQ(ClickHouseVersion("25.8").toString(), "25.8"); + EXPECT_EQ(ClickHouseVersion("18.12.17").toString(), "18.12.17"); + EXPECT_EQ(ClickHouseVersion("26.3.1.20001").toString(), "26.3.1.20001"); + EXPECT_EQ(ClickHouseVersion("26.1.3.20001.altinityantalya").toString(), "26.1.3.20001.altinityantalya"); + EXPECT_EQ(ClickHouseVersion("25.8.16.20001.altinityantalya").toString(), "25.8.16.20001.altinityantalya"); +} + +TEST(ClickHouseVersion, RejectsMalformedComponents) +{ + EXPECT_THROW(ClickHouseVersion("26..1"), Exception); /// empty middle component + EXPECT_THROW(ClickHouseVersion("26.1."), Exception); /// trailing dot + EXPECT_THROW(ClickHouseVersion("26.x.1"), Exception); /// non-numeric token that is not the last token + EXPECT_THROW(ClickHouseVersion(".26"), Exception); /// leading dot + EXPECT_THROW(ClickHouseVersion(""), Exception); /// empty string + EXPECT_THROW(ClickHouseVersion(" "), Exception); /// whitespace-only token + EXPECT_THROW(ClickHouseVersion("v26.1"), Exception); /// alphabetic prefix where a number is required + EXPECT_THROW(ClickHouseVersion("."), Exception); /// lone separator, no numeric component + EXPECT_THROW(ClickHouseVersion("26.."), Exception); /// two empty trailing tokens + EXPECT_THROW(ClickHouseVersion("26.3.1.20001."), Exception); /// trailing dot after a full version +} + +TEST(ClickHouseVersion, RejectsInvalidSuffix) +{ + EXPECT_THROW(ClickHouseVersion("26.3.1.20001.altinityantalya.1"), Exception); + EXPECT_THROW(ClickHouseVersion("26.altinityantalya"), Exception); + EXPECT_THROW(ClickHouseVersion("26.1.altinityantalya"), Exception); + EXPECT_THROW(ClickHouseVersion("26.3.1.20001.altinitystable"), Exception); + EXPECT_THROW(ClickHouseVersion("altinityantalya"), Exception); +} + +TEST(ClickHouseVersion, RejectsWrongGroupCount) +{ + EXPECT_THROW(ClickHouseVersion("26"), Exception); /// 1 group + EXPECT_THROW(ClickHouseVersion("26.3.1.20001.1"), Exception); /// 5 groups, no suffix +} + +TEST(ClickHouseVersion, ComparesGroupVersion) +{ + EXPECT_TRUE(ClickHouseVersion("26.1") < ClickHouseVersion("26.1.0")); +}