github-actions[bot] commented on code in PR #69527:
URL: https://github.com/apache/doris/pull/69527#discussion_r4366199150
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,158 @@ 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");
+ }
+ // Match URL preprocessing only for actual file URIs; bare paths keep
literal whitespace.
+ const auto is_c0_or_space = [](unsigned char c) { return c <= 0x20; };
+ size_t begin = 0;
+ size_t end = file_path.size();
+ while (begin < end && is_c0_or_space(file_path[begin])) {
+ ++begin;
+ }
+ while (end > begin && is_c0_or_space(file_path[end - 1])) {
+ --end;
+ }
+ std::string uri_input;
+ for (size_t i = begin; i < end; ++i) {
+ if (file_path[i] != '\t' && file_path[i] != '\r' && file_path[i] !=
'\n') {
+ uri_input += file_path[i];
+ }
+ }
+ const bool is_file_uri = uri_input.size() >= 5 &&
+ (uri_input[0] == 'f' || uri_input[0] == 'F') &&
+ (uri_input[1] == 'i' || uri_input[1] == 'I') &&
+ (uri_input[2] == 'l' || uri_input[2] == 'L') &&
+ (uri_input[3] == 'e' || uri_input[3] == 'E') &&
uri_input[4] == ':';
+ std::string local_input = is_file_uri ? std::move(uri_input) : file_path;
+ if (is_file_uri) {
+ local_input.replace(0, 4, "file");
+ // File URLs treat raw backslashes as separators, including before the
authority.
+ // Do this before conversion; percent-encoded backslashes must remain
literal.
+ for (size_t i = 5; i < local_input.size(); ++i) {
+ if (local_input[i] == '\\') {
+ local_input[i] = '/';
+ }
+ }
+ }
+ 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(local_input, local_path));
Review Comment:
[P3] Preserve drive components when converting file URLs
An enabled, authorized LOCAL Lance read can use a valid Unix secure
directory `/C:` containing `/C:/data.lance`. For
`file_path="file://C:/data.lance"`, the [pinned URL parser treats `C:` as a
path
component](https://github.com/servo/rust-url/blob/v2.5.8/url/src/parser.rs#L1036-L1064),
and its [Unix conversion preserves
`/C:/data.lance`](https://github.com/servo/rust-url/blob/v2.5.8/url/src/lib.rs#L2934-L2980).
`convert_to_abs_path` instead strips `C:` as an authority, yielding
`/data.lance`, so the new validation rejects this valid in-root dataset. With
secure root `/` and both datasets present, it silently reads the other dataset.
Preserve SDK-compatible drive parsing before containment, and cover this case
separately from rootless `file:path` URLs.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java:
##########
@@ -662,6 +663,11 @@ private Plan
bindTVFTableSink(MatchingContext<UnboundTVFTableSink<Plan>> ctx) {
"INSERT INTO TVF only supports local/s3/hdfs, but got: " +
tvfName);
}
+ // INSERT/EXPLAIN INSERT bind a sink without constructing the LOCAL
read TVF.
+ if (tvfName.equals("local")) {
Review Comment:
[P1] Gate local JNI writers behind remote sink names
With the default `enable_local_tvf=false`, a user with global LOAD can still
execute `INSERT INTO
s3('file_path'='/tmp/local_gate_bypass.csv','format'='csv','writer_class'='org/apache/doris/writer/LocalFileJniWriter')
SELECT 1` (likewise `hdfs`). This check only gates the SQL name.
`TVFTableSink` accepts those properties and selects JNI; `VTVFTableWriter`
skips `FileFactory`, and the bundled `LocalFileJniWriter.open()` calls `new
FileWriter` on the original path, creating or truncating a BE-local file
despite the disabled switch. Apply admission to the effective local writer
regardless of the sink name, and add a disabled-mode negative test. Requiring
remote storage properties alone is insufficient because the same JNI class can
be selected alongside genuine S3 properties.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -35,11 +35,50 @@
#include "format_v2/lance/lance_reader_helper.h"
#include "format_v2/lance/lance_runtime_filter_helper.h"
#include "format_v2/lance/lance_session_manager.h"
+#include "io/fs/local_file_system.h"
#include "runtime/file_scan_profile.h"
#include "runtime/runtime_state.h"
namespace doris::format::lance {
+namespace {
+Status resolve_dataset_uri(const TFileRangeDesc& range, const
TFileScanRangeParams& scan_params,
+ std::string* uri) {
+ const auto& requested_uri =
range.table_format_params.lance_params.dataset_uri;
+ // Modern plans tag each range; older plans put the type in the scan
parameters.
+ // Untagged catalog scans retain their original storage-provider selection.
+ const bool is_local = range.__isset.file_type
+ ? range.file_type == TFileType::FILE_LOCAL
+ : scan_params.__isset.file_type &&
+ scan_params.file_type ==
TFileType::FILE_LOCAL;
+ // LOCAL TVF uses version zero for both schema discovery and whole-dataset
scans.
+ // Catalog scans pin a positive snapshot version, including file://
catalogs whose
+ // operator-configured warehouse is independent of the LOCAL TVF secure
directory.
+ if (is_local && range.table_format_params.lance_params.version == 0) {
+ // LOCAL Lance bypasses glob because it opens a dataset directory.
Apply the same
+ // canonical secure-root check before either schema discovery or a
cached dataset open.
+ std::string local_input = requested_uri;
+ const auto colon = requested_uri.find(':');
+ const auto slash = requested_uri.find('/');
+ const bool bare_path =
+ colon == std::string::npos || (slash != std::string::npos &&
slash < colon);
+ if (bare_path && !requested_uri.empty() && requested_uri.find('\0') ==
std::string::npos &&
+ !config::user_files_secure_path.empty()) {
+ // Lance's PathAbs fallback resolves bare dot segments before
following symlinks.
+ // Anchor relative TVF paths to the canonical secure root, not the
process cwd.
+ std::string secure_root;
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize(
+ config::user_files_secure_path, &secure_root));
+ local_input = (io::Path(secure_root) /
requested_uri).lexically_normal().string();
Review Comment:
[P2] Expand Lance home shorthand before anchoring the path
For an enabled, authorized LOCAL Lance read, let the selected BE's home and
`user_files_secure_path` both be `/home/doris`, with a dataset at
`/home/doris/data.lance`. FE preserves `file_path="~/data.lance"`, and the
[pinned SDK expands `~/` before PathAbs
normalization](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance-io/src/object_store.rs#L504-L538).
This join instead produces `/home/doris/~/data.lance`; both schema discovery
and scanning now reject the valid in-root dataset, or silently read another
dataset if that literal subdirectory exists. Preserve the SDK's `~`/`~/`
expansion before anchoring and canonical containment, while continuing to
reject expanded paths outside the secure root. Add distinguishable-dataset
coverage for this bare-path form.
--
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]
