fix(catalog): validate identifier names used to build catalog paths (#264)
diff --git a/include/paimon/catalog/catalog.h b/include/paimon/catalog/catalog.h
index 9213bde..1162521 100644
--- a/include/paimon/catalog/catalog.h
+++ b/include/paimon/catalog/catalog.h
@@ -162,19 +162,20 @@
/// 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 string representing the expected location of the database.
- 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 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<std::string> 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<std::string> GetTableLocation(const Identifier& identifier) const = 0;
/// Returns the root path of the catalog.
diff --git a/src/paimon/common/utils/path_util.cpp b/src/paimon/common/utils/path_util.cpp
index 143049e..628cc71 100644
--- a/src/paimon/common/utils/path_util.cpp
+++ b/src/paimon/common/utils/path_util.cpp
@@ -21,6 +21,8 @@
#include <unistd.h>
+#include <algorithm>
+#include <cctype>
#include <cerrno>
#include <cstddef>
#include <cstdint>
@@ -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<unsigned char>(c)) != 0) {
+ escaped += fmt::format("\\x{{{:02x}}}", static_cast<unsigned char>(c));
+ } else {
+ escaped += c;
+ }
+ }
+ }
+ return escaped;
+}
+
+} // namespace
+
std::string Path::ToString() const {
std::string ret;
if (!scheme.empty()) {
@@ -169,4 +206,24 @@
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<unsigned char>(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 9aaff11..7dd81c6 100644
--- a/src/paimon/common/utils/path_util.h
+++ b/src/paimon/common/utils/path_util.h
@@ -52,6 +52,15 @@
static Result<Path> ToPath(const std::string& path) noexcept;
static Result<std::string> 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 "<kind> name <reason>: '<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 9ab278e..1f66523 100644
--- a/src/paimon/common/utils/path_util_test.cpp
+++ b/src/paimon/common/utils/path_util_test.cpp
@@ -171,4 +171,43 @@
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 eae23ad..104a71b 100644
--- a/src/paimon/core/catalog/catalog_utils.cpp
+++ b/src/paimon/core/catalog/catalog_utils.cpp
@@ -22,6 +22,8 @@
#include "fmt/format.h"
#include "paimon/catalog/catalog.h"
+#include "paimon/common/utils/path_util.h"
+#include "paimon/core/utils/branch_manager.h"
#include "paimon/result.h"
namespace paimon {
@@ -70,4 +72,26 @@
return Status::OK();
}
+Status CatalogUtils::CheckValidDatabaseName(const std::string& 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(PathUtil::CheckSinglePathComponent("table", data_table_name));
+ PAIMON_ASSIGN_OR_RAISE(std::optional<std::string> branch, identifier.GetBranchName());
+ if (branch) {
+ // 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<std::string> system_table,
+ identifier.GetSystemTableName());
+ if (system_table) {
+ PAIMON_RETURN_NOT_OK(
+ PathUtil::CheckSinglePathComponent("system table", system_table.value()));
+ }
+ return Status::OK();
+}
+
} // namespace paimon
diff --git a/src/paimon/core/catalog/catalog_utils.h b/src/paimon/core/catalog/catalog_utils.h
index e92cd84..235ad19 100644
--- a/src/paimon/core/catalog/catalog_utils.h
+++ b/src/paimon/core/catalog/catalog_utils.h
@@ -43,6 +43,14 @@
/// 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, 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,
+ /// branch name, system table name) cannot be used as a single path component.
+ static Status CheckValidTableName(const Identifier& identifier);
};
} // namespace paimon
diff --git a/src/paimon/core/catalog/file_system_catalog.cpp b/src/paimon/core/catalog/file_system_catalog.cpp
index 85907c1..be5f11b 100644
--- a/src/paimon/core/catalog/file_system_catalog.cpp
+++ b/src/paimon/core/catalog/file_system_catalog.cpp
@@ -87,7 +87,7 @@
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 @@
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<bool> FileSystemCatalog::TableExists(const Identifier& identifier) const {
@@ -104,6 +105,9 @@
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<std::string> system_table_name,
@@ -122,7 +126,7 @@
return latest_schema != std::nullopt;
}
-std::string FileSystemCatalog::GetDatabaseLocation(const std::string& db_name) const {
+Result<std::string> FileSystemCatalog::GetDatabaseLocation(const std::string& db_name) const {
return NewDatabasePath(warehouse_, db_name);
}
@@ -204,16 +208,19 @@
return IsSpecifiedSystemTable(identifier);
}
-std::string FileSystemCatalog::NewDatabasePath(const std::string& warehouse,
- const std::string& db_name) {
+Result<std::string> 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<std::string> 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<std::vector<std::string>> FileSystemCatalog::ListDatabases() const {
@@ -235,7 +242,7 @@
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<BasicFileStatus> file_status_list;
PAIMON_RETURN_NOT_OK(fs_->ListDir(database_path, &file_status_list));
std::vector<std::string> table_names;
@@ -284,6 +291,9 @@
system_table->ArrowSchema());
return std::make_shared<SystemTableSchema>(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<std::string> system_table_name,
@@ -342,7 +352,7 @@
}
}
- 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 +521,7 @@
Result<std::vector<SnapshotInfo>> FileSystemCatalog::ListSnapshots(
const Identifier& identifier, const std::string& branch) const {
+ 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 3925aff..a373dcf 100644
--- a/src/paimon/core/catalog/file_system_catalog.h
+++ b/src/paimon/core/catalog/file_system_catalog.h
@@ -59,7 +59,7 @@
Result<std::vector<std::string>> ListTables(const std::string& db_name) const override;
Result<bool> DatabaseExists(const std::string& db_name) const override;
Result<bool> TableExists(const Identifier& identifier) const override;
- std::string GetDatabaseLocation(const std::string& db_name) const override;
+ Result<std::string> GetDatabaseLocation(const std::string& db_name) const override;
Result<std::string> GetTableLocation(const Identifier& identifier) const override;
Result<std::shared_ptr<Schema>> LoadTableSchema(const Identifier& identifier) const override;
std::string GetRootPath() const override;
@@ -70,7 +70,12 @@
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<std::string> 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<std::string> NewDataTablePath(const std::string& warehouse,
const Identifier& identifier);
static Result<bool> 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 9f6a568..dba3e67 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 @@
ASSERT_OK_AND_ASSIGN(std::vector<std::string> 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) {
@@ -1270,4 +1271,150 @@
ASSERT_FALSE(external_exists);
}
+TEST(FileSystemCatalogTest, TestRejectInvalidNames) {
+ std::map<std::string, std::string> 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 the test can assert the
+ // surrounding directory stays untouched.
+ std::string warehouse = PathUtil::JoinPath(dir->Str(), "warehouse");
+ ASSERT_OK(fs->Mkdirs(warehouse));
+ std::string outer_db_path = PathUtil::JoinPath(dir->Str(), "outside.db");
+ FileSystemCatalog catalog(fs, warehouse, options);
+
+ // 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 path_exists, fs->Exists(outer_db_path));
+ ASSERT_FALSE(path_exists);
+
+ // 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(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");
+ ASSERT_NOK_WITH_MSG(catalog.GetDatabaseLocation("../outside"),
+ "cannot contain path separators");
+
+ 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 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 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(rejected_table, &schema, {}, {}, options, false),
+ "cannot contain path separators");
+ ArrowSchemaRelease(&schema);
+
+ 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"), rejected_table,
+ /*ignore_if_not_exists=*/false),
+ "cannot contain path separators");
+
+ // 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");
+
+ // 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);
+ ASSERT_OK_AND_ASSIGN(bool table_exists, catalog.TableExists(Identifier("db1", "t")));
+ ASSERT_TRUE(table_exists);
+}
+
+TEST(FileSystemCatalogTest, TestIdentifierNameValidationRules) {
+ std::map<std::string, std::string> 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<InvalidName> 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 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 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,
+ /*ignore_if_exists=*/false));
+ ASSERT_OK_AND_ASSIGN(bool table_exists, catalog.TableExists(Identifier("db1", table_name)));
+ ASSERT_TRUE(table_exists);
+ }
+}
+
} // namespace paimon::test
diff --git a/src/paimon/core/core_options.cpp b/src/paimon/core/core_options.cpp
index 71ba73d..385b230 100644
--- a/src/paimon/core/core_options.cpp
+++ b/src/paimon/core/core_options.cpp
@@ -799,8 +799,14 @@
parser.Parse<bool>(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 c424b9c..56ab466 100644
--- a/src/paimon/core/core_options_test.cpp
+++ b/src/paimon/core/core_options_test.cpp
@@ -545,6 +545,23 @@
"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<std::pair<std::string, CoreOptions::NestedKeyNullStrategy>> cases = {
{"MERGE", CoreOptions::NestedKeyNullStrategy::MERGE},
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 afe529e..ce9f2b5 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 @@
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/operation/read_context.cpp b/src/paimon/core/operation/read_context.cpp
index deacfa7..774f2a5 100644
--- a/src/paimon/core/operation/read_context.cpp
+++ b/src/paimon/core/operation/read_context.cpp
@@ -278,6 +278,8 @@
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 c686ccc..1174568 100644
--- a/src/paimon/core/operation/read_context_test.cpp
+++ b/src/paimon/core/operation/read_context_test.cpp
@@ -126,6 +126,20 @@
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<MockFileSystem>();
diff --git a/src/paimon/core/operation/write_context.cpp b/src/paimon/core/operation/write_context.cpp
index 457f8d5..c186a6c 100644
--- a/src/paimon/core/operation/write_context.cpp
+++ b/src/paimon/core/operation/write_context.cpp
@@ -198,6 +198,8 @@
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 ef2225c..b41d147 100644
--- a/src/paimon/core/operation/write_context_test.cpp
+++ b/src/paimon/core/operation/write_context_test.cpp
@@ -100,6 +100,20 @@
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 5a4a582..975cec9 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,12 +45,25 @@
return StringUtils::IsNullOrWhitespaceOnly(branch) ? DEFAULT_MAIN_BRANCH : branch;
}
- /// Returns the table root path for the selected 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. 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 e3731fa..74fdd04 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) {
@@ -40,5 +41,25 @@
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) {
+ 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 eb86035..a6abf18 100644
--- a/src/paimon/rest/rest_catalog.cpp
+++ b/src/paimon/rest/rest_catalog.cpp
@@ -159,18 +159,13 @@
return status;
}
-std::string RestCatalog::GetDatabaseLocation(const std::string& db_name) const {
+Result<std::string> 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<GetDatabaseResponse> 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 response.value().GetLocation();
+ PAIMON_ASSIGN_OR_RAISE(GetDatabaseResponse response, api_->GetDatabase(db_name));
+ return response.GetLocation();
}
Result<std::vector<std::string>> RestCatalog::ListTables(const std::string& db_name) const {
diff --git a/src/paimon/rest/rest_catalog.h b/src/paimon/rest/rest_catalog.h
index e952263..c0f4a39 100644
--- a/src/paimon/rest/rest_catalog.h
+++ b/src/paimon/rest/rest_catalog.h
@@ -66,7 +66,7 @@
Result<std::vector<std::string>> ListTables(const std::string& db_name) const override;
Result<bool> DatabaseExists(const std::string& db_name) const override;
Result<bool> TableExists(const Identifier& identifier) const override;
- std::string GetDatabaseLocation(const std::string& db_name) const override;
+ Result<std::string> GetDatabaseLocation(const std::string& db_name) const override;
Result<std::string> GetTableLocation(const Identifier& identifier) const override;
Result<std::shared_ptr<Schema>> 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 140f3f4..ed61d85 100644
--- a/src/paimon/rest/rest_catalog_test.cpp
+++ b/src/paimon/rest/rest_catalog_test.cpp
@@ -444,8 +444,14 @@
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);
+ // 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));
