zjw1111 commented on code in PR #264:
URL: https://github.com/apache/paimon-cpp/pull/264#discussion_r3893948053
##########
src/paimon/core/catalog/catalog_utils.cpp:
##########
@@ -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<unsigned char>(c)) != 0;
Review Comment:
`StringUtils::IsNullOrWhitespaceOnly` and `std::iscntrl` classify individual
UTF-8 bytes under the current C locale, while the referenced Rust
implementation uses Unicode-aware `trim` and `char::is_control`. Under the
typical C locale, a name consisting of U+2003 EM SPACE or containing U+0085 is
accepted here but rejected by Rust. C++ can therefore create catalog objects
that Rust cannot operate on, and the result also depends on process locale.
##########
src/paimon/core/catalog/catalog_utils.cpp:
##########
@@ -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<unsigned char>(c)) != 0;
+ })) {
+ reason = "cannot contain control characters";
+ }
+ if (reason != nullptr) {
+ return Status::Invalid(fmt::format("{} name {}: '{}'", kind, reason,
name));
Review Comment:
The rejected `name` is formatted verbatim even when it contains control
characters. A newline remains in the returned `Status` and can inject
additional log lines when the status is propagated, while an embedded NUL can
truncate downstream C-string output. The referenced Rust implementation escapes
these characters through debug formatting.
##########
src/paimon/core/catalog/catalog_utils.cpp:
##########
@@ -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<std::string> branch,
identifier.GetBranchName());
+ if (branch) {
+ PAIMON_RETURN_NOT_OK(CheckValidIdentifierName("branch",
branch.value()));
Review Comment:
This path sends an identifier branch through the generic validator, so a
whitespace-only branch is rejected. `CheckValidBranchName` below and the
existing `BranchManager::NormalizeBranch` both treat a whitespace-only branch
as the main branch. Consequently, `ListSnapshots(id, " ")` is accepted while
the equivalent `t$branch_ ` identifier is rejected, producing inconsistent
branch semantics between entry points.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
