apache / apache/doris

[Bug] BE SIGSEGV caused by data race on Segment::_footer_pb

Open
#67,719 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
15.9k
Forks
3.9k
Avg merge
2d 23h
Merged PRs (30d)
520

Description

## Version

4.1.3 (branch-4.1; local build based on Doris commit 13a108d04b)

## What's Wrong?

Long-running BE nodes crash intermittently with SIGSEGV under concurrent query/compaction load. Observed innermost frames:

- `SegmentFooterPB::ByteSizeLong` ← `Segment::get_metadata_size` (reader side)
- `~SegmentFooterPB` ← `LRUCache::prune_if` (release side)

plus crashes at unrelated, seemingly random sites. Every observed crash is healed by a BE restart without repair, and the implicated segment files remain byte-identical. This argues against persistent on-disk corruption and is consistent with process-memory corruption. Full `be.out` stacks from the incident can be attached in a comment if needed.

Root cause is a data race on `std::weak_ptr Segment::_footer_pb` (`be/src/storage/segment/segment.h`):

- **Writer** — `Segment::_get_segment_footer()` (`be/src/storage/segment/segment.cpp`) rebinds the weak_ptr (`_footer_pb = footer_pb_shared`) from query/compaction threads whenever the previously cached footer has expired (its `StoragePageCache` entry pruned/erased). It also reads the weak_ptr via `_footer_pb.lock()`.
- **Reader** — `Segment::get_metadata_size()` calls `_footer_pb.lock()` from the Daemon `memory_maintenance_thread` (`MemoryProfile::refresh_memory_overview_profile` → `MetadataAdder::get_all_segments_estimate_size()`), with no synchronization against the writers.

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 crashes are consistent with process-memory corruption caused by this undefined behavior.

The same unsynchronized pattern is present in both current `master` and `branch-4.1`.

## What You Expected?

Concurrent footer-cache expiry (query/compaction) and periodic memory-maintenance size estimation should be safe; BE should not crash.

## How to Reproduce?

Hard to reproduce via SQL alone: it requires segment footer cache entries expiring while the memory maintenance thread's refresh runs (a matter of timing under sustained load). It reproduces intermittently in production on long-running BEs under mixed query + compaction traffic.

The exact C++ pattern reproduces reliably under ThreadSanitizer with a standalone, dependency-free program that mimics the two access paths (`_get_segment_footer` rebind vs `get_metadata_size` lock):

```cpp
// repro_weakptr_race.cpp — micro-repro of the Segment::_footer_pb pattern
// clang++ -std=c++17 -O1 -g -fsanitize=thread repro_weakptr_race.cpp -o repro_tsan && ./repro_tsan
#include
#include
#include
#include
#include

struct FooterPB {
char payload[64];
size_t ByteSizeLong() const { return sizeof(FooterPB); }
};

std::weak_ptr g_footer_pb; // == Segment::_footer_pb

// "Segment::_get_segment_footer" writer (query / compaction thread):
// parse footer, insert into footer cache, rebind the weak_ptr.
void writer_loop() {
uint64_t n = 0;
while (true) {
auto footer = std::make_shared();
footer->payload[0] = static_cast(n++ & 0xff);
g_footer_pb = footer; // unlocked rebind
std::this_thread::sleep_for(std::chrono::microseconds(50));
}
}

// "Segment::get_metadata_size" reader (Daemon memory_maintenance_thread):
void reader_loop() {
while (true) {
auto footer = g_footer_pb.lock(); // unlocked lock()
if (footer) {
volatile size_t sz = footer->ByteSizeLong();
(void)sz;
}
std::this_thread::sleep_for(std::chrono::microseconds(50));
}
}

int main() {
std::vector ts;
for (int i = 0; i < 4; i++) ts.emplace_back(writer_loop);
for (int i = 0; i < 4; i++) ts.emplace_back(reader_loop);
for (auto& t : ts) t.join(); // never reached; Ctrl-C after the first TSAN report
return 0;
}
```

In our test TSAN reports the race within seconds, with one access in `std::weak_ptr::operator=` and the other in `std::weak_ptr::lock`.

## Anything Else?

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: serialize every `_footer_pb` access with a dedicated `mutable std::mutex _footer_pb_lock` in `Segment` — read in `get_metadata_size()`, 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 `ByteSizeLong()` runs outside it, so the critical sections are a few instructions. No behavior change. Validated on a production canary BE (crash family absent after deploy; previously recurring).

- [x] I had searched in the issues and found no similar issues.
- [x] Yes I am willing to submit a PR!
- [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct).

Contributor guide

Open the contributing guide

Research direction

Read be/src/storage/segment/segment.h and segment.cpp, focusing on Segment::_get_segment_footer() and get_metadata_size(), then review the provided ThreadSanitizer reproducer. Done means all _footer_pb accesses are synchronized while footer size calculation remains safe, with no race reported for the concurrent cache-expiry and metadata-estimation paths.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
database
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.