Fix IOBuf TLS block pool self-loop caused by double-return (#3243)

[walterzhaoJR]

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.

[Copilot]

Pull request overview

Fixes a TLS IOBuf block-pool corruption case where double-returning a block can create a cyclic free-list and hang the thread on later traversal (notably at thread exit cleanup).

Changes:

• Add a guard in release_tls_block() intended to prevent re-inserting the TLS head and forming a self-loop.
• Add an overlap check in release_tls_block_chain() intended to prevent linking a chain that would create a cycle and to keep num_blocks consistent.
• Add three unit tests reproducing the reported hang scenarios from issue #3243.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

File	Description
src/butil/iobuf_inl.h	Adds a duplicate-return guard in release_tls_block() to prevent self-looping the TLS free list.
src/butil/iobuf.cpp	Adds an overlap check and uses a saved old_head when returning a block chain to TLS.
test/iobuf_unittest.cpp	Adds regression tests that attempt to reproduce and detect the TLS free-list cycle described in #3243.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[yanglimingcn]

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?

[walterzhaoJR]

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.

[yanglimingcn]

Some unit tests failed, which is probably related to this. Can you open and view the failed case?

[yanglimingcn]

@copilot Check if the same vulnerability exists again, or if there is a better solution.

[Copilot]

Pull request overview

This PR fixes a TLS free-list corruption bug in butil::IOBuf where returning the same block to the per-thread cache multiple times could form cycles (including a self-loop), potentially causing infinite spins during later traversal (e.g., at thread exit). It also adds regression tests to reproduce the hang scenarios described in issue #3243.

Changes:

• Add a TLS-list membership helper and use it to prevent duplicate insertion in release_tls_block().
• Update release_tls_block_chain() to detect overlap with the existing TLS list, avoid creating cycles, and keep num_blocks consistent.
• Add multiple regression unit tests that exercise the bug via direct double-release, overlapping chains, and IOBufAsZeroCopyOutputStream::BackUp().

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/butil/iobuf_inl.h	Adds TLS-chain membership helper and uses it to guard release_tls_block() against duplicate returns.
src/butil/iobuf.cpp	Hardens release_tls_block_chain() against overlap/cycles and adjusts accounting when TLS is at capacity.
test/iobuf_unittest.cpp	Adds regression tests (run in a dedicated pthread) to detect cycles and validate TLS block counts.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[walterzhaoJR]

Some unit tests failed, which is probably related to this. Can you open and view the failed case?

Of course, I also noticed the related failure and resubmitted the fix. Now I will focus on the ai review situation.

[walterzhaoJR]

For the latest failed use case, I tested it locally multiple times and the observation was that it passed.