This is an automated email from the ASF dual-hosted git repository.
wwbmmm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 98f42bcb Harden bvar sampling against use-after-free (#3487)
98f42bcb is described below
commit 98f42bcb572c2d6a64d656a342e4708462f08b9f
Author: Bright Chen <[email protected]>
AuthorDate: Tue Aug 25 16:05:43 2026 +0800
Harden bvar sampling against use-after-free (#3487)
---
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 81154be1..26aac063 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 430dcfee..85e681d9 100644
--- a/src/bvar/detail/percentile.h
+++ b/src/bvar/detail/percentile.h
@@ -523,10 +523,16 @@ public:
}
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 @@ public:
// 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 @@ public:
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 @@ public:
// 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 1b89a590..65b1f5de 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 c3d60828..06533e4e 100644
--- a/src/bvar/detail/sampler.h
+++ b/src/bvar/detail/sampler.h
@@ -21,6 +21,9 @@
#define BVAR_DETAIL_SAMPLER_H
#include <vector>
+#include <string> // std::string
+#include <type_traits> // std::true_type
+#include <utility> // 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 @@ public:
// 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 <typename R>
+class HasShareCombiner {
+ template <typename U>
+ 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<R*>()))::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 <typename R, typename T, typename Op, typename InvOp>
+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 <typename R, typename T, typename Op, typename InvOp>
+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 <typename R, typename T, typename Op, typename InvOp>
class ReducerSampler : public Sampler {
+ typedef typename butil::conditional<
+ HasShareCombiner<R>::value,
+ CombinerSampleSource<R, T, Op, InvOp>,
+ HostSampleSource<R, T, Op, InvOp> >::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 @@ public:
// 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 @@ public:
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 @@ public:
}
private:
- R* _reducer;
+ source_type _source;
time_t _window_size;
butil::BoundedQueue<Sample<T> > _q;
};
diff --git a/src/bvar/latency_recorder.cpp b/src/bvar/latency_recorder.cpp
index 0bb4d5d8..5f1bd597 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<IntRecorder>::value,
+ "IntRecorder should be sampled through its shared combiner");
+static_assert(detail::HasShareCombiner<Maxer<int64_t>::Base>::value,
+ "Reducer should be sampled through its shared combiner");
+static_assert(detail::HasShareCombiner<detail::Percentile>::value,
+ "Percentile should be sampled through its shared combiner");
+static_assert(!detail::HasShareCombiner<PassiveStatus<int64_t> >::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 aeecec0c..7122e741 100644
--- a/src/bvar/passive_status.h
+++ b/src/bvar/passive_status.h
@@ -143,6 +143,7 @@ public:
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 @@ public:
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 d535ab67..147fcb72 100644
--- a/src/bvar/recorder.h
+++ b/src/bvar/recorder.h
@@ -164,6 +164,11 @@ public:
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 @@ public:
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 @@ public:
// 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 @@ private:
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 @@ public:
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 @@ public:
// 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 943a2e75..9fdb1960 100644
--- a/src/bvar/reducer.h
+++ b/src/bvar/reducer.h
@@ -86,6 +86,7 @@ public:
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 @@ protected:
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<InvOp, VoidOp>::value &&
!butil::is_same<T, std::string>::value &&
FLAGS_save_series) {
@@ -255,10 +262,16 @@ public:
// 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 @@ public:
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<InvOp, detail::VoidOp>::value &&
!butil::is_same<T, std::string>::value &&
FLAGS_save_series) {
diff --git a/src/bvar/window.h b/src/bvar/window.h
index fcd29e11..31c73b98 100644
--- a/src/bvar/window.h
+++ b/src/bvar/window.h
@@ -22,6 +22,8 @@
#include <limits> // std::numeric_limits
#include <math.h> // round
+#include <type_traits> // std::decay
+#include <utility> // std::declval
#include <gflags/gflags_declare.h>
#include "butil/logging.h" // LOG
#include "bvar/detail/sampler.h"
@@ -45,19 +47,30 @@ public:
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<decltype(std::declval<R&>().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 @@ public:
void describe(std::ostream& os) { _series.describe(os, nullptr); }
private:
WindowBase* _owner;
- detail::Series<value_type, Op> _series;
+ Series<value_type, Op> _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<value_type>* result)
const {
+ bool get_span(time_t window_size, Sample<value_type>* result) const {
return _sampler->get_value(window_size, result);
}
- bool get_span(detail::Sample<value_type>* result) const {
+ bool get_span(Sample<value_type>* result) const {
return get_span(_window_size, result);
}
virtual value_type get_value(time_t window_size) const {
- detail::Sample<value_type> tmp;
+ Sample<value_type> tmp;
if (get_span(window_size, &tmp)) {
return tmp.data;
}
@@ -148,13 +169,17 @@ protected:
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 @@ public:
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_type, std::string>::value && quote_string) {
os << '"' << get_value() << '"';
} else {
@@ -298,7 +322,7 @@ public:
}
}
- virtual ~WindowExAdapter() {
+ ~WindowExAdapter() override {
hide();
}
diff --git a/test/bvar_sampler_unittest.cpp b/test/bvar_sampler_unittest.cpp
index 40cc22d9..bfa79eed 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 <limits> //std::numeric_limits
#include "bvar/detail/sampler.h"
-#include "butil/time.h"
#include "butil/logging.h"
#include <gtest/gtest.h>
diff --git a/test/bvar_window_unittest.cpp b/test/bvar_window_unittest.cpp
index e50cf109..ace39bd3 100644
--- a/test/bvar_window_unittest.cpp
+++ b/test/bvar_window_unittest.cpp
@@ -23,6 +23,8 @@
#include <sstream>
#include <butil/time.h>
#include <butil/macros.h>
+#include <butil/logging.h> // logging::StringSink
+#include <butil/compiler_specific.h> // BUTIL_USE_ASAN
#include <gflags/gflags.h>
#include <gtest/gtest.h>
#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<bvar::Adder<int64_t> >* ps = nullptr;
+ {
+ // Named so that the diagnostic below can identify the offending bvar.
+ bvar::Adder<int64_t> a("window_outliving_referenced_var_adder");
+ a << 10;
+ ps = new bvar::PerSecond<bvar::Adder<int64_t> >(&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
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
