github-actions[bot] commented on code in PR #68063:
URL: https://github.com/apache/doris/pull/68063#discussion_r4024428700
##########
be/src/service/http/action/check_encryption_action.cpp:
##########
@@ -68,7 +68,11 @@ Result<bool> is_tablet_encrypted(const BaseTabletSPtr&
tablet) {
rs_meta->end_version() == 1) {
return;
}
- auto fs = rs_meta->physical_fs();
+ // Must not be `physical_fs()`: packed rowsets keep their segments as
slices inside a
+ // shared object, and only the `PackedFileSystem` wrapper can resolve
a segment path to
+ // that slice. `fs()` is not an option either, since it decrypts and
would hide the
+ // encryption footer this check is looking for.
+ auto fs = rs_meta->packed_physical_fs();
Review Comment:
[P1] Keep the packed V2 index path alive through the scan
Once this packed segment open succeeds, V2-index rowsets reach the second
`file_paths` iteration. That entry is a `std::string_view` to the block-local
`inverted_index_file_path` created below; the owning string is destroyed before
the loop, so `open_file` copies from freed storage. The first remote segment
read can also reuse that buffer, causing the packed-map lookup to miss or
invoking undefined behavior. Please keep owning strings through the loop (and
cover a packed V2 segment plus index in the test).
##########
be/src/service/http/action/check_encryption_action.cpp:
##########
@@ -137,12 +141,18 @@ Result<std::string> get_last_encrypt_footer(const
BaseTabletSPtr& tablet) {
if (config::is_cloud_mode() && rs_meta->start_version() == 0 &&
rs_meta->end_version() == 1) {
return "{}";
}
- auto fs = rs_meta->physical_fs();
+ // See the comment in `is_tablet_encrypted()` for why this is neither
`physical_fs()`
+ // nor `fs()`.
+ auto fs = rs_meta->packed_physical_fs();
Review Comment:
[P1] Reject truncated packed slices before trailer-relative reads
With this filesystem the reader size is the logical slice size, but the
footer code later computes `size() - 256 + ...` without first requiring a
256-byte slice (and the magic path similarly subtracts 8). On
`PackedFileReader`, that unsigned underflow is translated with the slice's
physical offset and can wrap into bytes before the slice; the existing
beyond-boundary reader test explicitly permits this behavior. A
truncated/corrupt packed segment can therefore decode a neighboring slice
instead of returning corruption. Please validate the minimum layout and exact
`bytes_read` values before either subtraction (and ideally reject out-of-range
offsets in `PackedFileReader`).
##########
be/src/service/http/action/check_encryption_action.cpp:
##########
@@ -137,12 +141,18 @@ Result<std::string> get_last_encrypt_footer(const
BaseTabletSPtr& tablet) {
if (config::is_cloud_mode() && rs_meta->start_version() == 0 &&
rs_meta->end_version() == 1) {
return "{}";
}
- auto fs = rs_meta->physical_fs();
+ // See the comment in `is_tablet_encrypted()` for why this is neither
`physical_fs()`
+ // nor `fs()`.
+ auto fs = rs_meta->packed_physical_fs();
+ if (fs == nullptr) {
+ return ResultError(Status::InternalError("failed to get fs for rowset:
tablet={}, rs={}",
+ tablet->tablet_id(),
rs->rowset_id().to_string()));
+ }
io::FileReaderSPtr reader;
RETURN_IF_ERROR_RESULT(fs->open_file(maybe_seg_path.value(), &reader));
std::vector<uint8_t> pb_len_buf;
- pb_len_buf.reserve(sizeof(uint64_t));
+ pb_len_buf.resize(sizeof(uint64_t));
Review Comment:
[P1] Bound the decoded footer length before allocating
Resizing this buffer fixes the immediate out-of-bounds write, but the
decoded on-disk `uint64_t` is then passed directly to `info_pb_buf.resize()`
and later narrowed to protobuf's `int` length. `get_footer=true` also reaches
this parser after the encryption scan returns `false`, so a plaintext/mixed or
corrupt latest file can turn arbitrary bytes into a huge allocation instead of
a corruption response. Please bound the length to the payload available in the
fixed 256-byte footer (and `INT_MAX`) and require an exact payload read before
allocating/parsing.
--
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]
