From 6b8738ce710edd122703e004d673104507f8c986 Mon Sep 17 00:00:00 2001 From: chenBright Date: Sat, 22 Aug 2026 23:05:40 +0800 Subject: [PATCH] Harden bvar sampling against use-after-free --- src/brpc/input_messenger.cpp | 2 +- src/bvar/detail/percentile.h | 13 ++++ src/bvar/detail/sampler.cpp | 66 ++++++++++++++++-- src/bvar/detail/sampler.h | 119 ++++++++++++++++++++++++++++++--- src/bvar/latency_recorder.cpp | 19 ++++++ src/bvar/passive_status.h | 12 +++- src/bvar/recorder.h | 53 +++++++++++++-- src/bvar/reducer.h | 28 ++++++-- src/bvar/window.h | 56 +++++++++++----- test/bvar_sampler_unittest.cpp | 2 - test/bvar_window_unittest.cpp | 40 +++++++++++ 11 files changed, 365 insertions(+), 45 deletions(-) diff --git a/src/brpc/input_messenger.cpp b/src/brpc/input_messenger.cpp index 81154be134..26aac06378 100644 --- a/src/brpc/input_messenger.cpp +++ b/src/brpc/input_messenger.cpp @@ -218,7 +218,7 @@ int InputMessenger::ProcessNewMessage( << "Close " << *m << " due to unknown message: " << butil::ToPrintable(m->_read_buf); m->SetFailed(EINVAL, "Close %s due to unknown message", - m->description().c_str()); + m->description().c_str()); return -1; } else { LOG(WARNING) << "Close " << *m << ": " << pr.error_str(); diff --git a/src/bvar/detail/percentile.h b/src/bvar/detail/percentile.h index 430dcfeeea..85e681d997 100644 --- a/src/bvar/detail/percentile.h +++ b/src/bvar/detail/percentile.h @@ -523,10 +523,16 @@ class Percentile { } VoidOp inv_op() const { return VoidOp(); } + // Expose the shared data carrier, so that ReducerSampler holds it instead + // of `this`. Sampling then keeps reading valid memory even if this + // Percentile is destructed before the sampler is recycled. + shared_combiner_type share_combiner() const { return _combiner; } + // The sampler for windows over percentile. sampler_type* get_sampler() { if (nullptr == _sampler) { _sampler = new sampler_type(this); + _sampler->set_debug_name(_debug_name); _sampler->schedule(); } return _sampler; @@ -543,6 +549,9 @@ class Percentile { // This name is useful for warning negative latencies in operator<< void set_debug_name(const butil::StringPiece& name) { _debug_name.assign(name.data(), name.size()); + if (nullptr != _sampler) { + _sampler->set_debug_name(_debug_name); + } } private: @@ -591,6 +600,7 @@ class Percentile { sampler_type* get_sampler() { if (nullptr == _sampler) { _sampler = new sampler_type(this); + _sampler->set_debug_name(_debug_name); _sampler->schedule(); } return _sampler; @@ -610,6 +620,9 @@ class Percentile { // This name is useful for warning negative latencies in operator<< void set_debug_name(const butil::StringPiece& name) { _debug_name.assign(name.data(), name.size()); + if (nullptr != _sampler) { + _sampler->set_debug_name(_debug_name); + } } private: diff --git a/src/bvar/detail/sampler.cpp b/src/bvar/detail/sampler.cpp index 1b89a59080..65b1f5de16 100644 --- a/src/bvar/detail/sampler.cpp +++ b/src/bvar/detail/sampler.cpp @@ -166,9 +166,16 @@ void SamplerCollector::run() { Sampler* s = p->value(); s->_mutex.lock(); if (!s->_used) { + // If the sampler is still borrowed (a Window outlived the bvar + // it references), deleting it would leave the borrowers with a + // dangling pointer, so leak it on purpose. destroy() already + // reported the misuse. + const bool leaked = s->_leaked; s->_mutex.unlock(); p->RemoveFromList(); - delete s; + if (!leaked) { + delete s; + } } else { s->take_sample(); s->_mutex.unlock(); @@ -196,12 +203,17 @@ void SamplerCollector::run() { } } -Sampler::Sampler() : _used(true) {} +Sampler::Sampler() : _used(true), _nborrow(0), _leaked(false) {} Sampler::~Sampler() {} DEFINE_bool(bvar_enable_sampling, true, "is enable bvar sampling"); +DEFINE_bool(bvar_abort_on_sampler_still_borrowed, false, + "Abort when a bvar is destructed while its sampler is still " + "borrowed by a Window/PerSecond, namely the Window outlives the " + "bvar it references"); + void Sampler::schedule() { // since the SamplerCollector is initialized before the program starts // flags will not take effect if used in the SamplerCollector constructor @@ -211,9 +223,53 @@ void Sampler::schedule() { } void Sampler::destroy() { - _mutex.lock(); - _used = false; - _mutex.unlock(); + int nborrow = 0; + std::string owner; + { + BAIDU_SCOPED_LOCK(_mutex); + _used = false; + nborrow = _nborrow; + if (nborrow > 0) { + // The owning bvar is being destructed while Window/PerSecond objects + // still borrow this sampler. Leak the sampler so that the borrowers + // keep pointing at valid memory (they just stop getting new samples), + // which turns a use-after-free into a bounded leak. + _leaked = true; + if (!_debug_name.empty()) { + owner.append("bvar`").append(_debug_name).append("`"); + } else { + owner.append("An unnamed bvar"); + } + } + } + + if (nborrow <= 0) { + return; + } + if (FLAGS_bvar_abort_on_sampler_still_borrowed) { + LOG(FATAL) << "Abort because " << owner << " is destructed while " + << nborrow << " Window/PerSecond still reference its" + " sampler"; + } else { + LOG(ERROR) << owner << " is destructed while " << nborrow + << " Window/PerSecond still reference its sampler. The" + " bvar referenced by a Window MUST be destructed" + " AFTER that Window, see comments of Window in" + " bvar/window.h. The sampler is leaked to avoid a" + " dangling pointer."; + } +} + +void Sampler::add_borrower() { + BAIDU_SCOPED_LOCK(_mutex); + ++_nborrow; +} + +void Sampler::remove_borrower() { + BAIDU_SCOPED_LOCK(_mutex); + CHECK_GT(_nborrow, 0) << "remove_borrower() is called more times than " + "add_borrower(), which is a bug of the caller"; + --_nborrow; } } // namespace detail diff --git a/src/bvar/detail/sampler.h b/src/bvar/detail/sampler.h index c3d6082883..06533e4e0c 100644 --- a/src/bvar/detail/sampler.h +++ b/src/bvar/detail/sampler.h @@ -21,6 +21,9 @@ #define BVAR_DETAIL_SAMPLER_H #include +#include // std::string +#include // std::true_type +#include // std::declval #include "butil/containers/linked_list.h"// LinkNode #include "butil/scoped_lock.h" // BAIDU_SCOPED_LOCK #include "butil/logging.h" // LOG() @@ -57,14 +60,41 @@ class Sampler : public butil::LinkNode { // Call this function instead of delete to destroy the sampler. Deletion // of the sampler may be delayed for seconds. void destroy(); - + + // Declare/undeclare that an external object borrows this sampler which is + // owned by another bvar. Window/PerSecond does this because it samples + // through the sampler of the bvar it references. + // If the owner is destructed while borrowers remain (namely a Window + // outlives the bvar it references, which violates the contract documented + // in bvar/window.h), destroy() reports the misuse and the sampler is + // deliberately leaked so that borrowers are not left with a dangling + // pointer. + void add_borrower(); + void remove_borrower(); + + // Name of the owning bvar, purely for diagnostics. + void set_debug_name(const std::string& name) { + BAIDU_SCOPED_LOCK(_mutex); + _debug_name = name; + } + std::string debug_name() const { + BAIDU_SCOPED_LOCK(_mutex); + return _debug_name; + } + protected: virtual ~Sampler(); friend class SamplerCollector; bool _used; - // Sync destroy() and take_sample(). - butil::Mutex _mutex; + // Number of external borrowers, guarded by _mutex. + int _nborrow; + // Set by destroy() when _nborrow > 0, telling the sampling thread to leak + // this sampler instead of deleting it. Guarded by _mutex. + bool _leaked; + mutable butil::Mutex _mutex; + // For diagnostics only, see set_debug_name(). + std::string _debug_name; }; // Representing a non-existing operator so that we can test @@ -78,19 +108,90 @@ struct VoidOp { } }; +// Detects whether the host R exposes share_combiner(), namely whether its +// sampling data lives in a shared_ptr-managed carrier (an AgentCombiner) that +// the sampler is able to hold on its own. Hosts keeping the data elsewhere -- +// a user callback in PassiveStatus, or a value-type babylon counter -- do NOT +// provide it and are sampled through the host pointer as before. +template +class HasShareCombiner { + template + static auto probe(U* p) -> decltype(p->share_combiner(), std::true_type()); + static std::false_type probe(...); +public: + static const bool value = decltype(probe(std::declval()))::value; +}; + +// Samples through the host pointer, for hosts keeping their data outside a +// shared carrier (a user callback in PassiveStatus, a value-type babylon +// counter, ...). +template +class HostSampleSource { +public: + explicit HostSampleSource(R* host) + : _host(host), _op(host->op()), _inv_op(host->inv_op()) {} + + // Only reached from take_sample(), namely from the sampling thread, which is + // mutually exclusive with the host's destroy(). The host is therefore always + // alive here. + T reset() { return _host->reset(); } + T get_value() const { return _host->get_value(); } + + // Never touch the host, see the ctor. + const Op& op() const { return _op; } + const InvOp& inv_op() const { return _inv_op; } + +private: + R* _host; + Op _op; + InvOp _inv_op; +}; + +// Samples directly from the shared data carrier, so that sampling still reads +// valid memory even if the host is destructed before the sampler is recycled. +// `Op'/`InvOp' are stateless functors, thus copied by value at construction and +// the host is never touched afterwards. +template +class CombinerSampleSource { +public: + explicit CombinerSampleSource(R* host) + : _combiner(host->share_combiner()) + , _op(host->op()) + , _inv_op(host->inv_op()) {} + + T reset() { return _combiner->reset_all_agents(); } + T get_value() const { return _combiner->combine_agents(); } + const Op& op() const { return _op; } + const InvOp& inv_op() const { return _inv_op; } + +private: + typename R::shared_combiner_type _combiner; + Op _op; + InvOp _inv_op; +}; + // The sampler for reducer-alike variables. // The R should have following methods: // - T reset(); // - T get_value(); // - Op op(); // - InvOp inv_op(); +// Additionally, if R exposes +// - shared_combiner_type share_combiner(); +// the sampler holds that shared carrier instead of R itself, which makes +// sampling immune to R being destructed first. template class ReducerSampler : public Sampler { + typedef typename butil::conditional< + HasShareCombiner::value, + CombinerSampleSource, + HostSampleSource >::type source_type; + public: static const time_t MAX_SECONDS_LIMIT = 3600; explicit ReducerSampler(R* reducer) - : _reducer(reducer) + : _source(reducer) , _window_size(1) { // Invoked take_sample at begining so the value of the first second @@ -127,14 +228,14 @@ class ReducerSampler : public Sampler { // Suming up samples gives the result within a window. // In this case, get_value() of _reducer gives wrong answer and // should not be called. - latest.data = _reducer->reset(); + latest.data = _source.reset(); } else { // The operator can be inversed. // We save the result as a sample. // Inversed operation between latest and oldest sample within a // window gives result. // get_value() of _reducer can still be called. - latest.data = _reducer->get_value(); + latest.data = _source.get_value(); } latest.time_us = butil::cpuwide_time_us(); _q.elim_push(latest); @@ -164,12 +265,12 @@ class ReducerSampler : public Sampler { if (e == oldest) { break; } - _reducer->op()(result->data, e->data); + _source.op()(result->data, e->data); } } else { // Diff the latest and oldest sample within the window. result->data = latest->data; - _reducer->inv_op()(result->data, oldest->data); + _source.inv_op()(result->data, oldest->data); } result->time_us = latest->time_us - oldest->time_us; return true; @@ -212,7 +313,7 @@ class ReducerSampler : public Sampler { } private: - R* _reducer; + source_type _source; time_t _window_size; butil::BoundedQueue > _q; }; diff --git a/src/bvar/latency_recorder.cpp b/src/bvar/latency_recorder.cpp index 0bb4d5d827..5f1bd59720 100644 --- a/src/bvar/latency_recorder.cpp +++ b/src/bvar/latency_recorder.cpp @@ -24,6 +24,25 @@ namespace bvar { +#if !WITH_BABYLON_COUNTER +// Verify how ReducerSampler picks its data source, using the very hosts that +// LatencyRecorder is made of. +// Hosts keeping their data in a shared combiner are sampled through that carrier, +// so sampling reads valid memory even if the host is destructed before the sampler +// is recycled. +// PassiveStatus keeps its data in a user callback instead, hence it is still sampled +// through the host pointer. +static_assert(detail::HasShareCombiner::value, + "IntRecorder should be sampled through its shared combiner"); +static_assert(detail::HasShareCombiner::Base>::value, + "Reducer should be sampled through its shared combiner"); +static_assert(detail::HasShareCombiner::value, + "Percentile should be sampled through its shared combiner"); +static_assert(!detail::HasShareCombiner >::value, + "PassiveStatus has no shared carrier, it must keep being sampled" + " through the host pointer"); +#endif // !WITH_BABYLON_COUNTER + static bool valid_percentile(const char*, int32_t v) { return v > 0 && v < 100; } diff --git a/src/bvar/passive_status.h b/src/bvar/passive_status.h index aeecec0c91..7122e74113 100644 --- a/src/bvar/passive_status.h +++ b/src/bvar/passive_status.h @@ -143,6 +143,7 @@ class PassiveStatus : public Variable { sampler_type* get_sampler() { if (nullptr == _sampler) { _sampler = new sampler_type(this); + _sampler->set_debug_name(name()); _sampler->schedule(); } return _sampler; @@ -168,11 +169,16 @@ class PassiveStatus : public Variable { protected: int expose_impl(const butil::StringPiece& prefix, - const butil::StringPiece& name, + const butil::StringPiece& n, DisplayFilter display_filter) override { - const int rc = Variable::expose_impl(prefix, name, display_filter); + const int rc = Variable::expose_impl(prefix, n, display_filter); + if (rc != 0) { + return rc; + } + if (_sampler != nullptr) { + _sampler->set_debug_name(name()); + } if (ADDITIVE && - rc == 0 && _series_sampler == nullptr && FLAGS_save_series) { _series_sampler = new SeriesSampler(this); diff --git a/src/bvar/recorder.h b/src/bvar/recorder.h index d535ab67c2..147fcb72cf 100644 --- a/src/bvar/recorder.h +++ b/src/bvar/recorder.h @@ -164,6 +164,11 @@ class IntRecorder : public Variable { detail::AddStat op() const { return detail::AddStat(); } detail::MinusStat inv_op() const { return detail::MinusStat(); } + + // Expose the shared data carrier, so that ReducerSampler holds it instead + // of `this'. Sampling then keeps reading valid memory even if this + // IntRecorder is destructed before the sampler is recycled. + shared_combiner_type share_combiner() const { return _combiner; } void describe(std::ostream& os, bool /*quote_string*/) const override { os << get_value(); @@ -172,8 +177,9 @@ class IntRecorder : public Variable { bool valid() const { return _combiner->valid(); } sampler_type* get_sampler() { - if (nullptr == _sampler) { + if (_sampler == nullptr) { _sampler = new sampler_type(this); + _sampler->set_debug_name(diagnostic_name()); _sampler->schedule(); } return _sampler; @@ -183,9 +189,27 @@ class IntRecorder : public Variable { // IntRecorder is often used as the source of data and not exposed. void set_debug_name(const butil::StringPiece& name) { _debug_name.assign(name.data(), name.size()); + if (_sampler != nullptr) { + _sampler->set_debug_name(diagnostic_name()); + } } - + +protected: + int expose_impl(const butil::StringPiece& prefix, + const butil::StringPiece& name, + DisplayFilter display_filter) override { + const int rc = Variable::expose_impl(prefix, name, display_filter); + if (rc == 0 && _sampler != nullptr) { + _sampler->set_debug_name(diagnostic_name()); + } + return rc; + } + private: + const std::string& diagnostic_name() const { + return name().empty() ? _debug_name : name(); + } + // TODO: The following numeric functions should be independent utils static uint64_t _get_sum(const uint64_t n) { return (n & MAX_SUM_PER_THREAD); @@ -240,8 +264,8 @@ class IntRecorder : public Variable { private: shared_combiner_type _combiner; - sampler_type* _sampler; - std::string _debug_name; + sampler_type* _sampler; + std::string _debug_name; }; inline IntRecorder& IntRecorder::operator<<(int64_t sample) { @@ -372,6 +396,7 @@ class IntRecorder : public Variable { sampler_type* get_sampler() { if (nullptr == _sampler) { _sampler = new sampler_type(this); + _sampler->set_debug_name(diagnostic_name()); _sampler->schedule(); } return _sampler; @@ -381,8 +406,28 @@ class IntRecorder : public Variable { // IntRecorder is often used as the source of data and not exposed. void set_debug_name(const butil::StringPiece& name) { _debug_name.assign(name.data(), name.size()); + if (nullptr != _sampler) { + _sampler->set_debug_name(diagnostic_name()); + } } + +protected: + int expose_impl(const butil::StringPiece& prefix, + const butil::StringPiece& name, + DisplayFilter display_filter) override { + const int rc = Variable::expose_impl(prefix, name, display_filter); + if (rc == 0 && nullptr != _sampler) { + _sampler->set_debug_name(diagnostic_name()); + } + return rc; + } + private: + // See the non-babylon IntRecorder for details. + const std::string& diagnostic_name() const { + return name().empty() ? _debug_name : name(); + } + babylon::ConcurrentSummer _summer; sampler_type* _sampler{nullptr}; std::string _debug_name; diff --git a/src/bvar/reducer.h b/src/bvar/reducer.h index 943a2e7538..9fdb1960b7 100644 --- a/src/bvar/reducer.h +++ b/src/bvar/reducer.h @@ -86,6 +86,7 @@ class BabylonVariable: public Variable { sampler_type* get_sampler() { if (nullptr == _sampler) { _sampler = new sampler_type(this); + _sampler->set_debug_name(name()); _sampler->schedule(); } return _sampler; @@ -136,7 +137,13 @@ class BabylonVariable: public Variable { const butil::StringPiece& name, DisplayFilter display_filter) override { const int rc = Variable::expose_impl(prefix, name, display_filter); - if (rc == 0 && nullptr == _series_sampler && + if (rc != 0) { + return rc; + } + if (nullptr != _sampler) { + _sampler->set_debug_name(this->name()); + } + if (nullptr == _series_sampler && !butil::is_same::value && !butil::is_same::value && FLAGS_save_series) { @@ -255,10 +262,16 @@ class Reducer : public Variable { // Get instance of Op. const Op& op() const { return _combiner->op(); } const InvOp& inv_op() const { return _inv_op; } + + // Expose the shared data carrier, so that ReducerSampler holds it instead + // of `this'. Sampling then keeps reading valid memory even if this Reducer + // is destructed before the sampler is recycled by the sampling thread. + shared_combiner_type share_combiner() const { return _combiner; } sampler_type* get_sampler() { if (nullptr == _sampler) { _sampler = new sampler_type(this); + _sampler->set_debug_name(name()); _sampler->schedule(); } return _sampler; @@ -276,11 +289,16 @@ class Reducer : public Variable { protected: int expose_impl(const butil::StringPiece& prefix, - const butil::StringPiece& name, + const butil::StringPiece& n, DisplayFilter display_filter) override { - const int rc = Variable::expose_impl(prefix, name, display_filter); - if (rc == 0 && - _series_sampler == nullptr && + const int rc = Variable::expose_impl(prefix, n, display_filter); + if (rc != 0) { + return rc; + } + if (_sampler != nullptr) { + _sampler->set_debug_name(name()); + } + if (_series_sampler == nullptr && !butil::is_same::value && !butil::is_same::value && FLAGS_save_series) { diff --git a/src/bvar/window.h b/src/bvar/window.h index fcd29e1190..31c73b9853 100644 --- a/src/bvar/window.h +++ b/src/bvar/window.h @@ -22,6 +22,8 @@ #include // std::numeric_limits #include // round +#include // std::decay +#include // std::declval #include #include "butil/logging.h" // LOG #include "bvar/detail/sampler.h" @@ -45,19 +47,30 @@ class WindowBase : public Variable { typedef typename R::value_type value_type; typedef typename R::sampler_type sampler_type; - class SeriesSampler : public detail::Sampler { + // Type of the underlying var's operator, copied by value so that appending + // to the series never dereferences the var. + typedef typename std::decay().op())>::type var_op_type; + + class SeriesSampler : public Sampler { public: + // Holds a COPY of the underlying var's operator rather than a pointer to + // the var. The operators of bvar (AddTo/MaxTo/AddStat/...) are stateless + // functors, so copying is cheap and, more importantly, the sampling + // thread never touches the var -- which may already be destructed if the + // user let this Window outlive it. struct Op { - explicit Op(R* var) : _var(var) {} + explicit Op(const var_op_type& op) : _op(op) {} void operator()(value_type& v1, const value_type& v2) const { - _var->op()(v1, v2); + _op(v1, v2); } private: - R* _var; + var_op_type _op; }; - SeriesSampler(WindowBase* owner, R* var) - : _owner(owner), _series(Op(var)) {} - ~SeriesSampler() {} + + SeriesSampler(WindowBase* owner, const var_op_type& op) + : _owner(owner), _series(Op(op)) {} + ~SeriesSampler() override = default; + void take_sample() override { if (series_freq == SERIES_IN_SECOND) { // Get one-second window value for PerSecond<>, otherwise the @@ -73,35 +86,43 @@ class WindowBase : public Variable { void describe(std::ostream& os) { _series.describe(os, nullptr); } private: WindowBase* _owner; - detail::Series _series; + Series _series; }; WindowBase(R* var, time_t window_size) : _var(var) + , _var_op(var->op()) , _window_size(window_size > 0 ? window_size : FLAGS_bvar_dump_interval) , _sampler(var->get_sampler()) , _series_sampler(nullptr) { + // Tell the borrowed sampler about us, so that destructing `var` before + // this Window is detected and reported instead of silently leaving + // `_sampler' dangling. See Sampler::add_borrower(). + _sampler->add_borrower(); CHECK_EQ(0, _sampler->set_window_size(_window_size)); } - ~WindowBase() { + ~WindowBase() override { hide(); if (_series_sampler) { _series_sampler->destroy(); _series_sampler = nullptr; } + // Safe even if `var` was destructed first: in that case destroy() marked + // the sampler as leaked, so it was not deleted by the sampling thread. + _sampler->remove_borrower(); } - bool get_span(time_t window_size, detail::Sample* result) const { + bool get_span(time_t window_size, Sample* result) const { return _sampler->get_value(window_size, result); } - bool get_span(detail::Sample* result) const { + bool get_span(Sample* result) const { return get_span(_window_size, result); } virtual value_type get_value(time_t window_size) const { - detail::Sample tmp; + Sample tmp; if (get_span(window_size, &tmp)) { return tmp.data; } @@ -148,13 +169,17 @@ class WindowBase : public Variable { if (rc == 0 && _series_sampler == nullptr && FLAGS_save_series) { - _series_sampler = new SeriesSampler(this, _var); + _series_sampler = new SeriesSampler(this, _var_op); _series_sampler->schedule(); } return rc; } + // NOTE: `_var` is only dereferenced in the ctor (get_sampler()/op()). Do NOT + // dereference it afterwards: the user may have destructed it already if this + // Window outlives it (see the contract in comments of Window below). R* _var; + var_op_type _var_op; time_t _window_size; sampler_type* _sampler; SeriesSampler* _series_sampler; @@ -289,8 +314,7 @@ class WindowExAdapter : public Variable{ return *this; } - // Implement Variable::describe() - void describe(std::ostream& os, bool quote_string) const { + void describe(std::ostream& os, bool quote_string) const override { if (butil::is_same::value && quote_string) { os << '"' << get_value() << '"'; } else { @@ -298,7 +322,7 @@ class WindowExAdapter : public Variable{ } } - virtual ~WindowExAdapter() { + ~WindowExAdapter() override { hide(); } diff --git a/test/bvar_sampler_unittest.cpp b/test/bvar_sampler_unittest.cpp index 40cc22d997..bfa79eedf7 100644 --- a/test/bvar_sampler_unittest.cpp +++ b/test/bvar_sampler_unittest.cpp @@ -15,9 +15,7 @@ // specific language governing permissions and limitations // under the License. -#include //std::numeric_limits #include "bvar/detail/sampler.h" -#include "butil/time.h" #include "butil/logging.h" #include diff --git a/test/bvar_window_unittest.cpp b/test/bvar_window_unittest.cpp index e50cf10946..ace39bd3bb 100644 --- a/test/bvar_window_unittest.cpp +++ b/test/bvar_window_unittest.cpp @@ -23,6 +23,8 @@ #include #include #include +#include // logging::StringSink +#include // BUTIL_USE_ASAN #include #include #include "bvar/bvar.h" @@ -95,3 +97,41 @@ TEST_F(WindowTest, window) { ASSERT_EQ(recorder_stat.get_average_int(), window_ex_recorder_stat.get_average_int()); ASSERT_DOUBLE_EQ(recorder_stat.get_average_double(), window_ex_recorder_stat.get_average_double()); } + +// A Window/PerSecond outliving the bvar it references violates the contract +// documented in bvar/window.h. Before this was handled, the Window was left with +// a dangling sampler pointer (the sampling thread had already deleted it), which +// silently became a use-after-free. Now: +// - Sampler::destroy() notices it is still borrowed, reports the misuse and +// marks the sampler so that the sampling thread leaks it instead of deleting +// it, keeping the borrower's pointer valid (edge A); +// - the series sampler holds a COPY of the var's operator, so appending to the +// series never dereferences the destructed var (edge B). +// +// NOTE: this test leaks the sampler ON PURPOSE, hence it is skipped under ASan +// (which bundles LeakSanitizer) that would (correctly) report that leak. +#ifndef BUTIL_USE_ASAN +TEST_F(WindowTest, window_outliving_referenced_var) { + bvar::PerSecond >* ps = nullptr; + { + // Named so that the diagnostic below can identify the offending bvar. + bvar::Adder a("window_outliving_referenced_var_adder"); + a << 10; + ps = new bvar::PerSecond >(&a, 1); + // Expose it so that a series sampler is created as well, covering the + // path where the series operator would touch the var (edge B). + ASSERT_EQ(0, ps->expose("window_outliving_referenced_var")); + sleep(1); + } + // `a' is destructed while `ps' still borrows its sampler. Give the sampling + // thread a chance to walk the destroy branch. + sleep(2); + // Used to be a use-after-free; the sampler memory is still valid now, `ps' + // merely stops receiving new samples. + (void)ps->get_value(); + std::ostringstream os; + ps->describe(os, false); + // remove_borrower() on the leaked sampler is safe too. + delete ps; +} +#endif // BUTIL_USE_ASAN