stinger1206 opened a new pull request, #67720:
URL: https://github.com/apache/doris/pull/67720
### What problem does this PR solve?
Issue Number: close #67719
Problem Summary:
`Segment::_footer_pb` is a non-atomic `std::weak_ptr<SegmentFooterPB>`.
`_get_segment_footer()` reads it via `lock()` and rebinds it
(`_footer_pb = footer_pb_shared`) from query/compaction threads whenever
the previously cached footer has expired (its `StoragePageCache` entry
pruned/erased), while `get_metadata_size()` calls `_footer_pb.lock()` from
the Daemon memory maintenance thread
(`MemoryProfile::refresh_memory_overview_profile` →
`MetadataAdder<Segment>::get_all_segments_estimate_size()`) without any
synchronization between the two.
Concurrent assignment to `_footer_pb` and `lock()` on the same non-atomic
`std::weak_ptr` object is a data race and therefore undefined behavior.
The exact failure mechanism is implementation-dependent; the observed
production crashes — SIGSEGV in `SegmentFooterPB::ByteSizeLong`
(`Segment::get_metadata_size`), in `~SegmentFooterPB` under
`LRUCache::prune_if`, and at unrelated crash sites — are consistent with
process-memory corruption caused by this undefined behavior. Every
observed crash is healed by a BE restart without repair and the implicated
segment files remain byte-identical, which argues against persistent
on-disk corruption.
The same unsynchronized pattern is present in both current `master` and
`branch-4.1`. Existing issue #64826 reports an overlapping
`SegmentFooterPB` crash symptom during page-cache capacity adjustment, but
does not report or identify the `Segment::_footer_pb` data race described
here.
Fix: add a dedicated `mutable std::mutex _footer_pb_lock` serializing every
`_footer_pb` access: the read in `get_metadata_size()`, and the read +
rebind in `_get_segment_footer()`. The mutex is `mutable` because
`get_metadata_size()` is `const`. The `shared_ptr` is copied under the lock
and all follow-up work (`ByteSizeLong()`, cache I/O) happens outside it, so
the critical sections are a few instructions on an uncontended mutex.
### Release note
Fix a data race on `Segment::_footer_pb` that could crash BE (SIGSEGV) when
segment footer cache expiry coincided with the memory maintenance thread.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] Unit Test
- [x] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
Manual test: standalone TSAN micro-repro of the exact pattern
(`weak_ptr` rebind vs `lock()` on 4+4 threads) reports the data race
without the fix; a production canary BE running the fix under the
previously-crashing load has shown no recurrence of the crash family.
A regression test is impractical: the race needs a cache expiry
window colliding with the maintenance thread's refresh, which unit
tests cannot hit reliably; TSAN on the full BE is not part of CI.
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
