chenBright opened a new pull request, #3487:
URL: https://github.com/apache/brpc/pull/3487
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
bvar sampling has two lifetime hazards, both of which end up as a silent
use-after-free:
1. `detail::ReducerSampler` holds a raw `R* _reducer` and dereferences it in
`take_sample()` and `get_value()`. Safety therefore relies entirely on
every
host correctly calling `Sampler::destroy()` from its dtor, which is a
convention,
not a structural guarantee.
2. `Window`/`PerSecond` borrow the sampler of the bvar they reference
(`var->get_sampler()`), and `WindowBase::SeriesSampler::Op` holds a raw
`R* _var`. If a Window outlives that bvar (violating the contract
documented
in `bvar/window.h`), the sampling thread has already `delete`d the
sampler, so
the Window is left with a permanently dangling pointer: both
`get_value()` and
the series sampler become a use-after-free, and nothing reports the
misuse.
### What is changed and the side effects?
Changed:
1. Sample through the shared data carrier where possible.
`ReducerSampler` now selects its data source with a trait: hosts exposing
`share_combiner()` are sampled through their `shared_ptr<AgentCombiner>` plus
by-value copies of `Op`/`InvOp`, so the sampler never dereferences the host
after
construction. Since the sampler keeps a reference to the combiner, sampling
reads
valid memory even if the host is destructed before the sampler is recycled.
`share_combiner()` is added to `Reducer` (Adder/Maxer/Miner), `IntRecorder`
and
`Percentile` (the one inside `LatencyRecorder`). Hosts without such a
carrier --
`PassiveStatus` (data lives in a user callback) and the babylon variants
(value
types) -- keep the previous host-pointer mode, so their behaviour is
unchanged.
2. Detect a Window outliving its bvar, and degrade UAF to a bounded leak.
`Sampler` gains a borrower counter (guarded by its existing `_mutex`) with
`add_borrower()`/`remove_borrower()`, called by `WindowBase`'s ctor/dtor. If
`destroy()` finds the sampler still borrowed, it reports the misuse
(including
the bvar name) and marks the sampler, and the sampling thread then skips the
`delete`, leaking it on purpose. The borrowers keep pointing at valid memory
and merely stop receiving new samples. Note the counter and the "don't
delete"
part are inseparable: otherwise `remove_borrower()` itself would be a
use-after-free. A new gflag `bvar_abort_on_sampler_still_borrowed` (default
`false`, i.e. `LOG(ERROR)`) can escalate this to an abort, mirroring the
existing
`bvar_abort_on_same_name`.
**3. Stop touching the var from the series sampler.**
`WindowBase::SeriesSampler::Op` used to hold `R* _var` and call `_var->op()`
from
the sampling thread. It now holds a copy of the operator (bvar operators
such as
`AddTo`/`MaxTo`/`AddStat` are stateless functors), copied once into
`WindowBase::_var_op`
at construction. As a result `_var` is only dereferenced in the ctor and
never afterwards.
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of
Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
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]
