OIiveirra commented on code in PR #69527:
URL: https://github.com/apache/doris/pull/69527#discussion_r4366035684
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,89 @@ const std::shared_ptr<LocalFileSystem>&
global_local_filesystem() {
Status LocalFileSystem::canonicalize_local_file(const std::string& dir,
const std::string& file_path,
std::string* full_path) {
- const std::string absolute_path = dir + "/" + file_path;
+ // An empty root would turn a relative request into an unrestricted
absolute path.
+ if (dir.empty()) {
+ return Status::InvalidArgument("user_files_secure_path must not be
empty");
+ }
+ // Reject embedded NULs before passing paths to filesystem APIs that use C
strings.
+ if (file_path.empty() || file_path.find('\0') != std::string::npos) {
+ return Status::InvalidArgument("local file path must not be empty or
contain NUL");
+ }
+ Path local_path;
+ // Convert file: URIs to filesystem paths and reject remote schemes before
any reader opens them.
+ RETURN_IF_ERROR(convert_to_abs_path(file_path, local_path));
+ if (file_path.rfind("file:", 0) == 0) {
+ // file:a%20b names "a b", but a bare path containing "%20" must stay
literal.
+ // Decode once, preserve '+' and invalid escapes, matching Lance's
file provider.
+ auto encoded = local_path.string();
+ // Query and fragment are URI components, not part of the filesystem
path.
+ encoded = encoded.substr(0, encoded.find_first_of("?#"));
+ const auto hex_digit = [](char c) -> int {
+ if (c >= '0' && c <= '9') return c - '0';
+ if (c >= 'a' && c <= 'f') return c - 'a' + 10;
+ if (c >= 'A' && c <= 'F') return c - 'A' + 10;
+ return -1;
+ };
+ std::string decoded;
+ decoded.reserve(encoded.size());
+ for (size_t i = 0; i < encoded.size(); ++i) {
+ if (encoded[i] == '%' && i + 2 < encoded.size()) {
+ const int high = hex_digit(encoded[i + 1]);
+ const int low = hex_digit(encoded[i + 2]);
+ if (high >= 0 && low >= 0) {
+ decoded += static_cast<char>((high << 4) | low);
+ i += 2;
+ continue;
+ }
+ }
+ decoded += encoded[i];
+ }
+ // %00 must not truncate the path when canonicalization reaches C
filesystem APIs.
+ if (decoded.find('\0') != std::string::npos) {
+ return Status::InvalidArgument("local file path must not contain
NUL");
+ }
+ local_path = std::move(decoded);
+ }
+ std::string secure_path;
+ RETURN_IF_ERROR(canonicalize(dir, &secure_path));
std::string canonical_path;
- RETURN_IF_ERROR(canonicalize(absolute_path, &canonical_path));
- if (!contain_path(dir, canonical_path)) {
- return Status::InvalidArgument("file path is not allowed: {}",
canonical_path);
+ const auto status = canonicalize(Path(secure_path) / local_path,
&canonical_path);
Review Comment:
Fixed in 33cbacc8078293f3e63b931f472e4841035dd4b1. Actual file URI dot
segments, including encoded-dot forms, are normalized before percent decoding
and before filesystem/symlink resolution. Empty URL segments and encoded
separator ordering are preserved; bare paths keep their filesystem semantics.
Canonical secure-root containment still runs afterward.
The new regression creates two distinguishable datasets (`root_only`/row 7
versus `nested_only`/row 42) and verifies schema, canonical dataset identity
and actual scans. Before the fix it reproduced the wrong schema and row 42;
afterward all 13 selected BE filesystem/Lance tests passed. Coverage includes
missing intermediate directories, encoded dots/slashes, case-insensitive
schemes, raw URI separators and bare-path controls.
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,89 @@ const std::shared_ptr<LocalFileSystem>&
global_local_filesystem() {
Status LocalFileSystem::canonicalize_local_file(const std::string& dir,
const std::string& file_path,
std::string* full_path) {
- const std::string absolute_path = dir + "/" + file_path;
+ // An empty root would turn a relative request into an unrestricted
absolute path.
+ if (dir.empty()) {
+ return Status::InvalidArgument("user_files_secure_path must not be
empty");
+ }
+ // Reject embedded NULs before passing paths to filesystem APIs that use C
strings.
+ if (file_path.empty() || file_path.find('\0') != std::string::npos) {
+ return Status::InvalidArgument("local file path must not be empty or
contain NUL");
+ }
+ Path local_path;
+ // Convert file: URIs to filesystem paths and reject remote schemes before
any reader opens them.
+ RETURN_IF_ERROR(convert_to_abs_path(file_path, local_path));
Review Comment:
Fixed in 33cbacc8078293f3e63b931f472e4841035dd4b1. Actual `file:` URI
schemes are now recognized case-insensitively before conversion, and the same
classification controls URI-only normalization/decoding. Bare path spelling and
the shared `convert_to_abs_path` implementation are preserved.
The new filesystem and real Lance tests cover `FILE:` and mixed-case `fIlE:`
input, including encoded paths, schema discovery and actual scans. All 13
selected BE filesystem/Lance tests passed under Clang21/ASAN with fresh XML,
zero failures/skips and unchanged source.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
