-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix](storage) fix IOContext Use-After-Free #59947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
use shared_ptr instead of raw pointer to manage lifespan of ioctx in storage layer. Signed-off-by: zhengyu <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 32771 ms |
TPC-DS: Total hot run time: 177988 ms |
ClickBench: Total hot run time: 27.5 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Code Review: fix fix IOContext Use-After-FreeSummaryThis PR addresses a use-after-free crash by changing
Critical Issues1. Incomplete Fix - IOContext Contains Pointers (HIGH)The backward-compatible API copies // buffered_reader.cpp:853-856
if (io_ctx != nullptr) {
// Old API: best-effort safety by copying the IOContext onto the heap.
io_ctx_holder = std::make_shared<IOContext>(*io_ctx);
}However, // From io_common.h (inferred from internal_service.cpp changes)
io_ctx->file_cache_stats = &file_cache_statis; // raw pointer
io_ctx->file_reader_stats = &file_reader_stats; // raw pointerCopying IOContext copies these pointers, not the objects they point to. If the original Evidence: The auto file_cache_statis = std::make_shared<io::FileCacheStatistics>();
auto file_reader_stats = std::make_shared<io::FileReaderStats>();
io_ctx->file_cache_stats = file_cache_statis.get();
io_ctx->file_reader_stats = file_reader_stats.get();Recommendation:
2. Dual-Ownership Pattern is Error-Prone (MEDIUM)The pattern of keeping both std::shared_ptr<const IOContext> _io_ctx_holder;
const IOContext* _io_ctx = nullptr;With initialization like: _io_ctx_holder(std::move(io_ctx_holder)),
_io_ctx(_io_ctx_holder.get()),Risk: If Recommendation: Either:
3. Inconsistent Constructor Patterns (MEDIUM)The constructors have inconsistent handling: CsvReader: CsvReader(..., io::IOContext* io_ctx, std::shared_ptr<io::IOContext> io_ctx_holder = nullptr)
: ..., _io_ctx(io_ctx), _io_ctx_holder(std::move(io_ctx_holder)) {
if (_io_ctx == nullptr && _io_ctx_holder) {
_io_ctx = _io_ctx_holder.get();
}
}This allows callers to pass mismatched parameters (raw pointer to one object, shared_ptr to another). Recommendation: Use a single parameter - either always Code Quality Issues4. Code Duplication in
|
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
|
run NonConcurrent |
|
run nonConcurrent |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
dataroaring
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Use shared_ptr instead of a raw pointer to extend the lifespan of the ioctx until the readers are destroyed
to avoid use-after-free crash.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)