From ae2af813df695bb553b9869593f6866e2541b15d Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Mon, 31 Aug 2026 15:25:46 +0800 Subject: [PATCH 1/7] fix: validate filesystem database/table/branch names --- include/paimon/catalog/catalog.h | 3 +- src/paimon/core/catalog/catalog_utils.cpp | 51 +++++++ src/paimon/core/catalog/catalog_utils.h | 12 ++ .../core/catalog/file_system_catalog.cpp | 24 ++-- src/paimon/core/catalog/file_system_catalog.h | 7 +- .../core/catalog/file_system_catalog_test.cpp | 136 ++++++++++++++++++ 6 files changed, 222 insertions(+), 11 deletions(-) diff --git a/include/paimon/catalog/catalog.h b/include/paimon/catalog/catalog.h index 9213bde94..0dc575f6d 100644 --- a/include/paimon/catalog/catalog.h +++ b/include/paimon/catalog/catalog.h @@ -165,7 +165,8 @@ class PAIMON_EXPORT Catalog { /// @note This does not check whether the database actually exists. /// /// @param db_name The name of the database to get the location for. - /// @return A string representing the expected location of the database. + /// @return A string representing the expected location of the database, or an empty string + /// when the name does not form a valid location. virtual std::string GetDatabaseLocation(const std::string& db_name) const = 0; /// Returns the expected location of a specified table. diff --git a/src/paimon/core/catalog/catalog_utils.cpp b/src/paimon/core/catalog/catalog_utils.cpp index eae23ad8e..e06c7c4fd 100644 --- a/src/paimon/core/catalog/catalog_utils.cpp +++ b/src/paimon/core/catalog/catalog_utils.cpp @@ -18,10 +18,13 @@ #include "paimon/core/catalog/catalog_utils.h" +#include +#include #include #include "fmt/format.h" #include "paimon/catalog/catalog.h" +#include "paimon/common/utils/string_utils.h" #include "paimon/result.h" namespace paimon { @@ -33,6 +36,27 @@ Status SystemTableError(const Identifier& identifier, const std::string& action) action, identifier.ToString())); } +/// Rejects names that cannot be used as a single path component: such a name would make the +/// path built from it escape the directory it is joined to. +Status CheckValidIdentifierName(const std::string& kind, const std::string& name) { + const char* reason = nullptr; + if (StringUtils::IsNullOrWhitespaceOnly(name)) { + reason = "cannot be empty or whitespace"; + } else if (name == "." || name == "..") { + reason = "cannot be '.' or '..'"; + } else if (name.find('/') != std::string::npos || name.find('\\') != std::string::npos) { + reason = "cannot contain path separators"; + } else if (std::any_of(name.begin(), name.end(), [](char c) { + return std::iscntrl(static_cast(c)) != 0; + })) { + reason = "cannot contain control characters"; + } + if (reason != nullptr) { + return Status::Invalid(fmt::format("{} name {}: '{}'", kind, reason, name)); + } + return Status::OK(); +} + } // namespace bool CatalogUtils::IsSystemDatabase(const std::string& db_name) { @@ -70,4 +94,31 @@ Status CatalogUtils::CheckNotBranch(const Identifier& identifier, const std::str return Status::OK(); } +Status CatalogUtils::CheckValidDatabaseName(const std::string& db_name) { + return CheckValidIdentifierName("database", db_name); +} + +Status CatalogUtils::CheckValidTableName(const Identifier& identifier) { + PAIMON_ASSIGN_OR_RAISE(std::string data_table_name, identifier.GetDataTableName()); + PAIMON_RETURN_NOT_OK(CheckValidIdentifierName("table", data_table_name)); + PAIMON_ASSIGN_OR_RAISE(std::optional branch, identifier.GetBranchName()); + if (branch) { + PAIMON_RETURN_NOT_OK(CheckValidIdentifierName("branch", branch.value())); + } + PAIMON_ASSIGN_OR_RAISE(std::optional system_table, + identifier.GetSystemTableName()); + if (system_table) { + PAIMON_RETURN_NOT_OK(CheckValidIdentifierName("system table", system_table.value())); + } + return Status::OK(); +} + +Status CatalogUtils::CheckValidBranchName(const std::string& branch) { + // An empty branch selects the main branch, see BranchManager::NormalizeBranch. + if (StringUtils::IsNullOrWhitespaceOnly(branch)) { + return Status::OK(); + } + return CheckValidIdentifierName("branch", branch); +} + } // namespace paimon diff --git a/src/paimon/core/catalog/catalog_utils.h b/src/paimon/core/catalog/catalog_utils.h index e92cd8443..c1949eaff 100644 --- a/src/paimon/core/catalog/catalog_utils.h +++ b/src/paimon/core/catalog/catalog_utils.h @@ -43,6 +43,18 @@ class CatalogUtils { /// Fails when `identifier` carries a "$branch_" suffix. static Status CheckNotBranch(const Identifier& identifier, const std::string& action); + + /// Fails when `db_name` cannot be used as a single path component; such a name would let + /// the database path escape the warehouse, e.g. "../outside". + static Status CheckValidDatabaseName(const std::string& db_name); + + /// Fails when any component parsed out of the identifier's table name (data table name, + /// branch name, system table name) cannot be used as a single path component. + static Status CheckValidTableName(const Identifier& identifier); + + /// Fails when `branch` cannot be used as a single path component. An empty or + /// whitespace-only branch selects the main branch and is accepted. + static Status CheckValidBranchName(const std::string& branch); }; } // namespace paimon diff --git a/src/paimon/core/catalog/file_system_catalog.cpp b/src/paimon/core/catalog/file_system_catalog.cpp index 85907c122..651a73421 100644 --- a/src/paimon/core/catalog/file_system_catalog.cpp +++ b/src/paimon/core/catalog/file_system_catalog.cpp @@ -87,7 +87,7 @@ Status FileSystemCatalog::CreateDatabaseImpl(const std::string& db_name, fmt::join(options, ", ")); PAIMON_LOG_DEBUG(logger_, "%s", log_msg.c_str()); } - std::string db_path = NewDatabasePath(warehouse_, db_name); + PAIMON_ASSIGN_OR_RAISE(std::string db_path, NewDatabasePath(warehouse_, db_name)); PAIMON_RETURN_NOT_OK(fs_->Mkdirs(db_path)); return Status::OK(); } @@ -96,7 +96,8 @@ Result FileSystemCatalog::DatabaseExists(const std::string& db_name) const if (CatalogUtils::IsSystemDatabase(db_name)) { return true; } - return fs_->Exists(NewDatabasePath(warehouse_, db_name)); + PAIMON_ASSIGN_OR_RAISE(std::string db_path, NewDatabasePath(warehouse_, db_name)); + return fs_->Exists(db_path); } Result FileSystemCatalog::TableExists(const Identifier& identifier) const { @@ -123,7 +124,8 @@ Result FileSystemCatalog::TableExists(const Identifier& identifier) const } std::string FileSystemCatalog::GetDatabaseLocation(const std::string& db_name) const { - return NewDatabasePath(warehouse_, db_name); + // An invalid name has no valid location, keep the same convention as RestCatalog. + return NewDatabasePath(warehouse_, db_name).value_or(""); } Result FileSystemCatalog::GetTableLocation(const Identifier& identifier) const { @@ -204,16 +206,19 @@ Result FileSystemCatalog::IsSystemTable(const Identifier& identifier) { return IsSpecifiedSystemTable(identifier); } -std::string FileSystemCatalog::NewDatabasePath(const std::string& warehouse, - const std::string& db_name) { +Result FileSystemCatalog::NewDatabasePath(const std::string& warehouse, + const std::string& db_name) { + PAIMON_RETURN_NOT_OK(CatalogUtils::CheckValidDatabaseName(db_name)); return PathUtil::JoinPath(warehouse, db_name + DB_SUFFIX); } Result FileSystemCatalog::NewDataTablePath(const std::string& warehouse, const Identifier& identifier) { + PAIMON_RETURN_NOT_OK(CatalogUtils::CheckValidTableName(identifier)); PAIMON_ASSIGN_OR_RAISE(std::string data_table_name, identifier.GetDataTableName()); - return PathUtil::JoinPath(NewDatabasePath(warehouse, identifier.GetDatabaseName()), - data_table_name); + PAIMON_ASSIGN_OR_RAISE(std::string database_path, + NewDatabasePath(warehouse, identifier.GetDatabaseName())); + return PathUtil::JoinPath(database_path, data_table_name); } Result> FileSystemCatalog::ListDatabases() const { @@ -235,7 +240,7 @@ Result> FileSystemCatalog::ListTables(const std::string if (CatalogUtils::IsSystemDatabase(db_name)) { return GlobalSystemTableLoader::GetSupportedTableNames(catalog_options_); } - std::string database_path = NewDatabasePath(warehouse_, db_name); + PAIMON_ASSIGN_OR_RAISE(std::string database_path, NewDatabasePath(warehouse_, db_name)); std::vector file_status_list; PAIMON_RETURN_NOT_OK(fs_->ListDir(database_path, &file_status_list)); std::vector table_names; @@ -342,7 +347,7 @@ Status FileSystemCatalog::DropDatabase(const std::string& name, bool ignore_if_n } } - std::string db_path = NewDatabasePath(warehouse_, name); + PAIMON_ASSIGN_OR_RAISE(std::string db_path, NewDatabasePath(warehouse_, name)); if (cascade) { // List all tables in the database and drop them @@ -511,6 +516,7 @@ Status FileSystemCatalog::RenameTable(const Identifier& from_table, const Identi Result> FileSystemCatalog::ListSnapshots( const Identifier& identifier, const std::string& branch) const { + PAIMON_RETURN_NOT_OK(CatalogUtils::CheckValidBranchName(branch)); PAIMON_ASSIGN_OR_RAISE(bool exists, TableExists(identifier)); if (!exists) { return Status::NotExist(fmt::format("table {} does not exist", identifier.ToString())); diff --git a/src/paimon/core/catalog/file_system_catalog.h b/src/paimon/core/catalog/file_system_catalog.h index 3925aff84..6b27455ba 100644 --- a/src/paimon/core/catalog/file_system_catalog.h +++ b/src/paimon/core/catalog/file_system_catalog.h @@ -70,7 +70,12 @@ class FileSystemCatalog : public Catalog { const std::string& branch) const override; private: - static std::string NewDatabasePath(const std::string& warehouse, const std::string& db_name); + /// Fails when `db_name` cannot be used as a single path component, so that the returned + /// path always stays under `warehouse`. + static Result NewDatabasePath(const std::string& warehouse, + const std::string& db_name); + /// Fails when the database name or any component of the table name cannot be used as a + /// single path component, so that the returned path always stays under `warehouse`. static Result NewDataTablePath(const std::string& warehouse, const Identifier& identifier); static Result IsSpecifiedSystemTable(const Identifier& identifier); diff --git a/src/paimon/core/catalog/file_system_catalog_test.cpp b/src/paimon/core/catalog/file_system_catalog_test.cpp index 9f6a568f6..0086c5de3 100644 --- a/src/paimon/core/catalog/file_system_catalog_test.cpp +++ b/src/paimon/core/catalog/file_system_catalog_test.cpp @@ -1270,4 +1270,140 @@ TEST(FileSystemCatalogTest, TestDropTableWithBranchExternalPaths) { ASSERT_FALSE(external_exists); } +TEST(FileSystemCatalogTest, TestRejectNamesEscapingWarehouse) { + std::map options; + options[Options::FILE_SYSTEM] = "local"; + options[Options::FILE_FORMAT] = "orc"; + ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options)); + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + auto fs = core_options.GetFileSystem(); + // The warehouse is nested inside the test directory, so that a name escaping the + // warehouse would become visible right next to it. + std::string warehouse = PathUtil::JoinPath(dir->Str(), "warehouse"); + ASSERT_OK(fs->Mkdirs(warehouse)); + std::string escaped_db_path = PathUtil::JoinPath(dir->Str(), "outside.db"); + FileSystemCatalog catalog(fs, warehouse, options); + + // Creating a database whose name escapes the warehouse must fail without creating + // anything outside of the warehouse. + ASSERT_NOK_WITH_MSG(catalog.CreateDatabase("../outside", {}, /*ignore_if_exists=*/false), + "cannot contain path separators"); + ASSERT_OK_AND_ASSIGN(bool escaped_exists, fs->Exists(escaped_db_path)); + ASSERT_FALSE(escaped_exists); + + // A directory that already exists outside of the warehouse must not be deletable. + ASSERT_OK(fs->Mkdirs(escaped_db_path)); + ASSERT_NOK_WITH_MSG(catalog.DropDatabase("../outside", /*ignore_if_not_exists=*/true, + /*cascade=*/true), + "cannot contain path separators"); + ASSERT_OK_AND_ASSIGN(escaped_exists, fs->Exists(escaped_db_path)); + ASSERT_TRUE(escaped_exists); + + ASSERT_NOK_WITH_MSG(catalog.DatabaseExists("../outside"), "cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.ListTables("../outside"), "cannot contain path separators"); + ASSERT_EQ(catalog.GetDatabaseLocation("../outside"), ""); + + arrow::FieldVector fields = {arrow::field("f0", arrow::int32())}; + arrow::Schema typed_schema(fields); + ASSERT_OK(catalog.CreateDatabase("db1", {}, /*ignore_if_exists=*/false)); + { + ::ArrowSchema schema; + ASSERT_TRUE(arrow::ExportSchema(typed_schema, &schema).ok()); + ASSERT_OK(catalog.CreateTable(Identifier("db1", "t"), &schema, {}, {}, options, + /*ignore_if_exists=*/false)); + } + + // All table entries reject escaping names before touching the file system. The schema is + // never imported on these paths, so a single exported schema can be reused. + ::ArrowSchema schema; + ASSERT_TRUE(arrow::ExportSchema(typed_schema, &schema).ok()); + const Identifier escaped_db_table("../outside", "t"); + const Identifier escaped_table("db1", "../evil"); + ASSERT_NOK_WITH_MSG(catalog.CreateTable(escaped_db_table, &schema, {}, {}, options, false), + "cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.CreateTable(escaped_table, &schema, {}, {}, options, false), + "cannot contain path separators"); + ArrowSchemaRelease(&schema); + + ASSERT_NOK_WITH_MSG(catalog.GetTableLocation(escaped_table), "cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.GetTable(escaped_table), "cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.TableExists(escaped_table), "cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.DropTable(escaped_table, /*ignore_if_not_exists=*/true), + "cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.RenameTable(Identifier("db1", "t"), escaped_table, + /*ignore_if_not_exists=*/false), + "cannot contain path separators"); + + // The branch component of a table name and the branch argument end up in the path too. + ASSERT_NOK_WITH_MSG(catalog.GetTableLocation(Identifier("db1", "t$branch_../../x")), + "branch name cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.ListSnapshots(Identifier("db1", "t"), "../../x"), + "branch name cannot contain path separators"); + + // Nothing escaped the warehouse and the valid table is untouched. + ASSERT_OK_AND_ASSIGN(escaped_exists, fs->Exists(PathUtil::JoinPath(dir->Str(), "db1.db"))); + ASSERT_FALSE(escaped_exists); + ASSERT_OK_AND_ASSIGN(bool table_exists, catalog.TableExists(Identifier("db1", "t"))); + ASSERT_TRUE(table_exists); +} + +TEST(FileSystemCatalogTest, TestIdentifierNameValidationRules) { + std::map options; + options[Options::FILE_SYSTEM] = "local"; + options[Options::FILE_FORMAT] = "orc"; + ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options)); + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options); + ASSERT_OK(catalog.CreateDatabase("db1", {}, /*ignore_if_exists=*/false)); + + arrow::FieldVector fields = {arrow::field("f0", arrow::int32())}; + arrow::Schema typed_schema(fields); + struct InvalidName { + std::string name; + std::string db_error; + // An empty table name is already rejected by the identifier itself. + std::string table_error; + }; + const std::vector invalid_names = { + {"", "cannot be empty or whitespace", "Invalid table name"}, + {" ", "cannot be empty or whitespace", "cannot be empty or whitespace"}, + {".", "cannot be '.' or '..'", "cannot be '.' or '..'"}, + {"..", "cannot be '.' or '..'", "cannot be '.' or '..'"}, + {"../escaped", "cannot contain path separators", "cannot contain path separators"}, + {"nested/name", "cannot contain path separators", "cannot contain path separators"}, + {"back\\slash", "cannot contain path separators", "cannot contain path separators"}, + {"line\nfeed", "cannot contain control characters", "cannot contain control characters"}, + {std::string("nul\0byte", 8), "cannot contain control characters", + "cannot contain control characters"}, + }; + ::ArrowSchema schema; + ASSERT_TRUE(arrow::ExportSchema(typed_schema, &schema).ok()); + for (const auto& invalid_name : invalid_names) { + ASSERT_NOK_WITH_MSG(catalog.CreateDatabase(invalid_name.name, {}, + /*ignore_if_exists=*/true), + invalid_name.db_error); + ASSERT_NOK_WITH_MSG(catalog.CreateTable(Identifier("db1", invalid_name.name), &schema, {}, + {}, options, /*ignore_if_exists=*/true), + invalid_name.table_error); + } + ArrowSchemaRelease(&schema); + + // Names that merely contain a dot or non-ascii characters stay usable. + for (const std::string& db_name : {"my.db", "a..b", "数据"}) { + ASSERT_OK(catalog.CreateDatabase(db_name, {}, /*ignore_if_exists=*/false)); + ASSERT_OK_AND_ASSIGN(bool db_exists, catalog.DatabaseExists(db_name)); + ASSERT_TRUE(db_exists); + } + for (const std::string& table_name : {"orders", "订单"}) { + ::ArrowSchema valid_schema; + ASSERT_TRUE(arrow::ExportSchema(typed_schema, &valid_schema).ok()); + ASSERT_OK(catalog.CreateTable(Identifier("db1", table_name), &valid_schema, {}, {}, options, + /*ignore_if_exists=*/false)); + ASSERT_OK_AND_ASSIGN(bool table_exists, catalog.TableExists(Identifier("db1", table_name))); + ASSERT_TRUE(table_exists); + } +} + } // namespace paimon::test From 4aa9c29a6914596be5766d89124519fed36668de Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Mon, 31 Aug 2026 15:46:06 +0800 Subject: [PATCH 2/7] fix --- src/paimon/core/catalog/catalog_utils.h | 4 +- .../core/catalog/file_system_catalog_test.cpp | 51 +++++++++---------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/paimon/core/catalog/catalog_utils.h b/src/paimon/core/catalog/catalog_utils.h index c1949eaff..0f4f6780e 100644 --- a/src/paimon/core/catalog/catalog_utils.h +++ b/src/paimon/core/catalog/catalog_utils.h @@ -44,8 +44,8 @@ class CatalogUtils { /// Fails when `identifier` carries a "$branch_" suffix. static Status CheckNotBranch(const Identifier& identifier, const std::string& action); - /// Fails when `db_name` cannot be used as a single path component; such a name would let - /// the database path escape the warehouse, e.g. "../outside". + /// Fails when `db_name` cannot be used as a single path component, which is required to + /// keep the database path under the warehouse. static Status CheckValidDatabaseName(const std::string& db_name); /// Fails when any component parsed out of the identifier's table name (data table name, diff --git a/src/paimon/core/catalog/file_system_catalog_test.cpp b/src/paimon/core/catalog/file_system_catalog_test.cpp index 0086c5de3..231876b87 100644 --- a/src/paimon/core/catalog/file_system_catalog_test.cpp +++ b/src/paimon/core/catalog/file_system_catalog_test.cpp @@ -1270,7 +1270,7 @@ TEST(FileSystemCatalogTest, TestDropTableWithBranchExternalPaths) { ASSERT_FALSE(external_exists); } -TEST(FileSystemCatalogTest, TestRejectNamesEscapingWarehouse) { +TEST(FileSystemCatalogTest, TestRejectInvalidNames) { std::map options; options[Options::FILE_SYSTEM] = "local"; options[Options::FILE_FORMAT] = "orc"; @@ -1278,27 +1278,26 @@ TEST(FileSystemCatalogTest, TestRejectNamesEscapingWarehouse) { auto dir = UniqueTestDirectory::Create(); ASSERT_TRUE(dir); auto fs = core_options.GetFileSystem(); - // The warehouse is nested inside the test directory, so that a name escaping the - // warehouse would become visible right next to it. + // The warehouse is nested inside the test directory, so that the test can assert the + // surrounding directory stays untouched. std::string warehouse = PathUtil::JoinPath(dir->Str(), "warehouse"); ASSERT_OK(fs->Mkdirs(warehouse)); - std::string escaped_db_path = PathUtil::JoinPath(dir->Str(), "outside.db"); + std::string outer_db_path = PathUtil::JoinPath(dir->Str(), "outside.db"); FileSystemCatalog catalog(fs, warehouse, options); - // Creating a database whose name escapes the warehouse must fail without creating - // anything outside of the warehouse. + // A rejected database name must fail without creating anything on disk. ASSERT_NOK_WITH_MSG(catalog.CreateDatabase("../outside", {}, /*ignore_if_exists=*/false), "cannot contain path separators"); - ASSERT_OK_AND_ASSIGN(bool escaped_exists, fs->Exists(escaped_db_path)); - ASSERT_FALSE(escaped_exists); + ASSERT_OK_AND_ASSIGN(bool path_exists, fs->Exists(outer_db_path)); + ASSERT_FALSE(path_exists); - // A directory that already exists outside of the warehouse must not be deletable. - ASSERT_OK(fs->Mkdirs(escaped_db_path)); + // A directory that already exists next to the warehouse must not be deleted either. + ASSERT_OK(fs->Mkdirs(outer_db_path)); ASSERT_NOK_WITH_MSG(catalog.DropDatabase("../outside", /*ignore_if_not_exists=*/true, /*cascade=*/true), "cannot contain path separators"); - ASSERT_OK_AND_ASSIGN(escaped_exists, fs->Exists(escaped_db_path)); - ASSERT_TRUE(escaped_exists); + ASSERT_OK_AND_ASSIGN(path_exists, fs->Exists(outer_db_path)); + ASSERT_TRUE(path_exists); ASSERT_NOK_WITH_MSG(catalog.DatabaseExists("../outside"), "cannot contain path separators"); ASSERT_NOK_WITH_MSG(catalog.ListTables("../outside"), "cannot contain path separators"); @@ -1314,36 +1313,36 @@ TEST(FileSystemCatalogTest, TestRejectNamesEscapingWarehouse) { /*ignore_if_exists=*/false)); } - // All table entries reject escaping names before touching the file system. The schema is + // All table entries reject invalid names before touching the file system. The schema is // never imported on these paths, so a single exported schema can be reused. ::ArrowSchema schema; ASSERT_TRUE(arrow::ExportSchema(typed_schema, &schema).ok()); - const Identifier escaped_db_table("../outside", "t"); - const Identifier escaped_table("db1", "../evil"); - ASSERT_NOK_WITH_MSG(catalog.CreateTable(escaped_db_table, &schema, {}, {}, options, false), + const Identifier rejected_db_table("../outside", "t"); + const Identifier rejected_table("db1", "../evil"); + ASSERT_NOK_WITH_MSG(catalog.CreateTable(rejected_db_table, &schema, {}, {}, options, false), "cannot contain path separators"); - ASSERT_NOK_WITH_MSG(catalog.CreateTable(escaped_table, &schema, {}, {}, options, false), + ASSERT_NOK_WITH_MSG(catalog.CreateTable(rejected_table, &schema, {}, {}, options, false), "cannot contain path separators"); ArrowSchemaRelease(&schema); - ASSERT_NOK_WITH_MSG(catalog.GetTableLocation(escaped_table), "cannot contain path separators"); - ASSERT_NOK_WITH_MSG(catalog.GetTable(escaped_table), "cannot contain path separators"); - ASSERT_NOK_WITH_MSG(catalog.TableExists(escaped_table), "cannot contain path separators"); - ASSERT_NOK_WITH_MSG(catalog.DropTable(escaped_table, /*ignore_if_not_exists=*/true), + ASSERT_NOK_WITH_MSG(catalog.GetTableLocation(rejected_table), "cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.GetTable(rejected_table), "cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.TableExists(rejected_table), "cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.DropTable(rejected_table, /*ignore_if_not_exists=*/true), "cannot contain path separators"); - ASSERT_NOK_WITH_MSG(catalog.RenameTable(Identifier("db1", "t"), escaped_table, + ASSERT_NOK_WITH_MSG(catalog.RenameTable(Identifier("db1", "t"), rejected_table, /*ignore_if_not_exists=*/false), "cannot contain path separators"); - // The branch component of a table name and the branch argument end up in the path too. + // The branch component of a table name and the branch argument become path components too. ASSERT_NOK_WITH_MSG(catalog.GetTableLocation(Identifier("db1", "t$branch_../../x")), "branch name cannot contain path separators"); ASSERT_NOK_WITH_MSG(catalog.ListSnapshots(Identifier("db1", "t"), "../../x"), "branch name cannot contain path separators"); - // Nothing escaped the warehouse and the valid table is untouched. - ASSERT_OK_AND_ASSIGN(escaped_exists, fs->Exists(PathUtil::JoinPath(dir->Str(), "db1.db"))); - ASSERT_FALSE(escaped_exists); + // The surrounding directory is untouched and the valid table still works. + ASSERT_OK_AND_ASSIGN(path_exists, fs->Exists(PathUtil::JoinPath(dir->Str(), "db1.db"))); + ASSERT_FALSE(path_exists); ASSERT_OK_AND_ASSIGN(bool table_exists, catalog.TableExists(Identifier("db1", "t"))); ASSERT_TRUE(table_exists); } From f5de5ca33176d0113297662d8ad94672660e987e Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Mon, 31 Aug 2026 16:20:43 +0800 Subject: [PATCH 3/7] fix --- src/paimon/core/catalog/file_system_catalog.cpp | 6 ++++++ src/paimon/core/catalog/file_system_catalog_test.cpp | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/paimon/core/catalog/file_system_catalog.cpp b/src/paimon/core/catalog/file_system_catalog.cpp index 651a73421..c38fc46ce 100644 --- a/src/paimon/core/catalog/file_system_catalog.cpp +++ b/src/paimon/core/catalog/file_system_catalog.cpp @@ -105,6 +105,9 @@ Result FileSystemCatalog::TableExists(const Identifier& identifier) const if (CatalogUtils::IsSystemDatabase(identifier.GetDatabaseName())) { return GlobalSystemTableLoader::IsSupported(identifier.GetTableName(), catalog_options_); } + // The branch component is dropped when the data table identifier is rebuilt below, so the + // identifier is validated as a whole here. + PAIMON_RETURN_NOT_OK(CatalogUtils::CheckValidTableName(identifier)); PAIMON_ASSIGN_OR_RAISE(bool is_system_table, identifier.IsSystemTable()); if (is_system_table) { PAIMON_ASSIGN_OR_RAISE(std::optional system_table_name, @@ -289,6 +292,9 @@ Result> FileSystemCatalog::LoadTableSchema( system_table->ArrowSchema()); return std::make_shared(std::move(arrow_schema)); } + // The branch component is dropped when the data table identifier is rebuilt below, so the + // identifier is validated as a whole here. + PAIMON_RETURN_NOT_OK(CatalogUtils::CheckValidTableName(identifier)); PAIMON_ASSIGN_OR_RAISE(bool is_system_table, identifier.IsSystemTable()); if (is_system_table) { PAIMON_ASSIGN_OR_RAISE(std::optional system_table_name, diff --git a/src/paimon/core/catalog/file_system_catalog_test.cpp b/src/paimon/core/catalog/file_system_catalog_test.cpp index 231876b87..4523a9e97 100644 --- a/src/paimon/core/catalog/file_system_catalog_test.cpp +++ b/src/paimon/core/catalog/file_system_catalog_test.cpp @@ -1340,6 +1340,16 @@ TEST(FileSystemCatalogTest, TestRejectInvalidNames) { ASSERT_NOK_WITH_MSG(catalog.ListSnapshots(Identifier("db1", "t"), "../../x"), "branch name cannot contain path separators"); + // A system table identifier keeps its own branch component, which the entries resolving the + // data table must reject as well. + const Identifier rejected_branch_system_table("db1", "t$branch_../../x$snapshots"); + ASSERT_NOK_WITH_MSG(catalog.TableExists(rejected_branch_system_table), + "branch name cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.LoadTableSchema(rejected_branch_system_table), + "branch name cannot contain path separators"); + ASSERT_NOK_WITH_MSG(catalog.GetTable(rejected_branch_system_table), + "branch name cannot contain path separators"); + // The surrounding directory is untouched and the valid table still works. ASSERT_OK_AND_ASSIGN(path_exists, fs->Exists(PathUtil::JoinPath(dir->Str(), "db1.db"))); ASSERT_FALSE(path_exists); From 784e3edeca72164a60e2331096313e84d434391a Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Mon, 31 Aug 2026 17:14:28 +0800 Subject: [PATCH 4/7] fix --- src/paimon/core/catalog/file_system_catalog_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/paimon/core/catalog/file_system_catalog_test.cpp b/src/paimon/core/catalog/file_system_catalog_test.cpp index 4523a9e97..4c61fab91 100644 --- a/src/paimon/core/catalog/file_system_catalog_test.cpp +++ b/src/paimon/core/catalog/file_system_catalog_test.cpp @@ -1400,12 +1400,12 @@ TEST(FileSystemCatalogTest, TestIdentifierNameValidationRules) { ArrowSchemaRelease(&schema); // Names that merely contain a dot or non-ascii characters stay usable. - for (const std::string& db_name : {"my.db", "a..b", "数据"}) { + for (const char* db_name : {"my.db", "a..b", "数据"}) { ASSERT_OK(catalog.CreateDatabase(db_name, {}, /*ignore_if_exists=*/false)); ASSERT_OK_AND_ASSIGN(bool db_exists, catalog.DatabaseExists(db_name)); ASSERT_TRUE(db_exists); } - for (const std::string& table_name : {"orders", "订单"}) { + for (const char* table_name : {"orders", "订单"}) { ::ArrowSchema valid_schema; ASSERT_TRUE(arrow::ExportSchema(typed_schema, &valid_schema).ok()); ASSERT_OK(catalog.CreateTable(Identifier("db1", table_name), &valid_schema, {}, {}, options, From a2f225ce37204f09f38dae5273b351a49499e99e Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Tue, 1 Sep 2026 10:27:17 +0800 Subject: [PATCH 5/7] fix --- include/paimon/catalog/catalog.h | 7 ++- src/paimon/common/utils/path_util.cpp | 57 +++++++++++++++++++ src/paimon/common/utils/path_util.h | 9 +++ src/paimon/common/utils/path_util_test.cpp | 39 +++++++++++++ src/paimon/core/catalog/catalog_utils.cpp | 45 +++------------ src/paimon/core/catalog/catalog_utils.h | 4 -- .../core/catalog/file_system_catalog.cpp | 7 +-- src/paimon/core/catalog/file_system_catalog.h | 2 +- .../core/catalog/file_system_catalog_test.cpp | 6 +- src/paimon/core/core_options.cpp | 6 ++ src/paimon/core/core_options_test.cpp | 17 ++++++ src/paimon/core/operation/read_context.cpp | 2 + .../core/operation/read_context_test.cpp | 14 +++++ src/paimon/core/operation/write_context.cpp | 2 + .../core/operation/write_context_test.cpp | 14 +++++ src/paimon/core/utils/branch_manager.h | 11 ++++ src/paimon/core/utils/branch_manager_test.cpp | 17 ++++++ src/paimon/rest/rest_catalog.cpp | 6 +- src/paimon/rest/rest_catalog.h | 2 +- src/paimon/rest/rest_catalog_test.cpp | 6 +- 20 files changed, 217 insertions(+), 56 deletions(-) diff --git a/include/paimon/catalog/catalog.h b/include/paimon/catalog/catalog.h index 0dc575f6d..20cd22a23 100644 --- a/include/paimon/catalog/catalog.h +++ b/include/paimon/catalog/catalog.h @@ -165,9 +165,10 @@ class PAIMON_EXPORT Catalog { /// @note This does not check whether the database actually exists. /// /// @param db_name The name of the database to get the location for. - /// @return A string representing the expected location of the database, or an empty string - /// when the name does not form a valid location. - virtual std::string GetDatabaseLocation(const std::string& db_name) const = 0; + /// @return A result containing the expected location of the database, or an error status when + /// the name cannot form one. An implementation that resolves the location on a server, such + /// as the REST catalog, returns an empty string for a database it cannot resolve. + virtual Result GetDatabaseLocation(const std::string& db_name) const = 0; /// Returns the expected location of a specified table. /// diff --git a/src/paimon/common/utils/path_util.cpp b/src/paimon/common/utils/path_util.cpp index 143049e55..628cc7134 100644 --- a/src/paimon/common/utils/path_util.cpp +++ b/src/paimon/common/utils/path_util.cpp @@ -21,6 +21,8 @@ #include +#include +#include #include #include #include @@ -34,6 +36,41 @@ #include "paimon/status.h" namespace paimon { +namespace { + +/// Escapes the control characters of `name`, so that a rejected name cannot inject a line into +/// the log the error is written to nor truncate the C string it is copied into. Bytes of a +/// multi-byte sequence are left alone, since none of them is a control character. +std::string EscapeControlCharacters(const std::string& name) { + std::string escaped; + escaped.reserve(name.size()); + for (char c : name) { + switch (c) { + case '\\': + escaped += "\\\\"; + break; + case '\n': + escaped += "\\n"; + break; + case '\r': + escaped += "\\r"; + break; + case '\t': + escaped += "\\t"; + break; + default: + if (std::iscntrl(static_cast(c)) != 0) { + escaped += fmt::format("\\x{{{:02x}}}", static_cast(c)); + } else { + escaped += c; + } + } + } + return escaped; +} + +} // namespace + std::string Path::ToString() const { std::string ret; if (!scheme.empty()) { @@ -169,4 +206,24 @@ Result PathUtil::CreateTempPath(const std::string& path) noexcept { return JoinPath(GetParentDirPath(path), fmt::format(".{}.{}.tmp", GetName(path), uuid)); } +Status PathUtil::CheckSinglePathComponent(const std::string& kind, const std::string& name) { + const char* reason = nullptr; + if (StringUtils::IsNullOrWhitespaceOnly(name)) { + reason = "cannot be empty or whitespace"; + } else if (name == "." || name == "..") { + reason = "cannot be '.' or '..'"; + } else if (name.find('/') != std::string::npos || name.find('\\') != std::string::npos) { + reason = "cannot contain path separators"; + } else if (std::any_of(name.begin(), name.end(), [](char c) { + return std::iscntrl(static_cast(c)) != 0; + })) { + reason = "cannot contain control characters"; + } + if (reason != nullptr) { + return Status::Invalid( + fmt::format("{} name {}: '{}'", kind, reason, EscapeControlCharacters(name))); + } + return Status::OK(); +} + } // namespace paimon diff --git a/src/paimon/common/utils/path_util.h b/src/paimon/common/utils/path_util.h index 9aaff1144..7dd81c629 100644 --- a/src/paimon/common/utils/path_util.h +++ b/src/paimon/common/utils/path_util.h @@ -52,6 +52,15 @@ class PAIMON_EXPORT PathUtil { static Result ToPath(const std::string& path) noexcept; static Result NormalizePath(const std::string& path) noexcept; + /// Fails when `name` cannot be used as a single path component, which is required to keep a + /// path built with `JoinPath` under the directory it is joined to: `name` must not be empty + /// or whitespace-only, must not be "." or "..", and must contain neither a path separator + /// nor a control character. `kind` names the rejected value in the error message, which + /// reads " name : ''" and escapes the control characters of `name`. + /// + /// The check is purely lexical and needs no IO. + static Status CheckSinglePathComponent(const std::string& kind, const std::string& name); + private: static std::string NormalizeInnerPath(const std::string& path) noexcept; }; diff --git a/src/paimon/common/utils/path_util_test.cpp b/src/paimon/common/utils/path_util_test.cpp index 9ab278ef7..1f665234a 100644 --- a/src/paimon/common/utils/path_util_test.cpp +++ b/src/paimon/common/utils/path_util_test.cpp @@ -171,4 +171,43 @@ TEST(PathUtilsTest, TestCreateTempPath) { ASSERT_TRUE(StringUtils::EndsWith(tmp_name, ".tmp")); } +TEST(PathUtilsTest, TestCheckSinglePathComponent) { + // Names that stay a single path component, including names that merely contain a dot and + // names outside ascii. + for (const char* name : {"db1", "my.db", "a..b", "a b", "数据", "\u00e9t\u00e9"}) { + ASSERT_OK(PathUtil::CheckSinglePathComponent("database", name)); + } + + ASSERT_NOK_WITH_MSG(PathUtil::CheckSinglePathComponent("database", ""), + "database name cannot be empty or whitespace"); + ASSERT_NOK_WITH_MSG(PathUtil::CheckSinglePathComponent("database", " \t\n "), + "database name cannot be empty or whitespace"); + ASSERT_NOK_WITH_MSG(PathUtil::CheckSinglePathComponent("table", "."), + "table name cannot be '.' or '..'"); + ASSERT_NOK_WITH_MSG(PathUtil::CheckSinglePathComponent("table", ".."), + "table name cannot be '.' or '..'"); + ASSERT_NOK_WITH_MSG(PathUtil::CheckSinglePathComponent("table", "../escaped"), + "table name cannot contain path separators"); + ASSERT_NOK_WITH_MSG(PathUtil::CheckSinglePathComponent("table", "back\\slash"), + "table name cannot contain path separators"); + ASSERT_NOK_WITH_MSG(PathUtil::CheckSinglePathComponent("branch", "line\nfeed"), + "branch name cannot contain control characters"); + ASSERT_NOK_WITH_MSG(PathUtil::CheckSinglePathComponent("branch", std::string("nul\0byte", 8)), + "branch name cannot contain control characters"); +} + +TEST(PathUtilsTest, TestCheckSinglePathComponentEscapesRejectedName) { + // The rejected name is escaped, so that it can neither add a line to the log the error is + // written to nor truncate the C string it is copied into. + Status newline = PathUtil::CheckSinglePathComponent("database", "line\nfeed\r\t"); + ASSERT_FALSE(newline.ok()); + ASSERT_EQ(newline.ToString().find('\n'), std::string::npos); + ASSERT_NE(newline.ToString().find("line\\nfeed\\r\\t"), std::string::npos); + + Status nul = PathUtil::CheckSinglePathComponent("database", std::string("nul\0byte", 8)); + ASSERT_FALSE(nul.ok()); + ASSERT_EQ(nul.ToString().find('\0'), std::string::npos); + ASSERT_NE(nul.ToString().find("nul\\x{00}byte"), std::string::npos); +} + } // namespace paimon::test diff --git a/src/paimon/core/catalog/catalog_utils.cpp b/src/paimon/core/catalog/catalog_utils.cpp index e06c7c4fd..104a71be0 100644 --- a/src/paimon/core/catalog/catalog_utils.cpp +++ b/src/paimon/core/catalog/catalog_utils.cpp @@ -18,13 +18,12 @@ #include "paimon/core/catalog/catalog_utils.h" -#include -#include #include #include "fmt/format.h" #include "paimon/catalog/catalog.h" -#include "paimon/common/utils/string_utils.h" +#include "paimon/common/utils/path_util.h" +#include "paimon/core/utils/branch_manager.h" #include "paimon/result.h" namespace paimon { @@ -36,27 +35,6 @@ Status SystemTableError(const Identifier& identifier, const std::string& action) action, identifier.ToString())); } -/// Rejects names that cannot be used as a single path component: such a name would make the -/// path built from it escape the directory it is joined to. -Status CheckValidIdentifierName(const std::string& kind, const std::string& name) { - const char* reason = nullptr; - if (StringUtils::IsNullOrWhitespaceOnly(name)) { - reason = "cannot be empty or whitespace"; - } else if (name == "." || name == "..") { - reason = "cannot be '.' or '..'"; - } else if (name.find('/') != std::string::npos || name.find('\\') != std::string::npos) { - reason = "cannot contain path separators"; - } else if (std::any_of(name.begin(), name.end(), [](char c) { - return std::iscntrl(static_cast(c)) != 0; - })) { - reason = "cannot contain control characters"; - } - if (reason != nullptr) { - return Status::Invalid(fmt::format("{} name {}: '{}'", kind, reason, name)); - } - return Status::OK(); -} - } // namespace bool CatalogUtils::IsSystemDatabase(const std::string& db_name) { @@ -95,30 +73,25 @@ Status CatalogUtils::CheckNotBranch(const Identifier& identifier, const std::str } Status CatalogUtils::CheckValidDatabaseName(const std::string& db_name) { - return CheckValidIdentifierName("database", db_name); + return PathUtil::CheckSinglePathComponent("database", db_name); } Status CatalogUtils::CheckValidTableName(const Identifier& identifier) { PAIMON_ASSIGN_OR_RAISE(std::string data_table_name, identifier.GetDataTableName()); - PAIMON_RETURN_NOT_OK(CheckValidIdentifierName("table", data_table_name)); + PAIMON_RETURN_NOT_OK(PathUtil::CheckSinglePathComponent("table", data_table_name)); PAIMON_ASSIGN_OR_RAISE(std::optional branch, identifier.GetBranchName()); if (branch) { - PAIMON_RETURN_NOT_OK(CheckValidIdentifierName("branch", branch.value())); + // The branch of an identifier selects the same directory as the `branch` option, so both + // go through the same check. + PAIMON_RETURN_NOT_OK(BranchManager::CheckValidBranch(branch.value())); } PAIMON_ASSIGN_OR_RAISE(std::optional system_table, identifier.GetSystemTableName()); if (system_table) { - PAIMON_RETURN_NOT_OK(CheckValidIdentifierName("system table", system_table.value())); + PAIMON_RETURN_NOT_OK( + PathUtil::CheckSinglePathComponent("system table", system_table.value())); } return Status::OK(); } -Status CatalogUtils::CheckValidBranchName(const std::string& branch) { - // An empty branch selects the main branch, see BranchManager::NormalizeBranch. - if (StringUtils::IsNullOrWhitespaceOnly(branch)) { - return Status::OK(); - } - return CheckValidIdentifierName("branch", branch); -} - } // namespace paimon diff --git a/src/paimon/core/catalog/catalog_utils.h b/src/paimon/core/catalog/catalog_utils.h index 0f4f6780e..235ad19d1 100644 --- a/src/paimon/core/catalog/catalog_utils.h +++ b/src/paimon/core/catalog/catalog_utils.h @@ -51,10 +51,6 @@ class CatalogUtils { /// Fails when any component parsed out of the identifier's table name (data table name, /// branch name, system table name) cannot be used as a single path component. static Status CheckValidTableName(const Identifier& identifier); - - /// Fails when `branch` cannot be used as a single path component. An empty or - /// whitespace-only branch selects the main branch and is accepted. - static Status CheckValidBranchName(const std::string& branch); }; } // namespace paimon diff --git a/src/paimon/core/catalog/file_system_catalog.cpp b/src/paimon/core/catalog/file_system_catalog.cpp index c38fc46ce..be5f11bb8 100644 --- a/src/paimon/core/catalog/file_system_catalog.cpp +++ b/src/paimon/core/catalog/file_system_catalog.cpp @@ -126,9 +126,8 @@ Result FileSystemCatalog::TableExists(const Identifier& identifier) const return latest_schema != std::nullopt; } -std::string FileSystemCatalog::GetDatabaseLocation(const std::string& db_name) const { - // An invalid name has no valid location, keep the same convention as RestCatalog. - return NewDatabasePath(warehouse_, db_name).value_or(""); +Result FileSystemCatalog::GetDatabaseLocation(const std::string& db_name) const { + return NewDatabasePath(warehouse_, db_name); } Result FileSystemCatalog::GetTableLocation(const Identifier& identifier) const { @@ -522,7 +521,7 @@ Status FileSystemCatalog::RenameTable(const Identifier& from_table, const Identi Result> FileSystemCatalog::ListSnapshots( const Identifier& identifier, const std::string& branch) const { - PAIMON_RETURN_NOT_OK(CatalogUtils::CheckValidBranchName(branch)); + PAIMON_RETURN_NOT_OK(BranchManager::CheckValidBranch(branch)); PAIMON_ASSIGN_OR_RAISE(bool exists, TableExists(identifier)); if (!exists) { return Status::NotExist(fmt::format("table {} does not exist", identifier.ToString())); diff --git a/src/paimon/core/catalog/file_system_catalog.h b/src/paimon/core/catalog/file_system_catalog.h index 6b27455ba..a373dcfa0 100644 --- a/src/paimon/core/catalog/file_system_catalog.h +++ b/src/paimon/core/catalog/file_system_catalog.h @@ -59,7 +59,7 @@ class FileSystemCatalog : public Catalog { Result> ListTables(const std::string& db_name) const override; Result DatabaseExists(const std::string& db_name) const override; Result TableExists(const Identifier& identifier) const override; - std::string GetDatabaseLocation(const std::string& db_name) const override; + Result GetDatabaseLocation(const std::string& db_name) const override; Result GetTableLocation(const Identifier& identifier) const override; Result> LoadTableSchema(const Identifier& identifier) const override; std::string GetRootPath() const override; diff --git a/src/paimon/core/catalog/file_system_catalog_test.cpp b/src/paimon/core/catalog/file_system_catalog_test.cpp index 4c61fab91..dba3e67cd 100644 --- a/src/paimon/core/catalog/file_system_catalog_test.cpp +++ b/src/paimon/core/catalog/file_system_catalog_test.cpp @@ -61,7 +61,8 @@ TEST(FileSystemCatalogTest, TestDatabaseExists) { ASSERT_OK_AND_ASSIGN(std::vector db_names, catalog.ListDatabases()); ASSERT_EQ(1, db_names.size()); ASSERT_EQ(db_names[0], "db1"); - ASSERT_EQ(catalog.GetDatabaseLocation("db1"), PathUtil::JoinPath(dir->Str(), "db1.db")); + ASSERT_OK_AND_ASSIGN(std::string db_location, catalog.GetDatabaseLocation("db1")); + ASSERT_EQ(db_location, PathUtil::JoinPath(dir->Str(), "db1.db")); } TEST(FileSystemCatalogTest, TestInvalidCreateDatabase) { @@ -1301,7 +1302,8 @@ TEST(FileSystemCatalogTest, TestRejectInvalidNames) { ASSERT_NOK_WITH_MSG(catalog.DatabaseExists("../outside"), "cannot contain path separators"); ASSERT_NOK_WITH_MSG(catalog.ListTables("../outside"), "cannot contain path separators"); - ASSERT_EQ(catalog.GetDatabaseLocation("../outside"), ""); + ASSERT_NOK_WITH_MSG(catalog.GetDatabaseLocation("../outside"), + "cannot contain path separators"); arrow::FieldVector fields = {arrow::field("f0", arrow::int32())}; arrow::Schema typed_schema(fields); diff --git a/src/paimon/core/core_options.cpp b/src/paimon/core/core_options.cpp index 71ba73deb..385b23002 100644 --- a/src/paimon/core/core_options.cpp +++ b/src/paimon/core/core_options.cpp @@ -799,8 +799,14 @@ struct CoreOptions::Impl { parser.Parse(Options::PREFETCH_IO_METRICS_ENABLED, &prefetch_io_metrics_enabled)); // Parse scan.fallback-branch - fallback branch when partition not found PAIMON_RETURN_NOT_OK(parser.Parse(Options::SCAN_FALLBACK_BRANCH, &scan_fallback_branch)); + if (scan_fallback_branch) { + PAIMON_RETURN_NOT_OK(BranchManager::CheckValidBranch(scan_fallback_branch.value())); + } // Parse branch - branch name, default "main" PAIMON_RETURN_NOT_OK(parser.Parse(Options::BRANCH, &branch)); + // Both branches name a directory under the table root, so they must stay a single path + // component. + PAIMON_RETURN_NOT_OK(BranchManager::CheckValidBranch(branch)); // Parse scan.tag-name - optional tag name for "from-snapshot" scan mode PAIMON_RETURN_NOT_OK(parser.Parse(Options::SCAN_TAG_NAME, &scan_tag_name)); return Status::OK(); diff --git a/src/paimon/core/core_options_test.cpp b/src/paimon/core/core_options_test.cpp index c424b9cfe..56ab466d5 100644 --- a/src/paimon/core/core_options_test.cpp +++ b/src/paimon/core/core_options_test.cpp @@ -545,6 +545,23 @@ TEST(CoreOptionsTest, TestInvalidCase) { "must not be negative"); } +TEST(CoreOptionsTest, TestRejectBranchLeavingTableRoot) { + // Both branch options name a directory under the table root, so a value that is not a single + // path component is rejected before it can be joined into a path. + ASSERT_NOK_WITH_MSG(CoreOptions::FromMap({{Options::BRANCH, "rt/../../../../../outside"}}), + "branch name cannot contain path separators"); + ASSERT_NOK_WITH_MSG(CoreOptions::FromMap({{Options::BRANCH, ".."}}), + "branch name cannot be '.' or '..'"); + ASSERT_NOK_WITH_MSG( + CoreOptions::FromMap({{Options::SCAN_FALLBACK_BRANCH, "rt/../../../../../outside"}}), + "branch name cannot contain path separators"); + + // An empty branch selects the main branch and stays accepted. + ASSERT_OK(CoreOptions::FromMap({{Options::BRANCH, ""}})); + ASSERT_OK(CoreOptions::FromMap({{Options::BRANCH, "rt"}})); + ASSERT_OK(CoreOptions::FromMap({{Options::SCAN_FALLBACK_BRANCH, "rt"}})); +} + TEST(CoreOptionsTest, TestNestedKeyNullStrategyIsCaseInsensitive) { const std::vector> cases = { {"MERGE", CoreOptions::NestedKeyNullStrategy::MERGE}, diff --git a/src/paimon/core/operation/read_context.cpp b/src/paimon/core/operation/read_context.cpp index deacfa78b..774f2a5b8 100644 --- a/src/paimon/core/operation/read_context.cpp +++ b/src/paimon/core/operation/read_context.cpp @@ -278,6 +278,8 @@ Result> ReadContextBuilder::Finish() { if (impl_->path_.empty()) { return Status::Invalid("cannot read with empty table path"); } + // The branch names a directory under the table path, so it must stay a single path component. + PAIMON_RETURN_NOT_OK(BranchManager::CheckValidBranch(impl_->branch_)); if (impl_->enable_prefetch_ && impl_->prefetch_max_parallel_num_ == 0) { return Status::Invalid("prefetch max parallel num should be greater than 0"); } diff --git a/src/paimon/core/operation/read_context_test.cpp b/src/paimon/core/operation/read_context_test.cpp index c686cccb5..1174568f3 100644 --- a/src/paimon/core/operation/read_context_test.cpp +++ b/src/paimon/core/operation/read_context_test.cpp @@ -126,6 +126,20 @@ TEST(ReadContextTest, TestSetOptionsOverridesAddedOptions) { ASSERT_EQ(expected_options, ctx->GetOptions()); } +TEST(ReadContextTest, TestRejectBranchLeavingTablePath) { + // The branch names a directory under the table path, so a value that is not a single path + // component is rejected when the context is built. + ReadContextBuilder builder("table_root_path"); + builder.WithBranch("rt/../../../../../outside"); + ASSERT_NOK_WITH_MSG(builder.Finish(), "branch name cannot contain path separators"); + + // An empty branch selects the main branch and stays accepted. + ReadContextBuilder main_builder("table_root_path"); + main_builder.WithBranch(""); + ASSERT_OK_AND_ASSIGN(auto ctx, main_builder.Finish()); + ASSERT_EQ("", ctx->GetBranch()); +} + TEST(ReadContextTest, TestFileSystemAndSchemeMapConflict) { ReadContextBuilder builder("table_root_path"); auto fs = std::make_shared(); diff --git a/src/paimon/core/operation/write_context.cpp b/src/paimon/core/operation/write_context.cpp index 457f8d5b0..c186a6c82 100644 --- a/src/paimon/core/operation/write_context.cpp +++ b/src/paimon/core/operation/write_context.cpp @@ -198,6 +198,8 @@ Result> WriteContextBuilder::Finish() { if (impl_->root_path_.empty()) { return Status::Invalid("root path is empty"); } + // The branch names a directory under the root path, so it must stay a single path component. + PAIMON_RETURN_NOT_OK(BranchManager::CheckValidBranch(impl_->branch_)); bool enable_multi_thread_spill = impl_->spill_thread_number_ > 0; if (enable_multi_thread_spill) { PAIMON_RETURN_NOT_OK_FROM_ARROW( diff --git a/src/paimon/core/operation/write_context_test.cpp b/src/paimon/core/operation/write_context_test.cpp index ef2225c6e..b41d147fb 100644 --- a/src/paimon/core/operation/write_context_test.cpp +++ b/src/paimon/core/operation/write_context_test.cpp @@ -100,6 +100,20 @@ TEST(WriteContextTest, TestSetOptionsOverridesAddedOptions) { ASSERT_EQ(expected_options, ctx->GetOptions()); } +TEST(WriteContextTest, TestRejectBranchLeavingRootPath) { + // The branch names a directory under the root path, so a value that is not a single path + // component is rejected when the context is built. + WriteContextBuilder builder("table_root_path", "commit_user_1"); + builder.WithBranch("rt/../../../../../outside"); + ASSERT_NOK_WITH_MSG(builder.Finish(), "branch name cannot contain path separators"); + + // An empty branch selects the main branch and stays accepted. + WriteContextBuilder main_builder("table_root_path", "commit_user_1"); + main_builder.WithBranch(""); + ASSERT_OK_AND_ASSIGN(auto ctx, main_builder.Finish()); + ASSERT_EQ("", ctx->GetBranch()); +} + TEST(WriteContextTest, TestSetWriteBufferSpillThreadNumber) { WriteContextBuilder builder("table_root_path", "commit_user_1"); builder.SetWriteBufferSpillThreadNumber(2); diff --git a/src/paimon/core/utils/branch_manager.h b/src/paimon/core/utils/branch_manager.h index 5a4a58216..9639fa9f2 100644 --- a/src/paimon/core/utils/branch_manager.h +++ b/src/paimon/core/utils/branch_manager.h @@ -24,6 +24,7 @@ #include "paimon/common/utils/path_util.h" #include "paimon/common/utils/string_utils.h" #include "paimon/result.h" +#include "paimon/status.h" namespace paimon { class FileSystem; @@ -44,6 +45,16 @@ class BranchManager { return StringUtils::IsNullOrWhitespaceOnly(branch) ? DEFAULT_MAIN_BRANCH : branch; } + /// Fails when `branch` cannot be used as a single path component, which is required to keep + /// the branch path under the table root. A branch that `NormalizeBranch` maps to `main` + /// names no directory of its own and is therefore accepted. + static Status CheckValidBranch(const std::string& branch) { + if (StringUtils::IsNullOrWhitespaceOnly(branch)) { + return Status::OK(); + } + return PathUtil::CheckSinglePathComponent("branch", branch); + } + /// Returns the table root path for the selected branch. static std::string BranchPath(const std::string& table_root, const std::string& branch) { return IsMainBranch(branch) diff --git a/src/paimon/core/utils/branch_manager_test.cpp b/src/paimon/core/utils/branch_manager_test.cpp index e3731fa08..946a4b93c 100644 --- a/src/paimon/core/utils/branch_manager_test.cpp +++ b/src/paimon/core/utils/branch_manager_test.cpp @@ -19,6 +19,7 @@ #include "paimon/core/utils/branch_manager.h" #include "gtest/gtest.h" +#include "paimon/testing/utils/testharness.h" namespace paimon::test { TEST(BranchManagerTest, TestIsMainBranch) { @@ -41,4 +42,20 @@ TEST(BranchManagerTest, TestBranchPath) { ASSERT_EQ(BranchManager::BranchPath("/root", BranchManager::DEFAULT_MAIN_BRANCH), "/root"); ASSERT_EQ(BranchManager::BranchPath("/root", "data"), "/root/branch/branch-data"); } + +TEST(BranchManagerTest, TestCheckValidBranch) { + ASSERT_OK(BranchManager::CheckValidBranch(BranchManager::DEFAULT_MAIN_BRANCH)); + ASSERT_OK(BranchManager::CheckValidBranch("data")); + ASSERT_OK(BranchManager::CheckValidBranch("d a t a")); + // A branch `NormalizeBranch` maps to `main` names no directory of its own. + ASSERT_OK(BranchManager::CheckValidBranch("")); + ASSERT_OK(BranchManager::CheckValidBranch(" ")); + + // A branch that would leave the table root is rejected. + ASSERT_NOK_WITH_MSG(BranchManager::CheckValidBranch(".."), "branch name cannot be '.' or '..'"); + ASSERT_NOK_WITH_MSG(BranchManager::CheckValidBranch("rt/../../../../../outside"), + "branch name cannot contain path separators"); + ASSERT_NOK_WITH_MSG(BranchManager::CheckValidBranch("line\nfeed"), + "branch name cannot contain control characters"); +} } // namespace paimon::test diff --git a/src/paimon/rest/rest_catalog.cpp b/src/paimon/rest/rest_catalog.cpp index eb86035c5..006d8c6b4 100644 --- a/src/paimon/rest/rest_catalog.cpp +++ b/src/paimon/rest/rest_catalog.cpp @@ -159,16 +159,16 @@ Status RestCatalog::DropDatabase(const std::string& name, bool ignore_if_not_exi return status; } -std::string RestCatalog::GetDatabaseLocation(const std::string& db_name) const { +Result RestCatalog::GetDatabaseLocation(const std::string& db_name) const { // The virtual "sys" database has no location and is unknown to the server. if (CatalogUtils::IsSystemDatabase(db_name)) { - return ""; + return std::string(); } Result response = api_->GetDatabase(db_name); if (!response.ok()) { PAIMON_LOG_WARN(logger_, "failed to get location of database %s: %s", db_name.c_str(), response.status().ToString().c_str()); - return ""; + return std::string(); } return response.value().GetLocation(); } diff --git a/src/paimon/rest/rest_catalog.h b/src/paimon/rest/rest_catalog.h index e952263aa..c0f4a39de 100644 --- a/src/paimon/rest/rest_catalog.h +++ b/src/paimon/rest/rest_catalog.h @@ -66,7 +66,7 @@ class RestCatalog : public Catalog { Result> ListTables(const std::string& db_name) const override; Result DatabaseExists(const std::string& db_name) const override; Result TableExists(const Identifier& identifier) const override; - std::string GetDatabaseLocation(const std::string& db_name) const override; + Result GetDatabaseLocation(const std::string& db_name) const override; Result GetTableLocation(const Identifier& identifier) const override; Result> LoadTableSchema(const Identifier& identifier) const override; std::string GetRootPath() const override; diff --git a/src/paimon/rest/rest_catalog_test.cpp b/src/paimon/rest/rest_catalog_test.cpp index 140f3f4c1..d7646231b 100644 --- a/src/paimon/rest/rest_catalog_test.cpp +++ b/src/paimon/rest/rest_catalog_test.cpp @@ -444,8 +444,10 @@ TEST_F(RestCatalogTest, DatabaseOperations) { ASSERT_OK_AND_ASSIGN(exists, catalog->DatabaseExists("db3")); ASSERT_FALSE(exists); - ASSERT_EQ("wh1/db1.db", catalog->GetDatabaseLocation("db1")); - ASSERT_EQ("", catalog->GetDatabaseLocation("db3")); + ASSERT_OK_AND_ASSIGN(std::string db1_location, catalog->GetDatabaseLocation("db1")); + ASSERT_EQ("wh1/db1.db", db1_location); + ASSERT_OK_AND_ASSIGN(std::string db3_location, catalog->GetDatabaseLocation("db3")); + ASSERT_EQ("", db3_location); ASSERT_OK(catalog->DropDatabase("db2", /*ignore_if_not_exists=*/false, /*cascade=*/false)); ASSERT_OK(catalog->DropDatabase("db2", /*ignore_if_not_exists=*/true, /*cascade=*/false)); From 28f31b7fca765f921deb634660aa59c8cce05409 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Tue, 1 Sep 2026 15:26:58 +0800 Subject: [PATCH 6/7] fix --- include/paimon/catalog/catalog.h | 15 +++++++-------- src/paimon/rest/rest_catalog.cpp | 9 ++------- src/paimon/rest/rest_catalog_test.cpp | 8 ++++++-- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/include/paimon/catalog/catalog.h b/include/paimon/catalog/catalog.h index 20cd22a23..1162521a9 100644 --- a/include/paimon/catalog/catalog.h +++ b/include/paimon/catalog/catalog.h @@ -162,21 +162,20 @@ class PAIMON_EXPORT Catalog { /// Returns the expected location of a specified database. /// - /// @note This does not check whether the database actually exists. - /// /// @param db_name The name of the database to get the location for. - /// @return A result containing the expected location of the database, or an error status when - /// the name cannot form one. An implementation that resolves the location on a server, such - /// as the REST catalog, returns an empty string for a database it cannot resolve. + /// @return A result containing the expected location of the database, or an error status on + /// failure. An implementation that builds the location from the warehouse path, such as the + /// file system catalog, answers without checking whether the database exists. One that resolves + /// the location on a server, such as the REST catalog, propagates the server's error and so + /// fails for a database that does not exist. virtual Result GetDatabaseLocation(const std::string& db_name) const = 0; /// Returns the expected location of a specified table. /// - /// @note This does not check whether the table actually exists. - /// /// @param identifier The table identifier containing database and table name. /// @return A result containing the expected location of the table, or an error status on - /// failure. + /// failure. Whether a missing table is an error depends on the implementation, in the same way + /// as for `GetDatabaseLocation`. virtual Result GetTableLocation(const Identifier& identifier) const = 0; /// Returns the root path of the catalog. diff --git a/src/paimon/rest/rest_catalog.cpp b/src/paimon/rest/rest_catalog.cpp index 006d8c6b4..a6abf18fa 100644 --- a/src/paimon/rest/rest_catalog.cpp +++ b/src/paimon/rest/rest_catalog.cpp @@ -164,13 +164,8 @@ Result RestCatalog::GetDatabaseLocation(const std::string& db_name) if (CatalogUtils::IsSystemDatabase(db_name)) { return std::string(); } - Result response = api_->GetDatabase(db_name); - if (!response.ok()) { - PAIMON_LOG_WARN(logger_, "failed to get location of database %s: %s", db_name.c_str(), - response.status().ToString().c_str()); - return std::string(); - } - return response.value().GetLocation(); + PAIMON_ASSIGN_OR_RAISE(GetDatabaseResponse response, api_->GetDatabase(db_name)); + return response.GetLocation(); } Result> RestCatalog::ListTables(const std::string& db_name) const { diff --git a/src/paimon/rest/rest_catalog_test.cpp b/src/paimon/rest/rest_catalog_test.cpp index d7646231b..ed61d859a 100644 --- a/src/paimon/rest/rest_catalog_test.cpp +++ b/src/paimon/rest/rest_catalog_test.cpp @@ -446,8 +446,12 @@ TEST_F(RestCatalogTest, DatabaseOperations) { ASSERT_OK_AND_ASSIGN(std::string db1_location, catalog->GetDatabaseLocation("db1")); ASSERT_EQ("wh1/db1.db", db1_location); - ASSERT_OK_AND_ASSIGN(std::string db3_location, catalog->GetDatabaseLocation("db3")); - ASSERT_EQ("", db3_location); + // the location is resolved on the server, so an unknown database is reported as an error + Status no_location = catalog->GetDatabaseLocation("db3").status(); + ASSERT_TRUE(no_location.IsNotExist()) << no_location.ToString(); + // the virtual "sys" database is never asked about and has no location + ASSERT_OK_AND_ASSIGN(std::string sys_location, catalog->GetDatabaseLocation("sys")); + ASSERT_EQ("", sys_location); ASSERT_OK(catalog->DropDatabase("db2", /*ignore_if_not_exists=*/false, /*cascade=*/false)); ASSERT_OK(catalog->DropDatabase("db2", /*ignore_if_not_exists=*/true, /*cascade=*/false)); From 1689eca40a493c638d004f910c04d12c34fe63af Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Tue, 1 Sep 2026 16:38:14 +0800 Subject: [PATCH 7/7] fix --- .../commit/realtime_commit_properties_test.cpp | 4 ++++ src/paimon/core/utils/branch_manager.h | 13 ++++++++----- src/paimon/core/utils/branch_manager_test.cpp | 4 ++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/paimon/core/operation/commit/realtime_commit_properties_test.cpp b/src/paimon/core/operation/commit/realtime_commit_properties_test.cpp index afe529e26..ce9f2b53d 100644 --- a/src/paimon/core/operation/commit/realtime_commit_properties_test.cpp +++ b/src/paimon/core/operation/commit/realtime_commit_properties_test.cpp @@ -185,6 +185,10 @@ TEST_F(RealtimeCommitPropertiesTest, PartitionBucketAndOffsetsDirectory) { ASSERT_EQ("/table/metadata", RealtimeCommitProperties::OffsetsDirectory("/table", "main")); ASSERT_EQ("/table/branch/branch-dev/metadata", RealtimeCommitProperties::OffsetsDirectory("/table", "dev")); + // A branch that selects the main branch resolves to the main offsets directory, even when the + // caller passes the raw option value instead of the normalized one a commit writes with. + ASSERT_EQ("/table/metadata", RealtimeCommitProperties::OffsetsDirectory("/table", "")); + ASSERT_EQ("/table/metadata", RealtimeCommitProperties::OffsetsDirectory("/table", " ")); } TEST_F(RealtimeCommitPropertiesTest, ReadOffsetsWithoutProgress) { diff --git a/src/paimon/core/utils/branch_manager.h b/src/paimon/core/utils/branch_manager.h index 9639fa9f2..975cec9ae 100644 --- a/src/paimon/core/utils/branch_manager.h +++ b/src/paimon/core/utils/branch_manager.h @@ -55,12 +55,15 @@ class BranchManager { return PathUtil::CheckSinglePathComponent("branch", branch); } - /// Returns the table root path for the selected branch. + /// Returns the table root path for the selected branch. A branch that `NormalizeBranch` maps + /// to `main` resolves to the table root, so that a caller passing a raw option value cannot + /// end up with a directory of its own. static std::string BranchPath(const std::string& table_root, const std::string& branch) { - return IsMainBranch(branch) - ? table_root - : PathUtil::JoinPath(table_root, - "/branch/" + std::string(BRANCH_PREFIX) + branch); + const std::string normalized = NormalizeBranch(branch); + if (IsMainBranch(normalized)) { + return table_root; + } + return PathUtil::JoinPath(table_root, "/branch/" + std::string(BRANCH_PREFIX) + normalized); } /// Returns whether the branch is the default main branch. diff --git a/src/paimon/core/utils/branch_manager_test.cpp b/src/paimon/core/utils/branch_manager_test.cpp index 946a4b93c..74fdd047b 100644 --- a/src/paimon/core/utils/branch_manager_test.cpp +++ b/src/paimon/core/utils/branch_manager_test.cpp @@ -41,6 +41,10 @@ TEST(BranchManagerTest, TestNormalizeBranch) { TEST(BranchManagerTest, TestBranchPath) { ASSERT_EQ(BranchManager::BranchPath("/root", BranchManager::DEFAULT_MAIN_BRANCH), "/root"); ASSERT_EQ(BranchManager::BranchPath("/root", "data"), "/root/branch/branch-data"); + // A branch `NormalizeBranch` maps to `main` resolves to the table root, so that a raw option + // value cannot select a directory the main branch never writes to. + ASSERT_EQ(BranchManager::BranchPath("/root", ""), "/root"); + ASSERT_EQ(BranchManager::BranchPath("/root", " "), "/root"); } TEST(BranchManagerTest, TestCheckValidBranch) {