Doris-Breakwater commented on issue #69863: URL: https://github.com/apache/doris/issues/69863#issuecomment-6503332702
Breakwater-GitHub-Analysis-Slot: slot_b3ac28a4b50b **Assessment:** The reported slice-boundary defect is supported by source inspection. It is reproducible in principle through a direct/uncached `PackedFileReader` read; this does not yet establish that a production query returned incorrect results. The issue currently has no labels or assignee. Suggested routing: BE storage / packed-file I/O correctness. **Verified evidence** - Inspected release tag `4.1.4` (`ad35a140c7fd0b842f18c23300bac581f7d04326`) and current master (`2a4191c1dc94e789e1725d6cb345f46c31a17272`). Both contain the same unguarded subtraction and translated read in [`PackedFileReader::read_at_impl`](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/io/fs/packed_file_reader.cpp#L44-L71). On the 64-bit target, subtracting a larger `size_t offset` from the signed slice size wraps in unsigned arithmetic. Casting the result afterward cannot repair that. The method then reads at `_packed_file_offset + offset`, where the underlying reader knows the packed object's boundary, not this slice's boundary. - The 4.1.4 [`FileReader::read_at`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/fs/file_reader.cpp#L33-L41) wrapper adds no bounds check. [`S3FileReader`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/fs/s3_file_reader.cpp#L110-L130) checks the physical file size, so it can legitimately return bytes beyond the logical slice if the translated range still lies inside the packed object. This is a wrong-range read; a successful underlying read does not prove those bytes belong to the requested logical file. - There is already a test that accepts this behavior: [`ReadBeyondFileBoundary`](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/test/io/fs/packed_file_reader_test.cpp#L176-L195) expects success for a 20-byte slice starting at physical offset 10, read at logical offset 25. With the existing 36-byte mock content, the code would return the final `z`, outside the slice. The test only asserts that the byte count does not exceed the buffer size. It should become a regression test requiring an error. **Scope qualification** In 4.1.4, [`PackedFileSystem::open_file_impl`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/fs/packed_file_system.cpp#L126-L159) returns either the packed reader directly or an outer cache wrapper. [`CachedRemoteFileReader::read_at_impl`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/cache/cached_remote_file_reader.cpp#L1182-L1203) rejects `offset > size()` before forwarding, with size taken from the wrapped logical reader. Thus an ordinary read through that wrapper, with a valid nonnegative slice size, is already protected against this specific invalid starting offset. Do not infer that every packed-file query is exposed. The reason an upstream caller supplied an invalid offset remains unverified; incorrect metadata is one hypothesis, not an established root cause. **Suggested next steps** 1. Proceed with the proposed focused reader fix: reject `_file_size < 0` or `offset > static_cast<size_t>(_file_size)` before translating the offset or subtracting, then compute the remaining length using the validated unsigned size. `Status::Corruption` is reasonable for this diagnostic, subject to maintainer agreement on the error contract. Include logical path, requested offset, slice size, packed-file start offset, and ideally the underlying packed path. 2. Preserve valid behavior: `offset == size` returns zero bytes successfully; a request starting inside the slice is truncated at its end. Add coverage for a negative size, an empty slice, a very large offset, and invalid offsets with empty buffers. For rejected reads, assert that the inner reader was not called and the destination buffer was untouched; define and test the `bytes_read` error behavior explicitly. Use distinct neighboring-slice bytes and keep the translated request within the physical object's size so physical EOF cannot mask the defect. 3. Run the focused packed-reader tests and applicable packed-filesystem/cache tests, then consider a 4.1 backport after confirming branch applicability. **Missing information / validation limits:** No additional production logs are required to justify the local boundary check. For production-impact attribution, please provide the exact BE build commit, a minimal reproducer or failing test, cache policy and actual reader chain, requested offset/length, slice start/size and packed-object size, plus the caller stack and relevant BE error logs if this occurred in a query. Redact sensitive paths/data. A query profile is not needed for this arithmetic defect. This assessment is based on source and existing-test inspection; I have not run the BE tests or a live-cluster reproduction, and no code was modified. -- 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]
