From b7eebfc6260a408fb06fb65c67103ce1f4ca1d5c Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Fri, 2 Sep 2022 07:47:12 +0000 Subject: [PATCH 1/4] Correctly check if the node is using system path --- src/Common/ZooKeeper/ZooKeeperCommon.cpp | 15 +++++++++ src/Common/ZooKeeper/ZooKeeperCommon.h | 9 ++++++ src/Coordination/KeeperSnapshotManager.cpp | 37 ++++------------------ src/Coordination/KeeperStorage.cpp | 8 ++--- 4 files changed, 34 insertions(+), 35 deletions(-) diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.cpp b/src/Common/ZooKeeper/ZooKeeperCommon.cpp index b15126f5701..749052cbba3 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.cpp +++ b/src/Common/ZooKeeper/ZooKeeperCommon.cpp @@ -898,4 +898,19 @@ ZooKeeperRequestFactory::ZooKeeperRequestFactory() registerZooKeeperRequest(*this); } +PathMatchResult matchPath(const std::string_view path, const std::string_view match_to) +{ + using enum PathMatchResult; + + auto [first_it, second_it] = std::mismatch(path.begin(), path.end(), match_to.begin(), match_to.end()); + + if (second_it != match_to.end()) + return NOT_MATCH; + + if (first_it == path.end()) + return EXACT; + + return *first_it == '/' ? IS_CHILD : NOT_MATCH; +} + } diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.h b/src/Common/ZooKeeper/ZooKeeperCommon.h index 53fabf651fa..9a9700b500b 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.h +++ b/src/Common/ZooKeeper/ZooKeeperCommon.h @@ -554,4 +554,13 @@ private: ZooKeeperRequestFactory(); }; +enum class PathMatchResult +{ + NOT_MATCH, + EXACT, + IS_CHILD +}; + +PathMatchResult matchPath(std::string_view path, std::string_view match_to); + } diff --git a/src/Coordination/KeeperSnapshotManager.cpp b/src/Coordination/KeeperSnapshotManager.cpp index 1e3f37b617f..fe4050eb685 100644 --- a/src/Coordination/KeeperSnapshotManager.cpp +++ b/src/Coordination/KeeperSnapshotManager.cpp @@ -13,8 +13,10 @@ #include #include #include -#include "Coordination/KeeperContext.h" +#include #include +#include + namespace DB { @@ -146,33 +148,6 @@ namespace } } -namespace -{ - -enum class PathMatchResult -{ - NOT_MATCH, - EXACT, - IS_CHILD -}; - -PathMatchResult matchPath(const std::string_view path, const std::string_view match_to) -{ - using enum PathMatchResult; - - auto [first_it, second_it] = std::mismatch(path.begin(), path.end(), match_to.begin(), match_to.end()); - - if (second_it != match_to.end()) - return NOT_MATCH; - - if (first_it == path.end()) - return EXACT; - - return *first_it == '/' ? IS_CHILD : NOT_MATCH; -} - -} - void KeeperStorageSnapshot::serialize(const KeeperStorageSnapshot & snapshot, WriteBuffer & out, KeeperContextPtr keeper_context) { writeBinary(static_cast(snapshot.version), out); @@ -217,7 +192,7 @@ void KeeperStorageSnapshot::serialize(const KeeperStorageSnapshot & snapshot, Wr const auto & path = it->key; // write only the root system path because of digest - if (matchPath(path.toView(), keeper_system_path) == PathMatchResult::IS_CHILD) + if (Coordination::matchPath(path.toView(), keeper_system_path) == Coordination::PathMatchResult::IS_CHILD) { ++it; continue; @@ -365,8 +340,8 @@ void KeeperStorageSnapshot::deserialize(SnapshotDeserializationResult & deserial KeeperStorage::Node node{}; readNode(node, in, current_version, storage.acl_map); - using enum PathMatchResult; - auto match_result = matchPath(path, keeper_system_path); + using enum Coordination::PathMatchResult; + auto match_result = Coordination::matchPath(path, keeper_system_path); const std::string error_msg = fmt::format("Cannot read node on path {} from a snapshot because it is used as a system node", path); if (match_result == IS_CHILD) diff --git a/src/Coordination/KeeperStorage.cpp b/src/Coordination/KeeperStorage.cpp index 397cd2c0c71..1c8bc454c8b 100644 --- a/src/Coordination/KeeperStorage.cpp +++ b/src/Coordination/KeeperStorage.cpp @@ -879,7 +879,7 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr path_created += seq_num_str.str(); } - if (path_created.starts_with(keeper_system_path)) + if (Coordination::matchPath(path_created, keeper_system_path) != Coordination::PathMatchResult::NOT_MATCH) { auto error_msg = fmt::format("Trying to create a node inside the internal Keeper path ({}) which is not allowed. Path: {}", keeper_system_path, path_created); @@ -1049,7 +1049,7 @@ struct KeeperStorageRemoveRequestProcessor final : public KeeperStorageRequestPr std::vector new_deltas; - if (request.path.starts_with(keeper_system_path)) + if (Coordination::matchPath(request.path, keeper_system_path) != Coordination::PathMatchResult::NOT_MATCH) { auto error_msg = fmt::format("Trying to delete an internal Keeper path ({}) which is not allowed", request.path); @@ -1203,7 +1203,7 @@ struct KeeperStorageSetRequestProcessor final : public KeeperStorageRequestProce std::vector new_deltas; - if (request.path.starts_with(keeper_system_path)) + if (Coordination::matchPath(request.path, keeper_system_path) != Coordination::PathMatchResult::NOT_MATCH) { auto error_msg = fmt::format("Trying to update an internal Keeper path ({}) which is not allowed", request.path); @@ -1472,7 +1472,7 @@ struct KeeperStorageSetACLRequestProcessor final : public KeeperStorageRequestPr { Coordination::ZooKeeperSetACLRequest & request = dynamic_cast(*zk_request); - if (request.path.starts_with(keeper_system_path)) + if (Coordination::matchPath(request.path, keeper_system_path) != Coordination::PathMatchResult::NOT_MATCH) { auto error_msg = fmt::format("Trying to update an internal Keeper path ({}) which is not allowed", request.path); From ad2196155c9dd21958dac0cc355c96be494bef2e Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Fri, 2 Sep 2022 08:14:06 +0000 Subject: [PATCH 2/4] Add test for system node modification --- src/Coordination/tests/gtest_coordination.cpp | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/Coordination/tests/gtest_coordination.cpp b/src/Coordination/tests/gtest_coordination.cpp index 493e76ee5fc..cdda6fc1f32 100644 --- a/src/Coordination/tests/gtest_coordination.cpp +++ b/src/Coordination/tests/gtest_coordination.cpp @@ -2141,6 +2141,38 @@ TEST_P(CoordinationTest, TestCurrentApiVersion) EXPECT_EQ(keeper_version, static_cast(current_keeper_api_version)); } +TEST_P(CoordinationTest, TestSystemNodeModify) +{ + using namespace Coordination; + int64_t zxid{0}; + + // On INIT we abort when a system path is modified + keeper_context->server_state = KeeperContext::Phase::RUNNING; + KeeperStorage storage{500, "", keeper_context}; + const auto assert_create = [&](const std::string_view path, const auto expected_code) + { + auto request = std::make_shared(); + request->path = path; + storage.preprocessRequest(request, 0, 0, zxid); + auto responses = storage.processRequest(request, 0, zxid); + ASSERT_FALSE(responses.empty()); + + const auto & response = responses[0]; + ASSERT_EQ(response.response->error, expected_code) << "Unexpected error for path " << path; + + ++zxid; + }; + + assert_create("/keeper", Error::ZBADARGUMENTS); + assert_create("/keeper/with_child", Error::ZBADARGUMENTS); + assert_create(DB::keeper_api_version_path, Error::ZBADARGUMENTS); + + assert_create("/keeper_map", Error::ZOK); + assert_create("/keeper1", Error::ZOK); + assert_create("/keepe", Error::ZOK); + assert_create("/keeper1/test", Error::ZOK); +} + INSTANTIATE_TEST_SUITE_P(CoordinationTestSuite, CoordinationTest, ::testing::ValuesIn(std::initializer_list{ From 09c0bf29310ec71626d039173d75467d28a77c7a Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Thu, 8 Sep 2022 08:16:38 +0000 Subject: [PATCH 3/4] Add unit tests for match path --- src/Common/ZooKeeper/ZooKeeperCommon.cpp | 14 +++++++++++++- src/Common/ZooKeeper/tests/gtest_zookeeper.cpp | 15 +++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 src/Common/ZooKeeper/tests/gtest_zookeeper.cpp diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.cpp b/src/Common/ZooKeeper/ZooKeeperCommon.cpp index 749052cbba3..3aba0a0651d 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.cpp +++ b/src/Common/ZooKeeper/ZooKeeperCommon.cpp @@ -898,10 +898,22 @@ ZooKeeperRequestFactory::ZooKeeperRequestFactory() registerZooKeeperRequest(*this); } -PathMatchResult matchPath(const std::string_view path, const std::string_view match_to) +PathMatchResult matchPath(std::string_view path, std::string_view match_to) { using enum PathMatchResult; + if (match_to == "/") + return path == "/" ? EXACT : IS_CHILD; + + const auto clean_path = [](auto & p) + { + if (p.ends_with('/')) + p.remove_suffix(1); + }; + + clean_path(path); + clean_path(match_to); + auto [first_it, second_it] = std::mismatch(path.begin(), path.end(), match_to.begin(), match_to.end()); if (second_it != match_to.end()) diff --git a/src/Common/ZooKeeper/tests/gtest_zookeeper.cpp b/src/Common/ZooKeeper/tests/gtest_zookeeper.cpp new file mode 100644 index 00000000000..5a989e5932f --- /dev/null +++ b/src/Common/ZooKeeper/tests/gtest_zookeeper.cpp @@ -0,0 +1,15 @@ +#include + +#include + +TEST(ZooKeeperTest, TestMatchPath) +{ + using namespace Coordination; + + ASSERT_EQ(matchPath("/path/file", "/path"), PathMatchResult::IS_CHILD); + ASSERT_EQ(matchPath("/path/file", "/path/"), PathMatchResult::IS_CHILD); + ASSERT_EQ(matchPath("/path/file", "/"), PathMatchResult::IS_CHILD); + ASSERT_EQ(matchPath("/", "/"), PathMatchResult::EXACT); + ASSERT_EQ(matchPath("/path", "/path/"), PathMatchResult::EXACT); + ASSERT_EQ(matchPath("/path/", "/path"), PathMatchResult::EXACT); +} From 3502718a2c7e842f78490f08c8b0b021f549072f Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Thu, 8 Sep 2022 13:52:14 +0000 Subject: [PATCH 4/4] Less complications --- src/Common/ZooKeeper/ZooKeeperCommon.cpp | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.cpp b/src/Common/ZooKeeper/ZooKeeperCommon.cpp index 3aba0a0651d..4ab93d814df 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.cpp +++ b/src/Common/ZooKeeper/ZooKeeperCommon.cpp @@ -902,17 +902,11 @@ PathMatchResult matchPath(std::string_view path, std::string_view match_to) { using enum PathMatchResult; - if (match_to == "/") - return path == "/" ? EXACT : IS_CHILD; + if (path.ends_with('/')) + path.remove_suffix(1); - const auto clean_path = [](auto & p) - { - if (p.ends_with('/')) - p.remove_suffix(1); - }; - - clean_path(path); - clean_path(match_to); + if (match_to.ends_with('/')) + match_to.remove_suffix(1); auto [first_it, second_it] = std::mismatch(path.begin(), path.end(), match_to.begin(), match_to.end());