You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "lh2debug (via GitHub)" <gi...@apache.org> on 2025/11/06 03:56:36 UTC
[PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
lh2debug opened a new pull request, #3140:
URL: https://github.com/apache/brpc/pull/3140
### What problem does this PR solve?
Issue Number: resolve #3068
Problem Summary:
Span lifecycle management defect in distributed storage system:
Problem Scenario:
- ChunkServer maintains a parent span for client WriteChunk requests
- Two child spans are created for append_entries RPCs to followers
- When the first follower responds, the parent span is prematurely destroyed along with all child spans
- When the second follower responds, it attempts to access the already-freed child span, causing use-after-free
Root Cause:
a. Premature deallocation: Parent span destroyed while child spans still in use
b. Dangling pointer: Response callback accesses freed span objects
More details:
https://github.com/lh2debug/brpc/blob/master-lh2-fix-span-pdf/Optimization%20of%20RPCZ%20Span%20Lifecycle%20Management.pdf
### What is changed and the side effects?
Changed:
Side effects: NO
- Performance effects: NO
- 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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2521108874
##########
src/brpc/span.h:
##########
@@ -181,11 +269,38 @@ friend class SpanDB;
// time2_us \s annotation2 <SEP>
// ...
std::string _info;
+ // Protects _info from concurrent modifications.
+ // Multiple threads may call Annotate() simultaneously (e.g., retry logic,
+ // network layer, user code via TRACEPRINTF), causing data corruption in
+ // string concatenation without synchronization.
+ mutable pthread_spinlock_t _info_spinlock;
+
+ std::weak_ptr<Span> _local_parent;
+ std::list<std::shared_ptr<Span>> _client_list;
+ // Protects _client_list from concurrent modifications.
+ // In some scenarios, multiple bthreads may simultaneously create child spans
+ // (e.g.,raft leader parallel RPCs to followers) and push_back to parent's _client_list.
+ // Also protects against concurrent iteration (e.g., CountClientSpans, SpanDB::Index)
+ // while the list is being modified.
+ mutable pthread_spinlock_t _client_list_spinlock;
+};
+
+class SpanContainer : public bvar::Collected {
+public:
+ explicit SpanContainer(std::shared_ptr<Span> span) : _span(span) {}
Review Comment:
const std::shared_ptr<Span>& span ?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3753396328
Okay, I'll think about these suggestions. Thank you.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2625497213
##########
src/brpc/controller.cpp:
##########
@@ -1387,8 +1390,18 @@ const Controller* Controller::sub(int index) const {
return NULL;
}
-uint64_t Controller::trace_id() const { return _span ? _span->trace_id() : 0; }
-uint64_t Controller::span_id() const { return _span ? _span->span_id() : 0; }
+uint64_t Controller::trace_id() const {
+ if (auto span = _span.lock()) {
+ return span->trace_id();
+ }
+ return 0;
+}
+uint64_t Controller::span_id() const {
Review Comment:
add a new line
##########
src/brpc/builtin/rpcz_service.cpp:
##########
@@ -372,22 +400,22 @@ static void PrintServerSpan(std::ostream& os, const RpczSpan& span,
if (PrintAnnotationsAndRealTimeSpan(
os, span.start_send_real_us(),
- &last_time, extr, ARRAY_SIZE(extr))) {
+ &last_time, extr, ARRAY_SIZE(extr), &span)) {
if (entered_user_method) {
- os << " Leave " << WebEscape(span.full_method_name()) << std::endl;
+ os << " [ServerSpan " << SPAN_ID_STR << '=' << Hex(span.span_id()) << "] Leave " << WebEscape(span.full_method_name()) << std::endl;
} else {
- os << " Responding" << std::endl;
+ os << " [ServerSpan " << SPAN_ID_STR << '=' << Hex(span.span_id()) << "] Responding" << std::endl;
}
}
if (PrintAnnotationsAndRealTimeSpan(
os, span.sent_real_us(),
- &last_time, extr, ARRAY_SIZE(extr))) {
- os << " Responded(" << span.response_size() << ')' << std::endl;
+ &last_time, extr, ARRAY_SIZE(extr), &span)) {
Review Comment:
This format is a little bit tedious. How about `[Server SPAN#S1] Processing the request in a new worker`?
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -642,12 +642,16 @@ void ProcessRpcRequest(InputMessageBase* msg_base) {
bthread_assign_data((void*)&server->thread_local_options());
}
- Span* span = NULL;
+ std::shared_ptr<Span> span;
if (IsTraceable(request_meta.has_trace_id())) {
span = Span::CreateServerSpan(
request_meta.trace_id(), request_meta.span_id(),
request_meta.parent_span_id(), msg->base_real_us());
+#if BRPC_SPAN_ENABLE_SHARED_PTR_API
accessor.set_span(span);
+#else
+ accessor.set_span(span.get());
Review Comment:
you can add an overloaded function `set_span(std::shared_ptr<Span> span)`,
so that you don't need to use `BRPC_SPAN_ENABLE_SHARED_PTR_API` everywhere.
##########
src/bthread/key.cpp:
##########
@@ -672,4 +694,19 @@ void* bthread_get_assigned_data() {
return bthread::tls_bls.assigned_data;
}
+int bthread_set_span_funcs(bthread_create_span_fn create_fn,
+ bthread_destroy_span_fn destroy_fn,
Review Comment:
I don't think `key.cpp` is responsible for span's lifecycle triggering points.
`key.cpp` is for the implementation of `bthread_get/setspecific`, it manage keytable_pool and keytable, which has no relationship to span.
What you need is to call the span functions in the bthread lifecycle (create/end/...), the better place is in the `task_group.cpp`, you can get/set bthread::tls_bls.rpcz_parent_span there.
And the implementation of `bthread_set_span_funcs` is better placed into `bthread.cpp`, because most bthread interface functions are there.
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,97 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ delete static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ }
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
Review Comment:
I don't know why you need to delete and new the weak_ptr, can you directly reset the weak_ptr like `*ls.rpcz_parent_span = span`?
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,97 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ delete static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ }
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ delete static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
Review Comment:
I think you don't need to delete here, just `ls.rpcz_parent_span->reset()`
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3140:
URL: https://github.com/apache/brpc/pull/3140
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3501704485
4. To ensure compatibility with advanced clients who modify their protocols, using the `BRPC_ENABLE_SHARED_SPAN` macro to control the use of `Span*` or `std::weak_ptr<Span>` in the `ControllerPrivateAccessor` layer can resolve compilation issues. However, the `Span*` approach may still cause crashes in certain scenarios.
If the implementation follows the `BRPC_ENABLE_SHARED_SPAN` macro control, could it mislead them into believing that the `Span*` approach is stable and reliable for performance analysis?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2521149775
##########
src/bthread/key.cpp:
##########
@@ -405,6 +417,11 @@ static void cleanup_pthread(void* arg) {
delete kt;
// After deletion: tls may be set during deletion.
tls_bls.keytable = NULL;
+
Review Comment:
What is the relationship between rpcz_parent_span and key.cpp?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2986814640
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ *static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span) = span;
+ } else {
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+ }
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span)->reset();
+ }
+}
+
+bool HasTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return false;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return !weak_ptr->expired();
+}
+
+
+void SpanDeleter::operator()(Span* r) const {
+ if (r == NULL) {
+ return;
+ }
+
+ // All children will be destroyed automatically along with the list.
+ // The list holds std::shared_ptr<> which will trigger deletion of
+ // children.
+ r->_client_list.clear();
+ r->_info.clear();
+ // Destroy the spinlocks, as the destructor might not be invoked.
+ pthread_spin_destroy(&r->_client_list_spinlock);
+ pthread_spin_destroy(&r->_info_spinlock);
Review Comment:
Although the object pool reuses memory, `butil::get_object(Forbidden())` internally employs placement new; this invokes the constructor anew on the reused memory, thereby re-initializing the spinlock.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2972600042
##########
src/bthread/task_group.cpp:
##########
@@ -495,7 +507,11 @@ int TaskGroup::start_foreground(TaskGroup** pg,
m->attr = using_attr;
m->local_storage = LOCAL_STORAGE_INIT;
if (using_attr.flags & BTHREAD_INHERIT_SPAN) {
- m->local_storage.rpcz_parent_span = run_create_span_func();
+ if (g_create_bthread_span) {
+ m->local_storage.rpcz_parent_span = g_create_bthread_span();
Review Comment:
> The parent Span's _client_list
What if the parent span is NULL?
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ *static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span) = span;
+ } else {
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+ }
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span)->reset();
+ }
+}
+
+bool HasTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return false;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return !weak_ptr->expired();
+}
+
+
+void SpanDeleter::operator()(Span* r) const {
+ if (r == NULL) {
+ return;
+ }
+
+ // All children will be destroyed automatically along with the list.
+ // The list holds std::shared_ptr<> which will trigger deletion of
+ // children.
+ r->_client_list.clear();
+ r->_info.clear();
+ // Destroy the spinlocks, as the destructor might not be invoked.
+ pthread_spin_destroy(&r->_client_list_spinlock);
+ pthread_spin_destroy(&r->_info_spinlock);
Review Comment:
> (the lock is destroyed, and it is returned to the object pool)
> Subsequently, only new spans will be retrieved from the object pool for use.
At this point, when a pre-constructed span is retrieved from the object pool, its spinlock will not be initialized, but the spinlock was destroyed during the previous return.
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
Review Comment:
I think bthread_t is not suitable as ending_cid because they are of different types, and the value of bthread_t may conflict with the value of bthread_id_t.
How about adding a `_ending_tid` to be used as the ending id of the bthread span?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3763345769
> All modifications have been completed and testing is finished. Please take a look when you have time. @chenBright
Please resolve conflicts in channel.cpp.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2958516752
##########
src/bthread/task_group.cpp:
##########
@@ -495,7 +507,11 @@ int TaskGroup::start_foreground(TaskGroup** pg,
m->attr = using_attr;
m->local_storage = LOCAL_STORAGE_INIT;
if (using_attr.flags & BTHREAD_INHERIT_SPAN) {
- m->local_storage.rpcz_parent_span = run_create_span_func();
+ if (g_create_bthread_span) {
+ m->local_storage.rpcz_parent_span = g_create_bthread_span();
Review Comment:
1.
1) The parent Span's _client_list
2) Temporarily holding a shared_ptr during various function calls, such as GetTlsParentSpan
2.
1) A bthread span is required. The original code already includes an implementation of bthread spans, which are used to track the creation and execution of bthreads.
2) Without a bthread span, it's impossible to track client RPCs initiated by other bthreads (such as the use case of Braft's append_entrics).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2992295120
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ *static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span) = span;
+ } else {
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+ }
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span)->reset();
+ }
+}
+
+bool HasTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return false;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return !weak_ptr->expired();
+}
+
+
+void SpanDeleter::operator()(Span* r) const {
+ if (r == NULL) {
+ return;
+ }
+
+ // All children will be destroyed automatically along with the list.
+ // The list holds std::shared_ptr<> which will trigger deletion of
+ // children.
+ r->_client_list.clear();
+ r->_info.clear();
+ // Destroy the spinlocks, as the destructor might not be invoked.
+ pthread_spin_destroy(&r->_client_list_spinlock);
+ pthread_spin_destroy(&r->_info_spinlock);
Review Comment:
done and test ok
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2522266344
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -269,9 +269,9 @@ struct BaiduProxyPBMessages : public RpcPBMessages {
// Used by UT, can't be static.
void SendRpcResponse(int64_t correlation_id, Controller* cntl,
RpcPBMessages* messages, const Server* server,
- MethodStatus* method_status, int64_t received_us) {
+ MethodStatus* method_status, int64_t received_us,
+ std::shared_ptr<Span> span) {
Review Comment:
SendRpcResponse needs to hold a strong reference count; otherwise, if the backend spancontainer releases the reference, the server span will be unable to set_start_send_us and set_response_size.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] WIP: Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3510052026
> > Yes. For example, in the Raft protocol, there is one leader and two followers. The leader only needs to wait for the first follower to complete before returning.
>
> Under this implementation logic, the span function still only counts the time taken for the first response, right?
Yes, in this scenario, when the server span is committed, only the first client span (i.e., only the terminated child span) will be counted; the second ongoing client span will not be counted.
When the server span is destroyed, the reference counts of the first and second client spans will be canceled. Shortly afterward, the reference counts of these two client spans will be reduced to 0, and they will be garbage collected.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2498299385
##########
src/bthread/task_meta.h:
##########
@@ -43,10 +48,10 @@ struct ButexWaiter;
struct LocalStorage {
KeyTable* keytable;
void* assigned_data;
- void* rpcz_parent_span;
+ std::weak_ptr<brpc::Span> rpcz_parent_span;
Review Comment:
I think you 'd better not use brpc types in the bthread code.
In the brpc design, bthread is a basic library, and brpc is built on top of it. So bthread should no depend on brpc. This is why the original type is `void*`, not `brpc::Span*`.
My suggestion: you can keep `void* rpcz_parent_span` here, and in the brpc code, you can new a `std::weak_ptr<brpc::Span>`, then cast the pointer `std::weak_ptr<brpc::Span>*` to `void*`. This may be more complicated, but keep the brpc design clean.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3500609551
I believe that spans are suitable for requesting related information and should not be more closely associated with threads or coroutines; they simply use thread-local storage to pass variables in their implementation.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3501682350
1. In LocalStorage, `rpcz_parent_span` maintains a `void*` type. The span layer provides functions to manage the `weak_ptr` pointer using `new`/`delete` and assign it to `rpcz_parent_span`. I've considered and tested this approach, and it seems feasible.
2. Regarding the method of embedding reference counting within the span:
1) It's more complex in principle and implementation than the smart pointer method. Imagine a Raft scenario: one server span creates two bthreads. Each bthread context initiates an RPC (i.e., creates a client span). That is, the server span is the parent of the bthread span, and the bthread span is the parent of the client span. In this case, manually adding references/releasing references to the server span, bthread span, and client span is much more complex than the smart pointer method.
2) It's not flexible enough for business trace collection. For example, if the business wants to collect some trace information in a background thread outside the brpc context, the smart pointer method simply passes the `weak_ptr` as a function parameter to the function of that background thread. When the background thread wants to collect traces, it only needs to use `weak_ptr.lock` (if the lock succeeds, the collection is successful; if the lock fails, it means the RPC has ended and collection is no longer needed). Implementing this logic manually using reference counting is not easy (it requires elegant manual `add ref` and `release` operations).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2972569508
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
Review Comment:
I think bthread_t is not suitable as ending_cid because they are of different types, and the value of bthread_t may conflict with the value of bthread_id_t.
How about adding a _ending_tid to be used as the ending id of the bthread span?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2522480292
##########
src/bthread/key.cpp:
##########
@@ -672,4 +694,19 @@ void* bthread_get_assigned_data() {
return bthread::tls_bls.assigned_data;
}
+int bthread_set_span_funcs(bthread_create_span_fn create_fn,
+ bthread_destroy_span_fn destroy_fn,
Review Comment:
Thank you for your suggestion. Personally, I think it might be more appropriate to manage it under key.cpp, because span is a brpc layer concept, not a bthread concept. bthread.cpp should focus on the bthread lifecycle. key.cpp is responsible for span's lifecycle triggering points (creation/destruction/end) and is decoupled from the specific implementation of brpc (span.cpp) through a global callback.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2986812762
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
Review Comment:
done
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3499926676
maybe we can add a reference counter in span, There may be much less code modification.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2697472882
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,97 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ delete static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ }
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ delete static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
Review Comment:
done
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3766057064
The conflict has been resolved, and compilation and testing have passed.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2992119843
##########
src/bthread/task_group.cpp:
##########
@@ -495,7 +507,11 @@ int TaskGroup::start_foreground(TaskGroup** pg,
m->attr = using_attr;
m->local_storage = LOCAL_STORAGE_INIT;
if (using_attr.flags & BTHREAD_INHERIT_SPAN) {
- m->local_storage.rpcz_parent_span = run_create_span_func();
+ if (g_create_bthread_span) {
+ m->local_storage.rpcz_parent_span = g_create_bthread_span();
Review Comment:
OK.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3500524671
Root Cause:
a. Premature deallocation: Parent span destroyed while child spans still in use
b. Dangling pointer: Response callback accesses freed span objects
The span information is a tree structure, and this relationship is maintained through the ref information. For example, ParentSpan initially has ref=1, and two ClientSpans are created, so ref becomes 3.
When one ClientSpan returns, ref becomes 2; when the other ClientSpan returns, ref becomes 1. At this point, it can be destructed.
Because a single call might be split into a tree, the bthreads generated by this call should follow the lifecycle principle of this call chain, which should be fine.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] WIP: Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3509889193
> Yes. For example, in the Raft protocol, there is one leader and two followers. The leader only needs to wait for the first follower to complete before returning.
Under this implementation logic, the span function still only counts the time taken for the first response, right?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] WIP: Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3514803890
@wwbmmm @yanglimingcn The code for bthread that doesn't depend on the brpc module (i.e., rpcz_parent_span remains void*) has been modified and tested. Please take a look, tkx~
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2520935793
##########
src/brpc/builtin/rpcz_service.cpp:
##########
@@ -372,22 +400,22 @@ static void PrintServerSpan(std::ostream& os, const RpczSpan& span,
if (PrintAnnotationsAndRealTimeSpan(
os, span.start_send_real_us(),
- &last_time, extr, ARRAY_SIZE(extr))) {
+ &last_time, extr, ARRAY_SIZE(extr), &span)) {
if (entered_user_method) {
- os << " Leave " << WebEscape(span.full_method_name()) << std::endl;
+ os << " [ServerSpan " << SPAN_ID_STR << '=' << Hex(span.span_id()) << "] Leave " << WebEscape(span.full_method_name()) << std::endl;
} else {
- os << " Responding" << std::endl;
+ os << " [ServerSpan " << SPAN_ID_STR << '=' << Hex(span.span_id()) << "] Responding" << std::endl;
}
}
if (PrintAnnotationsAndRealTimeSpan(
os, span.sent_real_us(),
- &last_time, extr, ARRAY_SIZE(extr))) {
- os << " Responded(" << span.response_size() << ')' << std::endl;
+ &last_time, extr, ARRAY_SIZE(extr), &span)) {
Review Comment:
Could you please post some of the display information?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2521164493
##########
src/bthread/key.cpp:
##########
@@ -672,4 +694,19 @@ void* bthread_get_assigned_data() {
return bthread::tls_bls.assigned_data;
}
+int bthread_set_span_funcs(bthread_create_span_fn create_fn,
+ bthread_destroy_span_fn destroy_fn,
Review Comment:
Would it be more reasonable to put this function in bthread.cpp?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2521881036
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,97 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
Review Comment:
When is this newly created `weak_ptr` deleted? This part seems a bit obscure, and it's easy for memory leaks to occur.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2958511335
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
Review Comment:
The reason for setting bthread_t to ending_cid is to ensure that SpanDB::Index only collects completed bthread spans; otherwise, a race condition will occur.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2958511946
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ *static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span) = span;
+ } else {
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+ }
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span)->reset();
+ }
+}
+
+bool HasTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return false;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return !weak_ptr->expired();
+}
+
+
+void SpanDeleter::operator()(Span* r) const {
+ if (r == NULL) {
+ return;
+ }
+
+ // All children will be destroyed automatically along with the list.
+ // The list holds std::shared_ptr<> which will trigger deletion of
+ // children.
+ r->_client_list.clear();
+ r->_info.clear();
+ // Destroy the spinlocks, as the destructor might not be invoked.
+ pthread_spin_destroy(&r->_client_list_spinlock);
+ pthread_spin_destroy(&r->_info_spinlock);
Review Comment:
A span is initialized only once and destroyed once when its reference count is reset to zero (the lock is destroyed, and it is returned to the object pool). At this point, the span's lifecycle ends.
Subsequently, only new spans will be retrieved from the object pool for use.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-4088551501
The conflict has been resolved, rpcz.md has been updated, and tests have passed.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2992119843
##########
src/bthread/task_group.cpp:
##########
@@ -495,7 +507,11 @@ int TaskGroup::start_foreground(TaskGroup** pg,
m->attr = using_attr;
m->local_storage = LOCAL_STORAGE_INIT;
if (using_attr.flags & BTHREAD_INHERIT_SPAN) {
- m->local_storage.rpcz_parent_span = run_create_span_func();
+ if (g_create_bthread_span) {
+ m->local_storage.rpcz_parent_span = g_create_bthread_span();
Review Comment:
ok
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2987607979
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ *static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span) = span;
+ } else {
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+ }
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span)->reset();
+ }
+}
+
+bool HasTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return false;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return !weak_ptr->expired();
+}
+
+
+void SpanDeleter::operator()(Span* r) const {
+ if (r == NULL) {
+ return;
+ }
+
+ // All children will be destroyed automatically along with the list.
+ // The list holds std::shared_ptr<> which will trigger deletion of
+ // children.
+ r->_client_list.clear();
+ r->_info.clear();
+ // Destroy the spinlocks, as the destructor might not be invoked.
+ pthread_spin_destroy(&r->_client_list_spinlock);
+ pthread_spin_destroy(&r->_info_spinlock);
Review Comment:
These two branches will not execute `placement new`, but will directly return the existing object.
https://github.com/apache/brpc/blob/0565d8d2c7a1cde87e94aa222fe4946e6f20c358/src/butil/object_pool_inl.h#L183-L193
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2522340078
##########
src/brpc/builtin/rpcz_service.cpp:
##########
@@ -372,22 +400,22 @@ static void PrintServerSpan(std::ostream& os, const RpczSpan& span,
if (PrintAnnotationsAndRealTimeSpan(
os, span.start_send_real_us(),
- &last_time, extr, ARRAY_SIZE(extr))) {
+ &last_time, extr, ARRAY_SIZE(extr), &span)) {
if (entered_user_method) {
- os << " Leave " << WebEscape(span.full_method_name()) << std::endl;
+ os << " [ServerSpan " << SPAN_ID_STR << '=' << Hex(span.span_id()) << "] Leave " << WebEscape(span.full_method_name()) << std::endl;
} else {
- os << " Responding" << std::endl;
+ os << " [ServerSpan " << SPAN_ID_STR << '=' << Hex(span.span_id()) << "] Responding" << std::endl;
}
}
if (PrintAnnotationsAndRealTimeSpan(
os, span.sent_real_us(),
- &last_time, extr, ARRAY_SIZE(extr))) {
- os << " Responded(" << span.response_size() << ')' << std::endl;
+ &last_time, extr, ARRAY_SIZE(extr), &span)) {
Review Comment:
As shown below, the type of span (server span, client span, bthread span) is indicated at the beginning to facilitate troubleshooting.
```
Fetching new trace TRACE#B from HOST#X
Received request REQ#65674 from HOST#Y via PROTO#1 log=0 trace=TRACE#B span=SPAN#S1
[ServerSpan span=SPAN#S1] Processing the request in a new worker
[ServerSpan span=SPAN#S1] ParseStats[cut=?, queue=?, worker=?, rpc=?]
[ServerSpan span=SPAN#S1] Enter Ecom.OrderService.PlaceOrder
[ClientSpan span=SPAN#C1] Cart.AddItem item=ITEM# qty=QTY# price=PRICE# user=USER# session=SESSION#
[ClientSpan span=SPAN#C1] Cart.VerifyOperation item=ITEM# op=OP# user=USER# session=SESSION#
[ClientSpan span=SPAN#C1] Cart.ProposeUpdate item=ITEM# op=OP# user=USER# session=SESSION# attempt=SN#
[ClientSpan span=SPAN#C1] Orchestration.Manager: enqueue step=PlaceOrder first=I# last=I#
[ClientSpan span=SPAN#C1] Orchestration.Queue: pending step
Requesting Ecom.Consensus.Append@HOST#Z PROTO#1 call=CALL# trace=TRACE#B span=SPAN#C1
[ClientSpan span=SPAN#C1] Requested(REQ#65778) [1]
[ClientSpan span=SPAN#C1] Received response(RSP#) of request[1]
[ClientSpan span=SPAN#C1] Processing the response in a new worker
[ClientSpan span=SPAN#C1] Enter client callback
[ClientSpan span=SPAN#C1] Workflow.OnApply, ongoing tasks: 0
[ClientSpan span=SPAN#C1] TaskQueue status running=R# queues=Q# active=A#
[ClientSpan span=SPAN#C1] OrderPipeline.WriteOrder
[ClientSpan span=SPAN#C1] Payment.Authorize
[ClientSpan span=SPAN#C1] Inventory.Reserve
[ClientSpan span=SPAN#C1] Promotion.Apply
[ClientSpan span=SPAN#C1] Promotion.Apply_done
[ClientSpan span=SPAN#C1] Notification.Prepare
[ClientSpan span=SPAN#C1] Notification.Prepare_done
[ClientSpan span=SPAN#C1] Inventory.Reserve_done
[ClientSpan span=SPAN#C1] Payment.Authorize_done
[ClientSpan span=SPAN#C1] OrderPipeline.Sync
[ClientSpan span=SPAN#C1] OrderPipeline.WriteOrder done (~2.7ms)
[ClientSpan span=SPAN#C1] Latency stats: avg≈58us p90≈66us p99≈160us
[ClientSpan span=SPAN#C1] Protocol response before join
[ClientSpan span=SPAN#C1] Protocol response after join
[ClientSpan span=SPAN#C1] FulfillmentWorker: ongoing tasks=3
[ClientSpan span=SPAN#C1] FulfillmentWorker: global queue running=R# queues=Q# active=A#
[ClientSpan span=SPAN#C1] FulfillmentWorker: enter
[ClientSpan span=SPAN#C1] Batcher.append
[ClientSpan span=SPAN#C1] Batcher.flush queue_wait≈~6.7ms
Requesting Ecom.Consensus.Append@HOST#Y PROTO#1 call=CALL# trace=TRACE#B span=SPAN#C2
[ClientSpan span=SPAN#C2] Requested(REQ#65778) [1]
[ClientSpan span=SPAN#C2] Received response(RSP#) of request[1]
[ClientSpan span=SPAN#C2] Processing the response in a new worker
[ClientSpan span=SPAN#C2] Enter client callback
[ServerSpan span=SPAN#S1] Leave Ecom.OrderService.PlaceOrder
[ServerSpan span=SPAN#S1] Responded
```
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2521068534
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -269,9 +269,9 @@ struct BaiduProxyPBMessages : public RpcPBMessages {
// Used by UT, can't be static.
void SendRpcResponse(int64_t correlation_id, Controller* cntl,
RpcPBMessages* messages, const Server* server,
- MethodStatus* method_status, int64_t received_us) {
+ MethodStatus* method_status, int64_t received_us,
+ std::shared_ptr<Span> span) {
Review Comment:
Why SendRpcResponse need span parameter, It can be obtained from the accessor?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2697472055
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -642,12 +642,16 @@ void ProcessRpcRequest(InputMessageBase* msg_base) {
bthread_assign_data((void*)&server->thread_local_options());
}
- Span* span = NULL;
+ std::shared_ptr<Span> span;
if (IsTraceable(request_meta.has_trace_id())) {
span = Span::CreateServerSpan(
request_meta.trace_id(), request_meta.span_id(),
request_meta.parent_span_id(), msg->base_real_us());
+#if BRPC_SPAN_ENABLE_SHARED_PTR_API
accessor.set_span(span);
+#else
+ accessor.set_span(span.get());
Review Comment:
done
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,97 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ delete static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ }
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
Review Comment:
done
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3500877688
> The span information is a tree structure, and this relationship is maintained through the ref information. For example, ParentSpan initially has ref=1, and two ClientSpans are created, so ref becomes 3.
>
> When one ClientSpan returns, ref becomes 2; when the other ClientSpan returns, ref becomes 1. At this point, it can be destructed.
Let me complement, the first reference to the ParentSpan is held by tls, when the bthread exit, ref_count will -1, when the ref_count become 0 (only after all ClientSpan return and the bthread exit) , it can be destructed.
Moreover, when a new bthread with BTHREAD_INHERIT_SPAN is created, the ref_count will +1; when this bthread exit, the ref_count will -1.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3500077098
Thank you for your answer. This approach of implementing reference counting internally within a Span seems impractical.
Because reference Counting with void* rpcz_parent_span Fails Across Bthreads:
Raw pointers in LocalStorage cannot detect when the referenced Span is destroyed in another bthread. When a parent Span is deleted (after its reference count reaches zero), child bthreads still hold the same pointer value pointing to freed memory, causing use-after-free. Each Span's reference count only tracks its own operations and cannot prevent destruction when other bthreads still reference it through raw pointers.
@yanglimingcn
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2522308829
##########
src/bthread/task_meta.h:
##########
@@ -43,10 +48,10 @@ struct ButexWaiter;
struct LocalStorage {
KeyTable* keytable;
void* assigned_data;
- void* rpcz_parent_span;
+ std::weak_ptr<brpc::Span> rpcz_parent_span;
Review Comment:
done
##########
src/brpc/span.h:
##########
@@ -181,11 +269,38 @@ friend class SpanDB;
// time2_us \s annotation2 <SEP>
// ...
std::string _info;
+ // Protects _info from concurrent modifications.
+ // Multiple threads may call Annotate() simultaneously (e.g., retry logic,
+ // network layer, user code via TRACEPRINTF), causing data corruption in
+ // string concatenation without synchronization.
+ mutable pthread_spinlock_t _info_spinlock;
+
+ std::weak_ptr<Span> _local_parent;
+ std::list<std::shared_ptr<Span>> _client_list;
+ // Protects _client_list from concurrent modifications.
+ // In some scenarios, multiple bthreads may simultaneously create child spans
+ // (e.g.,raft leader parallel RPCs to followers) and push_back to parent's _client_list.
+ // Also protects against concurrent iteration (e.g., CountClientSpans, SpanDB::Index)
+ // while the list is being modified.
+ mutable pthread_spinlock_t _client_list_spinlock;
+};
+
+class SpanContainer : public bvar::Collected {
+public:
+ explicit SpanContainer(std::shared_ptr<Span> span) : _span(span) {}
Review Comment:
Okay, I'll change this later.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3710052910
@lh2debug Friendly ping!
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3496094625
When the first follower responds, the parent span is prematurely destroyed along with all child spans
Because you just need to wait the first response?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3496132413
Yes. For example, in the Raft protocol, there is one leader and two followers. The leader only needs to wait for the first follower to complete before returning.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3496491514
@lh2debug Thank you for your contribution! This solve a very important issue. You have done a good job!
Currently I have two concerns:
1. This implementation add some locks and shard_ptr，it may affect performance. Have you compare its performance with the original version?
2. This implementation change some internal APIs (such as `ControllerPrivateAccessor::span()`. Although these will not be used by most brpc user, they may be used by some advance users, who have extended brpc functions (for example, add new protocols). So this PR will make a incompatible upgrade to these users. Maybe you can try to add some macros to controll this feature, for example, you can define alias types like SpanSharedPtr and SpanWeakPtr, when BRPC_ENABLE_SHARED_SPAN is on, they are `std::shared_ptr<Span>` and `std::weak_ptr<Span>`, when the flag is off, they are `Span*`.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2936385819
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ *static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span) = span;
+ } else {
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+ }
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span)->reset();
+ }
+}
+
+bool HasTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return false;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return !weak_ptr->expired();
+}
+
+
+void SpanDeleter::operator()(Span* r) const {
+ if (r == NULL) {
+ return;
+ }
+
+ // All children will be destroyed automatically along with the list.
+ // The list holds std::shared_ptr<> which will trigger deletion of
+ // children.
+ r->_client_list.clear();
+ r->_info.clear();
+ // Destroy the spinlocks, as the destructor might not be invoked.
+ pthread_spin_destroy(&r->_client_list_spinlock);
+ pthread_spin_destroy(&r->_info_spinlock);
Review Comment:
It appears that the spinlock is only initialized once in the Span constructor, but is destroyed once before each SpanDeleter::operator() call.
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
Review Comment:
Why is bthread_t set to ending_cid?
##########
src/bthread/task_group.cpp:
##########
@@ -495,7 +507,11 @@ int TaskGroup::start_foreground(TaskGroup** pg,
m->attr = using_attr;
m->local_storage = LOCAL_STORAGE_INIT;
if (using_attr.flags & BTHREAD_INHERIT_SPAN) {
- m->local_storage.rpcz_parent_span = run_create_span_func();
+ if (g_create_bthread_span) {
+ m->local_storage.rpcz_parent_span = g_create_bthread_span();
Review Comment:
Who has a strong reference to bthread span?
Furthermore, after resolving the thread safety issues of spans, is it still necessary to use bthread spans?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2958516752
##########
src/bthread/task_group.cpp:
##########
@@ -495,7 +507,11 @@ int TaskGroup::start_foreground(TaskGroup** pg,
m->attr = using_attr;
m->local_storage = LOCAL_STORAGE_INIT;
if (using_attr.flags & BTHREAD_INHERIT_SPAN) {
- m->local_storage.rpcz_parent_span = run_create_span_func();
+ if (g_create_bthread_span) {
+ m->local_storage.rpcz_parent_span = g_create_bthread_span();
Review Comment:
first
1) The parent Span's _client_list
2) Temporarily holding a shared_ptr during various function calls, such as GetTlsParentSpan
second
1) A bthread span is required. The original code already includes an implementation of bthread spans, which are used to track the creation and execution of bthreads.
2) Without a bthread span, it's impossible to track client RPCs initiated by other bthreads (such as the use case of Braft's append_entrics).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2992100665
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ *static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span) = span;
+ } else {
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+ }
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span)->reset();
+ }
+}
+
+bool HasTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return false;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return !weak_ptr->expired();
+}
+
+
+void SpanDeleter::operator()(Span* r) const {
+ if (r == NULL) {
+ return;
+ }
+
+ // All children will be destroyed automatically along with the list.
+ // The list holds std::shared_ptr<> which will trigger deletion of
+ // children.
+ r->_client_list.clear();
+ r->_info.clear();
+ // Destroy the spinlocks, as the destructor might not be invoked.
+ pthread_spin_destroy(&r->_client_list_spinlock);
+ pthread_spin_destroy(&r->_info_spinlock);
Review Comment:
pthread_spin_destroy needs to be called in the destructor.
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ *static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span) = span;
+ } else {
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+ }
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span)->reset();
+ }
+}
+
+bool HasTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return false;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return !weak_ptr->expired();
+}
+
+
+void SpanDeleter::operator()(Span* r) const {
+ if (r == NULL) {
+ return;
+ }
+
+ // All children will be destroyed automatically along with the list.
+ // The list holds std::shared_ptr<> which will trigger deletion of
+ // children.
+ r->_client_list.clear();
+ r->_info.clear();
+ // Destroy the spinlocks, as the destructor might not be invoked.
+ pthread_spin_destroy(&r->_client_list_spinlock);
+ pthread_spin_destroy(&r->_info_spinlock);
Review Comment:
`pthread_spin_destroy` needs to be called in the destructor.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3501688691
3. Based on our system performance tests, the implementation using smart pointers showed minimal performance impact.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2521053139
##########
src/brpc/controller.cpp:
##########
@@ -1714,4 +1727,31 @@ void Controller::DoPrintLogPrefix(std::ostream& os) const {
}
}
+
+#if BRPC_SPAN_ENABLE_SHARED_PTR_API
+ControllerPrivateAccessor& ControllerPrivateAccessor::set_span(
+ std::shared_ptr<Span> span) {
+ _cntl->_span = span;
+ return *this;
+}
+#else
+ControllerPrivateAccessor& ControllerPrivateAccessor::set_span(Span* span) {
+ if (span) {
+ _cntl->_span = span->shared_from_this();
+ } else {
+ _cntl->_span.reset();
+ }
+ return *this;
+}
+#endif
+
+SpanPtr ControllerPrivateAccessor::span() const {
+ std::shared_ptr<Span> span_internal = _cntl->_span.lock();
+#if BRPC_SPAN_ENABLE_SHARED_PTR_API
+ return span_internal;
+#else
+ return span_internal.get();
Review Comment:
return Span* not hold a reference, This will cause problems, won't it?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3526115024
> 2\. Regarding the method of embedding reference counting within the span:
>
> a) It's more complex in principle and implementation than the smart pointer method. Imagine a Raft scenario: one server span creates two bthreads. Each bthread context initiates an RPC (i.e., creates a client span). That is, the server span is the parent of the bthread span, and the bthread span is the parent of the client span. In this case, manually adding references/releasing references to the server span, bthread span, and client span is much more complex than the smart pointer method.
>
> b) It's not flexible enough for business trace collection. For example, if the business wants to collect some trace information in a background thread outside the brpc context, the smart pointer method simply passes the `weak_ptr` as a function parameter to the function of that background thread. When the background thread wants to collect traces, it only needs to use `weak_ptr.lock` (if the lock succeeds, the collection is successful; if the lock fails, it means the RPC has ended and collection is no longer needed). Implementing this logic manually using reference counting is not easy (it requires elegant manual `add ref` and `release` operations).
@chenBright What do you think of these two approaches ?
1. manually manage ref count of the parent (root) span
2. use shared_ptr and weak_ptr to manage all spans like this PR
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2522397911
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,97 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
Review Comment:
My earliest version used the `weak_ptr` type for `rpcz_parent_span` within `LocalStorage` because your team offered a very reasonable optimization reason: bthreads shouldn't depend on brpc. Therefore, `rpcz_parent_span` needed to retain its original `void*` type. The brpc layer provides manual `new/delete` management of `weak_ptr` for the bthread layer to register and use.
Therefore, during the bthread's lifecycle, the `weak_ptr` lifecycle of `rpcz_parent_span` needs to be manually managed using `new/delete` (i.e., `new` when the bthread is created, and `delete` when the bthread ends).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2522464644
##########
src/bthread/key.cpp:
##########
@@ -405,6 +417,11 @@ static void cleanup_pthread(void* arg) {
delete kt;
// After deletion: tls may be set during deletion.
tls_bls.keytable = NULL;
+
Review Comment:
`rpcz_parent_span` is a void* protocol field (type controlled by brpc) in bthread TLS that points to `weak_ptr<Span>`.
`key.cpp` is responsible for its lifecycle triggering points (creation/destruction/end) and is decoupled from the specific implementation of brpc (`span.cpp`) through a global callback.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3656203675
> > 2. Regarding the method of embedding reference counting within the span:
> > a) It's more complex in principle and implementation than the smart pointer method. Imagine a Raft scenario: one server span creates two bthreads. Each bthread context initiates an RPC (i.e., creates a client span). That is, the server span is the parent of the bthread span, and the bthread span is the parent of the client span. In this case, manually adding references/releasing references to the server span, bthread span, and client span is much more complex than the smart pointer method.
> > b) It's not flexible enough for business trace collection. For example, if the business wants to collect some trace information in a background thread outside the brpc context, the smart pointer method simply passes the `weak_ptr` as a function parameter to the function of that background thread. When the background thread wants to collect traces, it only needs to use `weak_ptr.lock` (if the lock succeeds, the collection is successful; if the lock fails, it means the RPC has ended and collection is no longer needed). Implementing this logic manually using reference counting is not easy (it requires elegant manual `add ref` and `release` operations).
>
> @chenBright What do you think of these two approaches ?
>
> 1. manually manage ref count of the parent (root) span
> 2. use shared_ptr and weak_ptr to manage all spans like this PR
Although we used [approache 1](https://github.com/apache/brpc/issues/2315#issuecomment-1683237115) before, I now prefer approache 2 because it offers better maintainability and extensibility after being refactored using smart pointers.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3501841280
> 4. To ensure compatibility with advanced clients who modify their protocols, using the `BRPC_ENABLE_SHARED_SPAN` macro to control the use of `Span*` or `std::weak_ptr<Span>` in the `ControllerPrivateAccessor` layer can resolve compilation issues. However, the `Span*` approach may still cause crashes in certain scenarios.
>
> If the implementation follows the `BRPC_ENABLE_SHARED_SPAN` macro control, could it mislead them into believing that the `Span*` approach is stable and reliable for performance analysis?
We can add some notes in the document and code comment. I think that's OK.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3497262017
Thank you for your suggestion. I will consider the above optimizations. @wwbmmm
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3770548053
LGTM
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2522288599
##########
src/brpc/controller.cpp:
##########
@@ -1714,4 +1727,31 @@ void Controller::DoPrintLogPrefix(std::ostream& os) const {
}
}
+
+#if BRPC_SPAN_ENABLE_SHARED_PTR_API
+ControllerPrivateAccessor& ControllerPrivateAccessor::set_span(
+ std::shared_ptr<Span> span) {
+ _cntl->_span = span;
+ return *this;
+}
+#else
+ControllerPrivateAccessor& ControllerPrivateAccessor::set_span(Span* span) {
+ if (span) {
+ _cntl->_span = span->shared_from_this();
+ } else {
+ _cntl->_span.reset();
+ }
+ return *this;
+}
+#endif
+
+SpanPtr ControllerPrivateAccessor::span() const {
+ std::shared_ptr<Span> span_internal = _cntl->_span.lock();
+#if BRPC_SPAN_ENABLE_SHARED_PTR_API
+ return span_internal;
+#else
+ return span_internal.get();
Review Comment:
If the server span waits for the child span to complete before returning (as is the case in most business scenarios), returning a raw pointer won't cause any problems.
For example, in a Raft scenario, if the server span returns before the second follower completes (causing the second follower's client span to be destroyed), the second follower's RPC callback will retrieve the destroyed dangling pointer through `ControllerPrivateAccessor::span`.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2525622496
##########
src/brpc/span.h:
##########
@@ -181,11 +269,38 @@ friend class SpanDB;
// time2_us \s annotation2 <SEP>
// ...
std::string _info;
+ // Protects _info from concurrent modifications.
+ // Multiple threads may call Annotate() simultaneously (e.g., retry logic,
+ // network layer, user code via TRACEPRINTF), causing data corruption in
+ // string concatenation without synchronization.
+ mutable pthread_spinlock_t _info_spinlock;
+
+ std::weak_ptr<Span> _local_parent;
+ std::list<std::shared_ptr<Span>> _client_list;
+ // Protects _client_list from concurrent modifications.
+ // In some scenarios, multiple bthreads may simultaneously create child spans
+ // (e.g.,raft leader parallel RPCs to followers) and push_back to parent's _client_list.
+ // Also protects against concurrent iteration (e.g., CountClientSpans, SpanDB::Index)
+ // while the list is being modified.
+ mutable pthread_spinlock_t _client_list_spinlock;
+};
+
+class SpanContainer : public bvar::Collected {
+public:
+ explicit SpanContainer(std::shared_ptr<Span> span) : _span(span) {}
Review Comment:
done
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2697471322
##########
src/brpc/controller.cpp:
##########
@@ -1387,8 +1390,18 @@ const Controller* Controller::sub(int index) const {
return NULL;
}
-uint64_t Controller::trace_id() const { return _span ? _span->trace_id() : 0; }
-uint64_t Controller::span_id() const { return _span ? _span->span_id() : 0; }
+uint64_t Controller::trace_id() const {
+ if (auto span = _span.lock()) {
+ return span->trace_id();
+ }
+ return 0;
+}
+uint64_t Controller::span_id() const {
Review Comment:
done
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in a… (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on PR #3140:
URL: https://github.com/apache/brpc/pull/3140#issuecomment-3758753988
All modifications have been completed and testing is finished. Please take a look when you have time.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2986817439
##########
src/bthread/task_group.cpp:
##########
@@ -495,7 +507,11 @@ int TaskGroup::start_foreground(TaskGroup** pg,
m->attr = using_attr;
m->local_storage = LOCAL_STORAGE_INIT;
if (using_attr.flags & BTHREAD_INHERIT_SPAN) {
- m->local_storage.rpcz_parent_span = run_create_span_func();
+ if (g_create_bthread_span) {
+ m->local_storage.rpcz_parent_span = g_create_bthread_span();
Review Comment:
When the parent span pointer is null, it indicates that the parent RPC has already returned. Subsequently, when the child span's reference count drops to zero, the child span will also be reclaimed.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2987618823
##########
src/bthread/task_group.cpp:
##########
@@ -495,7 +507,11 @@ int TaskGroup::start_foreground(TaskGroup** pg,
m->attr = using_attr;
m->local_storage = LOCAL_STORAGE_INIT;
if (using_attr.flags & BTHREAD_INHERIT_SPAN) {
- m->local_storage.rpcz_parent_span = run_create_span_func();
+ if (g_create_bthread_span) {
+ m->local_storage.rpcz_parent_span = g_create_bthread_span();
Review Comment:
If the parent span was not returned, but never existed, it seems impossible to create a bthread span.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2992018110
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,96 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread", base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ *static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span) = span;
+ } else {
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+ }
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span)->reset();
+ }
+}
+
+bool HasTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return false;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return !weak_ptr->expired();
+}
+
+
+void SpanDeleter::operator()(Span* r) const {
+ if (r == NULL) {
+ return;
+ }
+
+ // All children will be destroyed automatically along with the list.
+ // The list holds std::shared_ptr<> which will trigger deletion of
+ // children.
+ r->_client_list.clear();
+ r->_info.clear();
+ // Destroy the spinlocks, as the destructor might not be invoked.
+ pthread_spin_destroy(&r->_client_list_spinlock);
+ pthread_spin_destroy(&r->_info_spinlock);
Review Comment:
Testing on our Linux platform works fine. But you're right. The current code implementation calls pthread_spin_destroy when returning objects to the pool, yet it does not re-invoke pthread_spin_init when retrieving objects from the pool.
A more appropriate implementation approach I've thought of is to remove the pthread_spin_destroy call from SpanDeleter::operator, which ensures that the span fetched from the object pool still has pthread_spin_init initialized.
Do you have any suggestions?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2992113600
##########
src/bthread/task_group.cpp:
##########
@@ -495,7 +507,11 @@ int TaskGroup::start_foreground(TaskGroup** pg,
m->attr = using_attr;
m->local_storage = LOCAL_STORAGE_INIT;
if (using_attr.flags & BTHREAD_INHERIT_SPAN) {
- m->local_storage.rpcz_parent_span = run_create_span_func();
+ if (g_create_bthread_span) {
+ m->local_storage.rpcz_parent_span = g_create_bthread_span();
Review Comment:
Yes. Currently, if a bthread is created directly in a non-rpc context, the created bthread span will be destroyed immediately because there is no strong reference count. If you want to collect bthread span out of rpc context, the business layer needs to call CreateBthreadSpan and hold its strong reference count.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix span lifecycle with smart pointers to prevent use-after-free in async RPC callbacks (brpc)
Posted by "lh2debug (via GitHub)" <gi...@apache.org>.
lh2debug commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2958516752
##########
src/bthread/task_group.cpp:
##########
@@ -495,7 +507,11 @@ int TaskGroup::start_foreground(TaskGroup** pg,
m->attr = using_attr;
m->local_storage = LOCAL_STORAGE_INIT;
if (using_attr.flags & BTHREAD_INHERIT_SPAN) {
- m->local_storage.rpcz_parent_span = run_create_span_func();
+ if (g_create_bthread_span) {
+ m->local_storage.rpcz_parent_span = g_create_bthread_span();
Review Comment:
first question
1) The parent Span's _client_list
2) Temporarily holding a shared_ptr during various function calls, such as GetTlsParentSpan
second question
1) A bthread span is required. The original code already includes an implementation of bthread spans, which are used to track the creation and execution of bthreads.
2) Without a bthread span, it's impossible to track client RPCs initiated by other bthreads (such as the use case of Braft's append_entrics).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org