This is an automated email from the ASF dual-hosted git repository.
git-hulk pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new 3e564ca4d fix(replication): reject unsafe fullsync file names (#3483)
3e564ca4d is described below
commit 3e564ca4d0374130ad00a9c9ec03989c2ef44cf0
Author: hulk <[email protected]>
AuthorDate: Mon May 11 18:44:36 2026 +0800
fix(replication): reject unsafe fullsync file names (#3483)
Replication fullsync uses the peer's file names to fetch checkpoint
files
and materialize them in the local sync checkpoint directory. Previously,
those names were joined directly with local directories,
so traversal components could escape the intended directory during
existence checks,
temporary file creation, rename, cleanup, or master-side file open.
Assisted-by: Codex/GPT 5.5 xhigh
---
src/cluster/replication.cc | 4 ++++
src/storage/storage.cc | 47 +++++++++++++++++++++++++++++++++++++++++++
src/storage/storage.h | 2 ++
tests/cppunit/storage_test.cc | 41 +++++++++++++++++++++++++++++++++++++
4 files changed, 94 insertions(+)
diff --git a/src/cluster/replication.cc b/src/cluster/replication.cc
index f91a6a36e..ca7c414bd 100644
--- a/src/cluster/replication.cc
+++ b/src/cluster/replication.cc
@@ -847,6 +847,10 @@ ReplicationThread::CBState
ReplicationThread::fullSyncReadCB(bufferevent *bev) {
}
std::vector<std::string> need_files =
util::Split(std::string(line.get()), ",");
for (const auto &f : need_files) {
+ if (auto s =
engine::Storage::ReplDataManager::ValidateReplFileName(f); !s.IsOK()) {
+ WARN("[replication] Ignored the invalid fullsync file name '{}':
{}", f, s.Msg());
+ continue;
+ }
meta.files.emplace_back(f, 0);
}
diff --git a/src/storage/storage.cc b/src/storage/storage.cc
index 288524eb0..74a9bca3a 100644
--- a/src/storage/storage.cc
+++ b/src/storage/storage.cc
@@ -34,6 +34,7 @@
#include <iostream>
#include <memory>
#include <random>
+#include <string_view>
#include "compact_filter.h"
#include "db_util.h"
@@ -1226,7 +1227,40 @@ Status
Storage::ReplDataManager::CleanInvalidFiles(Storage *storage, const std::
return ret;
}
+Status Storage::ReplDataManager::ValidateReplFileName(const std::string
&repl_file) {
+ if (repl_file.empty()) {
+ return {Status::NotOK, "empty replication file name"};
+ }
+ if (repl_file.front() == '/') {
+ return {Status::NotOK, fmt::format("absolute replication file name '{}' is
not allowed", repl_file)};
+ }
+ if (repl_file.back() == '/') {
+ return {Status::NotOK, fmt::format("unsafe replication file name '{}' is
not allowed", repl_file)};
+ }
+
+ for (size_t begin = 0; begin < repl_file.size();) {
+ auto end = repl_file.find('/', begin);
+ auto component = std::string_view(repl_file).substr(begin, end - begin);
+ if (component.empty() || component == "." || component == "..") {
+ return {Status::NotOK, fmt::format("unsafe replication file name '{}' is
not allowed", repl_file)};
+ }
+ if (component.find('\0') != std::string_view::npos || component.find('\\')
!= std::string_view::npos ||
+ component.find(':') != std::string_view::npos) {
+ return {Status::NotOK, fmt::format("unsafe replication file name '{}' is
not allowed", repl_file)};
+ }
+ if (end == std::string::npos) break;
+ begin = end + 1;
+ }
+
+ return Status::OK();
+}
+
int Storage::ReplDataManager::OpenDataFile(Storage *storage, const std::string
&repl_file, uint64_t *file_size) {
+ if (auto s = ValidateReplFileName(repl_file); !s.IsOK()) {
+ ERROR("[storage] Invalid replication data file '{}': {}", repl_file,
s.Msg());
+ return NullFD;
+ }
+
std::string abs_path = storage->config_->checkpoint_dir + "/" + repl_file;
auto s = storage->env_->FileExists(abs_path);
if (!s.ok()) {
@@ -1282,6 +1316,9 @@ Status Storage::ReplDataManager::ParseMetaAndSave(Storage
*storage, rocksdb::Bac
}
auto filename = std::string(line.get(), cptr - line.get() - 1);
+ if (auto s = ValidateReplFileName(filename); !s.IsOK()) {
+ return s;
+ }
while (*(cptr++) != ' ') {
}
@@ -1311,6 +1348,11 @@ Status MkdirRecursively(rocksdb::Env *env, const
std::string &dir) {
std::unique_ptr<rocksdb::WritableFile>
Storage::ReplDataManager::NewTmpFile(Storage *storage, const std::string &dir,
const std::string &repl_file) {
+ if (auto s = ValidateReplFileName(repl_file); !s.IsOK()) {
+ ERROR("[storage] Invalid replication data file '{}': {}", repl_file,
s.Msg());
+ return nullptr;
+ }
+
std::string tmp_file = dir + "/" + repl_file + ".tmp";
auto s = storage->env_->FileExists(tmp_file);
if (s.ok()) {
@@ -1335,6 +1377,10 @@ std::unique_ptr<rocksdb::WritableFile>
Storage::ReplDataManager::NewTmpFile(Stor
}
Status Storage::ReplDataManager::SwapTmpFile(Storage *storage, const
std::string &dir, const std::string &repl_file) {
+ if (auto s = ValidateReplFileName(repl_file); !s.IsOK()) {
+ return s;
+ }
+
std::string tmp_file = dir + "/" + repl_file + ".tmp";
std::string orig_file = dir + "/" + repl_file;
@@ -1349,6 +1395,7 @@ Status Storage::ReplDataManager::SwapTmpFile(Storage
*storage, const std::string
bool Storage::ReplDataManager::FileExists(Storage *storage, const std::string
&dir, const std::string &repl_file,
uint32_t crc) {
if (storage->IsClosing()) return false;
+ if (!ValidateReplFileName(repl_file).IsOK()) return false;
auto file_path = dir + "/" + repl_file;
auto s = storage->env_->FileExists(file_path);
diff --git a/src/storage/storage.h b/src/storage/storage.h
index 69afeff28..e199052b6 100644
--- a/src/storage/storage.h
+++ b/src/storage/storage.h
@@ -320,6 +320,8 @@ class Storage {
// Full replication data files manager
class ReplDataManager {
public:
+ static Status ValidateReplFileName(const std::string &repl_file);
+
// Master side
static Status GetFullReplDataInfo(Storage *storage, std::string *files);
static int OpenDataFile(Storage *storage, const std::string &rel_file,
uint64_t *file_size);
diff --git a/tests/cppunit/storage_test.cc b/tests/cppunit/storage_test.cc
index 3b403e376..e6404e5d2 100644
--- a/tests/cppunit/storage_test.cc
+++ b/tests/cppunit/storage_test.cc
@@ -26,6 +26,8 @@
#include <filesystem>
#include <fstream>
+#include <string>
+#include <vector>
TEST(Storage, CreateBackup) {
std::error_code ec;
@@ -129,3 +131,42 @@ TEST(Storage, RocksDBDictionaryCompressionOptions) {
unlink(path);
}
+
+TEST(Storage, ReplDataManagerRejectsUnsafeFilenames) {
+ Config config;
+ config.db_dir = "test_repl_file_validation_dir/db";
+ config.checkpoint_dir = "test_repl_file_validation_dir/checkpoint";
+ config.slot_id_encoded = false;
+
+ std::error_code ec;
+ std::filesystem::remove_all("test_repl_file_validation_dir", ec);
+ ASSERT_FALSE(ec);
+
+ engine::Storage storage(&config);
+ auto base_dir = std::string("test_repl_file_validation_dir/sync_checkpoint");
+
+ auto valid_file = engine::Storage::ReplDataManager::NewTmpFile(&storage,
base_dir, "meta/1");
+ ASSERT_TRUE(valid_file);
+ ASSERT_TRUE(valid_file->Close().ok());
+ ASSERT_TRUE(engine::Storage::ReplDataManager::SwapTmpFile(&storage,
base_dir, "meta/1").IsOK());
+
EXPECT_TRUE(std::filesystem::exists("test_repl_file_validation_dir/sync_checkpoint/meta/1"));
+
+ const std::vector<std::string> unsafe_files = {
+ "../escape", "a/../../escape", "/tmp/escape", "a//b", "a/",
+ ".", "a/..", "a\\b", "C:/escape",
std::string("bad\0name", 8),
+ };
+ for (const auto &file : unsafe_files) {
+
EXPECT_FALSE(engine::Storage::ReplDataManager::ValidateReplFileName(file).IsOK())
<< file;
+ EXPECT_FALSE(engine::Storage::ReplDataManager::NewTmpFile(&storage,
base_dir, file)) << file;
+ EXPECT_FALSE(engine::Storage::ReplDataManager::SwapTmpFile(&storage,
base_dir, file).IsOK()) << file;
+ EXPECT_FALSE(engine::Storage::ReplDataManager::FileExists(&storage,
base_dir, file, 0)) << file;
+ uint64_t file_size = 0;
+ EXPECT_LT(engine::Storage::ReplDataManager::OpenDataFile(&storage, file,
&file_size), 0) << file;
+ }
+
+
EXPECT_FALSE(std::filesystem::exists("test_repl_file_validation_dir/escape.tmp"));
+
EXPECT_FALSE(std::filesystem::exists("test_repl_file_validation_dir/escape"));
+
+ std::filesystem::remove_all("test_repl_file_validation_dir", ec);
+ ASSERT_FALSE(ec);
+}
