You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "walterzhaoJR (via GitHub)" <gi...@apache.org> on 2026/03/15 16:49:52 UTC
[PR] Fix IOBuf TLS block pool self-loop caused by double-return (#3243) (brpc)
walterzhaoJR opened a new pull request, #3244:
URL: https://github.com/apache/brpc/pull/3244
release_tls_block() and release_tls_block_chain() do not guard against a block being returned to TLS when it is already the list head. The assignment `b->portal_next = block_head` becomes `b->portal_next = b`, forming a self-loop that causes remove_tls_block_chain() or share_tls_block() to spin forever, silently hanging the thread at exit.
Fix:
- release_tls_block(): add early return when b == block_head, skipping the duplicate insertion.
- release_tls_block_chain(): during the existing chain walk, check each node against block_head before linking. Return early if overlap is detected so that num_blocks stays consistent with the actual list length (remove_tls_block_chain verifies this via CHECK_EQ).
Add three unit tests that reproduce the self-loop through:
1. Direct double release_tls_block() of the same block.
2. release_tls_block_chain() with a chain overlapping the TLS head.
3. IOBufAsZeroCopyOutputStream::BackUp() followed by a second release.
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
### What is changed and the side effects?
Changed:
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 IOBuf TLS block pool self-loop caused by double-return (#3243) (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3244:
URL: https://github.com/apache/brpc/pull/3244#issuecomment-4162248866
Does the current IOBuf and other wrapper classes trigger this issue?
--
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 IOBuf TLS block pool self-loop caused by double-return (#3243) (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3244:
URL: https://github.com/apache/brpc/pull/3244#issuecomment-4108314614
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 IOBuf TLS block pool self-loop caused by double-return (#3243) (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3244:
URL: https://github.com/apache/brpc/pull/3244#discussion_r2937820083
##########
test/iobuf_unittest.cpp:
##########
@@ -1946,4 +1946,161 @@ TEST_F(IOBufTest, single_iobuf) {
ASSERT_TRUE(p != nullptr);
}
+// Reproduction test for https://github.com/apache/brpc/issues/3243
+//
+// release_tls_block() does not guard against a block being returned while it is
+// already the TLS list head. The assignment `b->portal_next = block_head`
+// becomes `b->portal_next = b` (self-loop). Any later traversal of the TLS
+// chain — remove_tls_block_chain() at thread exit, share_tls_block(), etc. —
+// spins forever, silently hanging the thread.
+//
+// The tests below run in a dedicated thread so the main thread's TLS is
+// unaffected and a self-loop can be detected and cleaned up safely.
+
+static void* double_return_release_tls_block_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ // Step 1: acquire a fresh block (removed from TLS or newly created).
+ butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
+ if (!b) return NULL;
+
+ // Step 2: return it to TLS — b is now block_head.
+ butil::iobuf::release_tls_block(b);
+
+ // Step 3: return the *same* block again while it is still block_head.
+ // BUG: release_tls_block executes
+ // b->portal_next = block_head // == b → self-loop!
+ // block_head = b // no-op
+ butil::iobuf::release_tls_block(b);
+
+ // Detect the self-loop.
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ if (head && butil::iobuf::get_portal_next(head) == head) {
+ *self_loop = true;
+ // Break the cycle so thread_atexit(remove_tls_block_chain) won't hang.
+ // acquire_tls_block() sets portal_next = NULL, breaking the loop.
+ butil::iobuf::acquire_tls_block();
+ // block_head is still b (was self-referencing), acquire again to drain.
+ butil::iobuf::acquire_tls_block();
+ // TLS is now empty — thread can exit without hanging.
+ } else {
+ butil::iobuf::remove_tls_block_chain();
+ }
+ return NULL;
+}
+
+TEST_F(IOBufTest, release_tls_block_double_return_creates_self_loop) {
+ bool self_loop = false;
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, NULL,
+ double_return_release_tls_block_thread,
+ &self_loop));
+ ASSERT_EQ(0, pthread_join(tid, NULL));
+
+ EXPECT_FALSE(self_loop)
+ << "release_tls_block() created a self-loop (b->portal_next == b) "
+ "when the same block was double-returned while already being the "
+ "TLS block_head. This causes remove_tls_block_chain() to loop "
+ "infinitely at thread exit. (GitHub issue #3243)";
+}
+
+static void* double_return_release_tls_block_chain_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ // Acquire a block and put it back as TLS head.
+ butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
+ if (!b) return NULL;
+ butil::iobuf::release_tls_block(b);
+ // Now: block_head -> b -> NULL
+
+ // Return the same block through release_tls_block_chain.
+ // The chain is: b -> NULL (b->portal_next was set to NULL by acquire).
+ // Internally: last_b = b, then last_b->portal_next = block_head = b
+ // → self-loop!
+ b->u.portal_next = NULL; // simulate a single-block chain
+ butil::iobuf::release_tls_block_chain(b);
+
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ if (head && butil::iobuf::get_portal_next(head) == head) {
+ *self_loop = true;
+ butil::iobuf::acquire_tls_block();
+ butil::iobuf::acquire_tls_block();
Review Comment:
Same leak issue here: the cycle-breaking calls to `acquire_tls_block()` discard the returned blocks without releasing their references, which can leak memory when the bug reproduces and trip the test suite's leak checks. Store the returned pointers and `dec_ref()` them after draining.
##########
src/butil/iobuf.cpp:
##########
@@ -334,18 +334,32 @@ void release_tls_block_chain(IOBuf::Block* b) {
g_num_hit_tls_threshold.fetch_add(n, butil::memory_order_relaxed);
return;
}
+ IOBuf::Block* const old_head = tls_data.block_head;
Review Comment:
This overlap protection is bypassed when `tls_data.num_blocks >= max_blocks_per_thread()`: the function immediately `dec_ref()`s every node in the incoming chain. If the incoming chain overlaps with the existing TLS list (the class of bug this change is addressing), that branch would `dec_ref()` blocks still referenced by TLS, leaving dangling pointers in the TLS free list. Overlap detection (or a safe handling strategy) should be applied before the threshold branch as well.
##########
test/iobuf_unittest.cpp:
##########
@@ -1946,4 +1946,161 @@ TEST_F(IOBufTest, single_iobuf) {
ASSERT_TRUE(p != nullptr);
}
+// Reproduction test for https://github.com/apache/brpc/issues/3243
+//
+// release_tls_block() does not guard against a block being returned while it is
+// already the TLS list head. The assignment `b->portal_next = block_head`
+// becomes `b->portal_next = b` (self-loop). Any later traversal of the TLS
+// chain — remove_tls_block_chain() at thread exit, share_tls_block(), etc. —
+// spins forever, silently hanging the thread.
+//
+// The tests below run in a dedicated thread so the main thread's TLS is
+// unaffected and a self-loop can be detected and cleaned up safely.
+
+static void* double_return_release_tls_block_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ // Step 1: acquire a fresh block (removed from TLS or newly created).
+ butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
+ if (!b) return NULL;
+
+ // Step 2: return it to TLS — b is now block_head.
+ butil::iobuf::release_tls_block(b);
+
+ // Step 3: return the *same* block again while it is still block_head.
+ // BUG: release_tls_block executes
+ // b->portal_next = block_head // == b → self-loop!
+ // block_head = b // no-op
+ butil::iobuf::release_tls_block(b);
+
+ // Detect the self-loop.
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ if (head && butil::iobuf::get_portal_next(head) == head) {
+ *self_loop = true;
+ // Break the cycle so thread_atexit(remove_tls_block_chain) won't hang.
+ // acquire_tls_block() sets portal_next = NULL, breaking the loop.
+ butil::iobuf::acquire_tls_block();
+ // block_head is still b (was self-referencing), acquire again to drain.
+ butil::iobuf::acquire_tls_block();
+ // TLS is now empty — thread can exit without hanging.
+ } else {
+ butil::iobuf::remove_tls_block_chain();
+ }
+ return NULL;
+}
+
+TEST_F(IOBufTest, release_tls_block_double_return_creates_self_loop) {
+ bool self_loop = false;
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, NULL,
+ double_return_release_tls_block_thread,
+ &self_loop));
+ ASSERT_EQ(0, pthread_join(tid, NULL));
+
+ EXPECT_FALSE(self_loop)
+ << "release_tls_block() created a self-loop (b->portal_next == b) "
+ "when the same block was double-returned while already being the "
+ "TLS block_head. This causes remove_tls_block_chain() to loop "
+ "infinitely at thread exit. (GitHub issue #3243)";
+}
+
+static void* double_return_release_tls_block_chain_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ // Acquire a block and put it back as TLS head.
+ butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
+ if (!b) return NULL;
+ butil::iobuf::release_tls_block(b);
+ // Now: block_head -> b -> NULL
+
+ // Return the same block through release_tls_block_chain.
+ // The chain is: b -> NULL (b->portal_next was set to NULL by acquire).
+ // Internally: last_b = b, then last_b->portal_next = block_head = b
+ // → self-loop!
+ b->u.portal_next = NULL; // simulate a single-block chain
+ butil::iobuf::release_tls_block_chain(b);
+
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ if (head && butil::iobuf::get_portal_next(head) == head) {
+ *self_loop = true;
+ butil::iobuf::acquire_tls_block();
+ butil::iobuf::acquire_tls_block();
+ } else {
+ butil::iobuf::remove_tls_block_chain();
+ }
+ return NULL;
+}
+
+TEST_F(IOBufTest, release_tls_block_chain_overlap_creates_self_loop) {
+ bool self_loop = false;
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, NULL,
+ double_return_release_tls_block_chain_thread,
+ &self_loop));
+ ASSERT_EQ(0, pthread_join(tid, NULL));
Review Comment:
Same naming mismatch here: the test name implies the self-loop is created, but the test asserts that it should not happen. Consider renaming to match the expected outcome/regression intent.
##########
test/iobuf_unittest.cpp:
##########
@@ -1946,4 +1946,161 @@ TEST_F(IOBufTest, single_iobuf) {
ASSERT_TRUE(p != nullptr);
}
+// Reproduction test for https://github.com/apache/brpc/issues/3243
+//
+// release_tls_block() does not guard against a block being returned while it is
+// already the TLS list head. The assignment `b->portal_next = block_head`
+// becomes `b->portal_next = b` (self-loop). Any later traversal of the TLS
+// chain — remove_tls_block_chain() at thread exit, share_tls_block(), etc. —
+// spins forever, silently hanging the thread.
+//
+// The tests below run in a dedicated thread so the main thread's TLS is
+// unaffected and a self-loop can be detected and cleaned up safely.
+
+static void* double_return_release_tls_block_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ // Step 1: acquire a fresh block (removed from TLS or newly created).
+ butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
+ if (!b) return NULL;
+
+ // Step 2: return it to TLS — b is now block_head.
+ butil::iobuf::release_tls_block(b);
+
+ // Step 3: return the *same* block again while it is still block_head.
+ // BUG: release_tls_block executes
+ // b->portal_next = block_head // == b → self-loop!
+ // block_head = b // no-op
+ butil::iobuf::release_tls_block(b);
+
+ // Detect the self-loop.
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ if (head && butil::iobuf::get_portal_next(head) == head) {
+ *self_loop = true;
+ // Break the cycle so thread_atexit(remove_tls_block_chain) won't hang.
+ // acquire_tls_block() sets portal_next = NULL, breaking the loop.
+ butil::iobuf::acquire_tls_block();
+ // block_head is still b (was self-referencing), acquire again to drain.
+ butil::iobuf::acquire_tls_block();
+ // TLS is now empty — thread can exit without hanging.
+ } else {
+ butil::iobuf::remove_tls_block_chain();
+ }
+ return NULL;
+}
+
+TEST_F(IOBufTest, release_tls_block_double_return_creates_self_loop) {
+ bool self_loop = false;
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, NULL,
+ double_return_release_tls_block_thread,
+ &self_loop));
+ ASSERT_EQ(0, pthread_join(tid, NULL));
+
+ EXPECT_FALSE(self_loop)
+ << "release_tls_block() created a self-loop (b->portal_next == b) "
+ "when the same block was double-returned while already being the "
+ "TLS block_head. This causes remove_tls_block_chain() to loop "
+ "infinitely at thread exit. (GitHub issue #3243)";
+}
+
+static void* double_return_release_tls_block_chain_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ // Acquire a block and put it back as TLS head.
+ butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
+ if (!b) return NULL;
+ butil::iobuf::release_tls_block(b);
+ // Now: block_head -> b -> NULL
+
+ // Return the same block through release_tls_block_chain.
+ // The chain is: b -> NULL (b->portal_next was set to NULL by acquire).
+ // Internally: last_b = b, then last_b->portal_next = block_head = b
+ // → self-loop!
+ b->u.portal_next = NULL; // simulate a single-block chain
+ butil::iobuf::release_tls_block_chain(b);
+
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ if (head && butil::iobuf::get_portal_next(head) == head) {
+ *self_loop = true;
+ butil::iobuf::acquire_tls_block();
+ butil::iobuf::acquire_tls_block();
+ } else {
+ butil::iobuf::remove_tls_block_chain();
+ }
+ return NULL;
+}
+
+TEST_F(IOBufTest, release_tls_block_chain_overlap_creates_self_loop) {
+ bool self_loop = false;
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, NULL,
+ double_return_release_tls_block_chain_thread,
+ &self_loop));
+ ASSERT_EQ(0, pthread_join(tid, NULL));
+
+ EXPECT_FALSE(self_loop)
+ << "release_tls_block_chain() created a self-loop when the returned "
+ "chain overlaps with the existing TLS block_head. "
+ "last_b->portal_next = block_head creates a cycle when first_b == "
+ "block_head. (GitHub issue #3243)";
+}
+
+// Reproduce the self-loop through the IOBufAsZeroCopyOutputStream::BackUp()
+// code path described in the issue. BackUp() eagerly returns _cur_block to
+// TLS. If the same block pointer is subsequently released again (e.g. via
+// _release_block()), the double-return triggers the self-loop.
+static void* backup_double_return_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ butil::IOBuf buf;
+ {
+ // _block_size == 0 → uses TLS blocks.
+ butil::IOBufAsZeroCopyOutputStream stream(&buf);
+ void* data = NULL;
+ int size = 0;
+ EXPECT_TRUE(stream.Next(&data, &size));
+
+ // BackUp releases _cur_block to TLS and sets _cur_block = NULL.
+ stream.BackUp(size);
+
+ // At this point the block is the TLS head.
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ EXPECT_TRUE(head != NULL);
+
+ // Simulate a second release of the same block, which can happen
+ // when the block pointer is obtained from a still-live BlockRef.
+ butil::iobuf::release_tls_block(head);
+
+ butil::IOBuf::Block* new_head = butil::iobuf::get_tls_block_head();
+ if (new_head && butil::iobuf::get_portal_next(new_head) == new_head) {
+ *self_loop = true;
+ butil::iobuf::acquire_tls_block();
+ butil::iobuf::acquire_tls_block();
+ }
+ }
+ if (!*self_loop) {
+ butil::iobuf::remove_tls_block_chain();
+ }
+ return NULL;
+}
+
+TEST_F(IOBufTest, backup_then_double_release_creates_self_loop) {
+ bool self_loop = false;
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, NULL,
+ backup_double_return_thread, &self_loop));
+ ASSERT_EQ(0, pthread_join(tid, NULL));
+
+ EXPECT_FALSE(self_loop)
+ << "After IOBufAsZeroCopyOutputStream::BackUp() returned a block to "
Review Comment:
Same naming mismatch here: the test name implies the self-loop is created, but it asserts `EXPECT_FALSE(self_loop)`. Consider renaming to reflect the expected post-fix behavior (or use a regression-style name tied to #3243).
##########
src/butil/iobuf.cpp:
##########
@@ -334,18 +334,32 @@ void release_tls_block_chain(IOBuf::Block* b) {
g_num_hit_tls_threshold.fetch_add(n, butil::memory_order_relaxed);
return;
}
+ IOBuf::Block* const old_head = tls_data.block_head;
IOBuf::Block* first_b = b;
IOBuf::Block* last_b = NULL;
do {
++n;
CHECK(!b->full());
+ // If any block in the incoming chain is already the TLS block_head,
+ // linking last_b->portal_next = block_head would create a cycle:
+ // - Single-block chain [B] where B == block_head:
+ // last_b->portal_next = B, i.e. B->portal_next = B (self-loop)
+ // - Multi-block chain [A -> B] where A == block_head:
Review Comment:
The new check only compares incoming nodes against `old_head`. A duplicate return of a block that is already in the TLS chain but *not* the head can still create a cycle (e.g. TLS: `H -> X -> NULL`, releasing chain `X` would produce `H -> X -> H`). If the goal is to prevent hangs in `remove_tls_block_chain()`/`share_tls_block()`, the overlap detection needs to cover any node already in the TLS list (bounded by `MAX_BLOCKS_PER_THREAD`, so scanning the existing chain is feasible).
##########
test/iobuf_unittest.cpp:
##########
@@ -1946,4 +1946,161 @@ TEST_F(IOBufTest, single_iobuf) {
ASSERT_TRUE(p != nullptr);
}
+// Reproduction test for https://github.com/apache/brpc/issues/3243
+//
+// release_tls_block() does not guard against a block being returned while it is
+// already the TLS list head. The assignment `b->portal_next = block_head`
+// becomes `b->portal_next = b` (self-loop). Any later traversal of the TLS
+// chain — remove_tls_block_chain() at thread exit, share_tls_block(), etc. —
+// spins forever, silently hanging the thread.
+//
+// The tests below run in a dedicated thread so the main thread's TLS is
+// unaffected and a self-loop can be detected and cleaned up safely.
+
+static void* double_return_release_tls_block_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ // Step 1: acquire a fresh block (removed from TLS or newly created).
+ butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
+ if (!b) return NULL;
+
+ // Step 2: return it to TLS — b is now block_head.
+ butil::iobuf::release_tls_block(b);
+
+ // Step 3: return the *same* block again while it is still block_head.
+ // BUG: release_tls_block executes
+ // b->portal_next = block_head // == b → self-loop!
+ // block_head = b // no-op
+ butil::iobuf::release_tls_block(b);
+
+ // Detect the self-loop.
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ if (head && butil::iobuf::get_portal_next(head) == head) {
+ *self_loop = true;
+ // Break the cycle so thread_atexit(remove_tls_block_chain) won't hang.
+ // acquire_tls_block() sets portal_next = NULL, breaking the loop.
+ butil::iobuf::acquire_tls_block();
+ // block_head is still b (was self-referencing), acquire again to drain.
+ butil::iobuf::acquire_tls_block();
+ // TLS is now empty — thread can exit without hanging.
Review Comment:
In the self-loop cleanup path, `acquire_tls_block()` returns blocks removed from TLS ownership, but the returned pointers are discarded without `dec_ref()`. If the reproduction triggers (or under debug allocator/sanitizers), this can leak blocks and cause unrelated test failures. Capture the returned blocks and `dec_ref()` them after breaking the cycle (or use an equivalent cleanup that preserves reference counting correctness).
##########
test/iobuf_unittest.cpp:
##########
@@ -1946,4 +1946,161 @@ TEST_F(IOBufTest, single_iobuf) {
ASSERT_TRUE(p != nullptr);
}
+// Reproduction test for https://github.com/apache/brpc/issues/3243
+//
+// release_tls_block() does not guard against a block being returned while it is
+// already the TLS list head. The assignment `b->portal_next = block_head`
+// becomes `b->portal_next = b` (self-loop). Any later traversal of the TLS
+// chain — remove_tls_block_chain() at thread exit, share_tls_block(), etc. —
+// spins forever, silently hanging the thread.
+//
+// The tests below run in a dedicated thread so the main thread's TLS is
+// unaffected and a self-loop can be detected and cleaned up safely.
+
+static void* double_return_release_tls_block_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ // Step 1: acquire a fresh block (removed from TLS or newly created).
+ butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
+ if (!b) return NULL;
+
+ // Step 2: return it to TLS — b is now block_head.
+ butil::iobuf::release_tls_block(b);
+
+ // Step 3: return the *same* block again while it is still block_head.
+ // BUG: release_tls_block executes
+ // b->portal_next = block_head // == b → self-loop!
+ // block_head = b // no-op
+ butil::iobuf::release_tls_block(b);
+
+ // Detect the self-loop.
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ if (head && butil::iobuf::get_portal_next(head) == head) {
+ *self_loop = true;
+ // Break the cycle so thread_atexit(remove_tls_block_chain) won't hang.
+ // acquire_tls_block() sets portal_next = NULL, breaking the loop.
+ butil::iobuf::acquire_tls_block();
+ // block_head is still b (was self-referencing), acquire again to drain.
+ butil::iobuf::acquire_tls_block();
+ // TLS is now empty — thread can exit without hanging.
+ } else {
+ butil::iobuf::remove_tls_block_chain();
+ }
+ return NULL;
+}
+
+TEST_F(IOBufTest, release_tls_block_double_return_creates_self_loop) {
+ bool self_loop = false;
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, NULL,
+ double_return_release_tls_block_thread,
+ &self_loop));
+ ASSERT_EQ(0, pthread_join(tid, NULL));
Review Comment:
The test name says "creates_self_loop" but the assertion expects `self_loop` to be false. Renaming to reflect the expected post-fix behavior (e.g. "...does_not_create_self_loop" or a "regression_3243" style name) would make failures easier to interpret.
##########
src/butil/iobuf_inl.h:
##########
@@ -615,6 +615,14 @@ inline void release_tls_block(IOBuf::Block* b) {
b->dec_ref();
// g_num_hit_tls_threshold.fetch_add(1, butil::memory_order_relaxed);
inc_g_num_hit_tls_threshold();
+ } else if (b == tls_data->block_head) {
Review Comment:
The duplicate-return guard only checks `b == tls_data->block_head` and it runs *after* the `num_blocks >= max_blocks_per_thread()` branch. If the TLS cache is already at the soft limit and a block is double-returned (or otherwise already present in the TLS list), this path will call `b->dec_ref()` even though TLS still holds a reference to `b`, leaving a dangling pointer in the TLS list. Consider checking whether `b` is already in the TLS chain (not just head) before any `dec_ref()`/insertion logic (the list is bounded by `MAX_BLOCKS_PER_THREAD`, so a linear scan is cheap).
##########
test/iobuf_unittest.cpp:
##########
@@ -1946,4 +1946,161 @@ TEST_F(IOBufTest, single_iobuf) {
ASSERT_TRUE(p != nullptr);
}
+// Reproduction test for https://github.com/apache/brpc/issues/3243
+//
+// release_tls_block() does not guard against a block being returned while it is
+// already the TLS list head. The assignment `b->portal_next = block_head`
+// becomes `b->portal_next = b` (self-loop). Any later traversal of the TLS
+// chain — remove_tls_block_chain() at thread exit, share_tls_block(), etc. —
+// spins forever, silently hanging the thread.
+//
+// The tests below run in a dedicated thread so the main thread's TLS is
+// unaffected and a self-loop can be detected and cleaned up safely.
+
+static void* double_return_release_tls_block_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ // Step 1: acquire a fresh block (removed from TLS or newly created).
+ butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
+ if (!b) return NULL;
+
+ // Step 2: return it to TLS — b is now block_head.
+ butil::iobuf::release_tls_block(b);
+
+ // Step 3: return the *same* block again while it is still block_head.
+ // BUG: release_tls_block executes
+ // b->portal_next = block_head // == b → self-loop!
+ // block_head = b // no-op
+ butil::iobuf::release_tls_block(b);
+
+ // Detect the self-loop.
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ if (head && butil::iobuf::get_portal_next(head) == head) {
+ *self_loop = true;
+ // Break the cycle so thread_atexit(remove_tls_block_chain) won't hang.
+ // acquire_tls_block() sets portal_next = NULL, breaking the loop.
+ butil::iobuf::acquire_tls_block();
+ // block_head is still b (was self-referencing), acquire again to drain.
+ butil::iobuf::acquire_tls_block();
+ // TLS is now empty — thread can exit without hanging.
+ } else {
+ butil::iobuf::remove_tls_block_chain();
+ }
+ return NULL;
+}
+
+TEST_F(IOBufTest, release_tls_block_double_return_creates_self_loop) {
+ bool self_loop = false;
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, NULL,
+ double_return_release_tls_block_thread,
+ &self_loop));
+ ASSERT_EQ(0, pthread_join(tid, NULL));
+
+ EXPECT_FALSE(self_loop)
+ << "release_tls_block() created a self-loop (b->portal_next == b) "
+ "when the same block was double-returned while already being the "
+ "TLS block_head. This causes remove_tls_block_chain() to loop "
+ "infinitely at thread exit. (GitHub issue #3243)";
+}
+
+static void* double_return_release_tls_block_chain_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ // Acquire a block and put it back as TLS head.
+ butil::IOBuf::Block* b = butil::iobuf::acquire_tls_block();
+ if (!b) return NULL;
+ butil::iobuf::release_tls_block(b);
+ // Now: block_head -> b -> NULL
+
+ // Return the same block through release_tls_block_chain.
+ // The chain is: b -> NULL (b->portal_next was set to NULL by acquire).
+ // Internally: last_b = b, then last_b->portal_next = block_head = b
+ // → self-loop!
+ b->u.portal_next = NULL; // simulate a single-block chain
+ butil::iobuf::release_tls_block_chain(b);
+
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ if (head && butil::iobuf::get_portal_next(head) == head) {
+ *self_loop = true;
+ butil::iobuf::acquire_tls_block();
+ butil::iobuf::acquire_tls_block();
+ } else {
+ butil::iobuf::remove_tls_block_chain();
+ }
+ return NULL;
+}
+
+TEST_F(IOBufTest, release_tls_block_chain_overlap_creates_self_loop) {
+ bool self_loop = false;
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, NULL,
+ double_return_release_tls_block_chain_thread,
+ &self_loop));
+ ASSERT_EQ(0, pthread_join(tid, NULL));
+
+ EXPECT_FALSE(self_loop)
+ << "release_tls_block_chain() created a self-loop when the returned "
+ "chain overlaps with the existing TLS block_head. "
+ "last_b->portal_next = block_head creates a cycle when first_b == "
+ "block_head. (GitHub issue #3243)";
+}
+
+// Reproduce the self-loop through the IOBufAsZeroCopyOutputStream::BackUp()
+// code path described in the issue. BackUp() eagerly returns _cur_block to
+// TLS. If the same block pointer is subsequently released again (e.g. via
+// _release_block()), the double-return triggers the self-loop.
+static void* backup_double_return_thread(void* arg) {
+ bool* self_loop = static_cast<bool*>(arg);
+ butil::iobuf::remove_tls_block_chain();
+
+ butil::IOBuf buf;
+ {
+ // _block_size == 0 → uses TLS blocks.
+ butil::IOBufAsZeroCopyOutputStream stream(&buf);
+ void* data = NULL;
+ int size = 0;
+ EXPECT_TRUE(stream.Next(&data, &size));
+
+ // BackUp releases _cur_block to TLS and sets _cur_block = NULL.
+ stream.BackUp(size);
+
+ // At this point the block is the TLS head.
+ butil::IOBuf::Block* head = butil::iobuf::get_tls_block_head();
+ EXPECT_TRUE(head != NULL);
+
+ // Simulate a second release of the same block, which can happen
+ // when the block pointer is obtained from a still-live BlockRef.
+ butil::iobuf::release_tls_block(head);
+
+ butil::IOBuf::Block* new_head = butil::iobuf::get_tls_block_head();
+ if (new_head && butil::iobuf::get_portal_next(new_head) == new_head) {
+ *self_loop = true;
+ butil::iobuf::acquire_tls_block();
+ butil::iobuf::acquire_tls_block();
Review Comment:
Same leak issue here: if a cycle is detected, the `acquire_tls_block()` calls used to break it drop the returned blocks on the floor without `dec_ref()`, which can leak blocks and cause follow-on failures in the suite’s leak checks. Capture and release the returned blocks after draining.
--
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 IOBuf TLS block pool self-loop caused by double-return (#3243) (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3244:
URL: https://github.com/apache/brpc/pull/3244#issuecomment-4071902479
Normally, this kind of problem is handled by adding a dummy element to the linked list during initialization. When there are no elements, the head pointer points to the dummy element. Checking for null is done by checking if `head == dummy->next`.
Would this implementation be better?
--
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 IOBuf TLS block pool self-loop caused by double-return (#3243) (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3244:
URL: https://github.com/apache/brpc/pull/3244#issuecomment-4159789557
I think the current approach is too complex for maintaining, and it increase the computational cost: `release_tls_block` from O(1) to O(N), `release_tls_block_chain` from O(M) to O(M*N). where N is the existing tls block number, M is the existing block number in the block_chain.
Why will this happen: "double release_tls_block() of the same block" ? This may indicate bugs in the upper-layer code. I suggest you to dig into this to find the root-cause, other than work around in the `release_tls_block` code.
--
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 IOBuf TLS block pool self-loop caused by double-return (#3243) (brpc)
Posted by "walterzhaoJR (via GitHub)" <gi...@apache.org>.
walterzhaoJR commented on PR #3244:
URL: https://github.com/apache/brpc/pull/3244#issuecomment-4088038988
For the latest failed use case, I tested it locally multiple times and the observation was that it passed.
<img width="537" height="148" alt="image" src="https://github.com/user-attachments/assets/80e09edc-5fe2-45de-bdfd-8bc039f28da3" />
--
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 IOBuf TLS block pool self-loop caused by double-return (#3243) (brpc)
Posted by "walterzhaoJR (via GitHub)" <gi...@apache.org>.
walterzhaoJR commented on PR #3244:
URL: https://github.com/apache/brpc/pull/3244#issuecomment-4108439188
Compress multiple commits into a single commit through rebase
--
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 IOBuf TLS block pool self-loop caused by double-return (#3243) (brpc)
Posted by "walterzhaoJR (via GitHub)" <gi...@apache.org>.
walterzhaoJR commented on PR #3244:
URL: https://github.com/apache/brpc/pull/3244#issuecomment-4072717699
> Normally, this kind of problem is handled by adding a dummy element to the linked list during initialization. When there are no elements, the head pointer points to the dummy element. Checking for null is done by checking if `head == dummy->next`.
>
> Would this implementation be better?
Thank you for the suggestion.
I agree that using a dummy/sentinel node is a common way to simplify linked-list handling, especially for empty-list and head-update cases.
For this bug, my understanding is that the main issue is not only the null-head case, but that the same Block may be returned to the TLS free list more than once, or that the returned chain may overlap with blocks already in the TLS list. In such cases, even with a sentinel node, I think we would still need an explicit check to prevent duplicate/overlapping insertion; otherwise the list could still become cyclic.
So I think a sentinel-based implementation could be a reasonable cleanup or refactoring, but I am not sure it would fully address this bug by itself. That is why my current fix focuses first on guarding against duplicate/overlapping returns.
I may be missing something here, so if you think the sentinel approach would help make this safer or cleaner overall, I would be happy to learn more and discuss it further.
--
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