From 1c56836eb4f47f44daa81de411a47c9725949510 Mon Sep 17 00:00:00 2001
From: Nobuhiro Ban <ban@nautilus-technologies.com>
Date: Mon, 30 Sep 2024 00:30:02 +0900
Subject: [PATCH] tglogutil-compaction: reject symlink target

---
 src/limestone/dblogutil/dblogutil.cpp         |  8 +++++++
 src/limestone/filepath.cpp                    | 17 ++++++++-----
 src/limestone/internal.h                      |  1 +
 .../utils/dblogutil_compaction_test.cpp       | 24 +++++++++++++++++++
 test/limestone/utils/make_tmp_test.cpp        | 17 +++++++++++++
 5 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/src/limestone/dblogutil/dblogutil.cpp b/src/limestone/dblogutil/dblogutil.cpp
index a0d37e6e..4d56b2c9 100644
--- a/src/limestone/dblogutil/dblogutil.cpp
+++ b/src/limestone/dblogutil/dblogutil.cpp
@@ -187,6 +187,14 @@ void compaction(dblog_scan &ds, std::optional<epoch_id_type> epoch) {
         std::cout << "durable-epoch: " << ld_epoch << std::endl;
     }
     auto from_dir = ds.get_dblogdir();
+    {
+        auto p = from_dir;  // make copy
+        remove_trailing_dir_separators(p);
+        if (boost::filesystem::is_symlink(p)) {
+            LOG(ERROR) << "dblogdir is symlink; compaction target must not be symlink";
+            log_and_exit(64);
+        }
+    }
     boost::filesystem::path tmp;
     if (!FLAGS_working_dir.empty()) {
         tmp = FLAGS_working_dir;
diff --git a/src/limestone/filepath.cpp b/src/limestone/filepath.cpp
index 05de36cd..7896a9d2 100644
--- a/src/limestone/filepath.cpp
+++ b/src/limestone/filepath.cpp
@@ -22,16 +22,21 @@
 
 namespace limestone::internal {
 
+void remove_trailing_dir_separators(boost::filesystem::path& p) {
+    std::string str = p.string();
+    std::size_t prev_len{};
+    do {
+        prev_len = str.size();
+        p.remove_trailing_separator();  // remove only one char
+        str = p.string();
+    } while (str.size() < prev_len);
+}
+
 boost::filesystem::path make_tmp_dir_next_to(const boost::filesystem::path& target_dir, const char* suffix) {
     auto canonicalpath = boost::filesystem::canonical(target_dir);
     // some versions of boost::filesystem::canonical do not remove trailing directory-separators ('/')
+    remove_trailing_dir_separators(canonicalpath);
     std::string targetdirstring = canonicalpath.string();
-    std::size_t prev_len{};
-    do {
-        prev_len = targetdirstring.size();
-        canonicalpath.remove_trailing_separator();  // remove only one char
-        targetdirstring = canonicalpath.string();
-    } while (targetdirstring.size() < prev_len);
 
     auto tmpdirname = targetdirstring + suffix;
     if (::mkdtemp(tmpdirname.data()) == nullptr) {
diff --git a/src/limestone/internal.h b/src/limestone/internal.h
index 1aea9c6d..3bd2ab9a 100644
--- a/src/limestone/internal.h
+++ b/src/limestone/internal.h
@@ -70,6 +70,7 @@ void create_compact_pwal(
 
 // filepath.cpp
 
+void remove_trailing_dir_separators(boost::filesystem::path& p);
 boost::filesystem::path make_tmp_dir_next_to(const boost::filesystem::path& target_dir, const char* suffix);
 
 }
diff --git a/test/limestone/utils/dblogutil_compaction_test.cpp b/test/limestone/utils/dblogutil_compaction_test.cpp
index dbde5593..0b472f27 100644
--- a/test/limestone/utils/dblogutil_compaction_test.cpp
+++ b/test/limestone/utils/dblogutil_compaction_test.cpp
@@ -171,4 +171,28 @@ TEST_F(dblogutil_compaction_test, nondblogdir) {
     EXPECT_TRUE(contains(out, "unsupport"));
 }
 
+TEST_F(dblogutil_compaction_test, rejects_symlink) {
+    // setup normal logdir as same as case1
+    boost::filesystem::path dir{location};
+    dir /= "log";
+    boost::filesystem::create_directory(dir);
+    create_file(dir / "epoch", data_case1_epoch);
+    create_file(dir / std::string(manifest_file_name), data_manifest());
+    create_file(dir / "pwal_0000", data_case1_pwal0);
+    create_file(dir / "pwal_0001", data_case1_pwal1);
+
+    // make symlink to data
+    boost::filesystem::path symlink{location};
+    symlink /= "symbolic_link";
+    boost::filesystem::create_directory_symlink(dir, symlink);
+
+    std::string command;
+    command = UTIL_COMMAND " compaction --force " + symlink.string() + " 2>&1";
+    std::string out;
+    int rc = invoke(command, out);
+    EXPECT_GE(rc, 64 << 8);
+    EXPECT_TRUE(contains_line_starts_with(out, "E"));  // LOG(ERROR)
+    EXPECT_TRUE(contains(out, "must not be symlink"));
+}
+
 }  // namespace limestone::testing
diff --git a/test/limestone/utils/make_tmp_test.cpp b/test/limestone/utils/make_tmp_test.cpp
index 2cd008c8..39f115aa 100644
--- a/test/limestone/utils/make_tmp_test.cpp
+++ b/test/limestone/utils/make_tmp_test.cpp
@@ -32,6 +32,23 @@ static constexpr const char* location = "/tmp/make_tmp_test";
 
 };
 
+TEST_F(make_tmp_test, remove_trailing_dir_separators) {
+    boost::filesystem::path p0{"/tmp/nonexist/0"};
+    remove_trailing_dir_separators(p0);
+    ASSERT_EQ(p0.string(), "/tmp/nonexist/0");
+    ASSERT_EQ(p0.filename().string(), "0");
+
+    boost::filesystem::path p1{"/tmp/nonexist/1/"};
+    remove_trailing_dir_separators(p1);
+    ASSERT_EQ(p1.string(), "/tmp/nonexist/1");
+    ASSERT_EQ(p1.filename().string(), "1");
+
+    boost::filesystem::path p2{"/tmp/nonexist/2//"};
+    remove_trailing_dir_separators(p2);
+    ASSERT_EQ(p2.string(), "/tmp/nonexist/2");
+    ASSERT_EQ(p2.filename().string(), "2");
+}
+
 TEST_F(make_tmp_test, make_tmp_dir_next_to_0slash) {
     std::string p = std::string(location) + "/test0";
     boost::filesystem::create_directory(p);