[PR #383] [MERGED] Fix SimpleRingBuffer issues #404

New issue

Closed

opened 2026-10-26 09:49:57 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 09:49:57 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/deepseek-ai/3FS/pull/383
Author: @qieqieplus
Created: 1/19/2026
Status: ✅ Merged
Merged: 1/20/2026
Merged by: @SF-Zhou

Base: main ← Head: main

📝 Commits (2)

66b050b Refactor SimpleRingBuffer
a317d45 tests: fix build and avoid NodeInfoWrapper copy

📊 Changes

2 files changed (+95 additions, -68 deletions)

View changed files

📝 src/common/utils/SimpleRingBuffer.h (+48 -21)
📝 tests/mgmtd/TestMgmtdOperator.cc (+47 -47)

📄 Description

There are major issuse/ bugs in current SimpleRingBuffer implementation.

Memory leak: When the object dies, the std::vector cleans up the raw bytes, but the destructors (~T()) of the objects living in that buffer are never called.
Iterator Logic is broken: operator++ performs pos_ %= rb_->cap_, which may lead to an infinite loop.

void test_iterator_infinite_loop() {    
    hf3fs::SimpleRingBuffer<int> rb(2);
    rb.push(1); rb.push(2);  // head_=2, tail_=0
    rb.pop();                 // head_=2, tail_=1
    rb.push(3);               // head_=3, tail_=1 (wraps to index 1)
    
    // BUG: begin().pos_=1, end().pos_=3, but ++pos_ wraps via %cap_
    // Sequence: 1 -> 0 -> 1 -> 0 ... (never reaches 3)
    int count = 0;
    for (auto it = rb.begin(); it != rb.end() && count < 100; ++it, ++count)
        std::cout << *it << " ";
    std::cout << (count >= 100 ? "\n>>> FAIL: Infinite loop!\n" : "\n");
}

operator++(int) should always return a value, return a dangling reference is UB.
std::vectorstd::byte guarantees an alignment of only 1 byte. (in practical it could be 8/ 16 bytes but still not safe for all types, e.g. SIMD vectors)

The fixes:

Add destructor to properly destroy elements and prevent memory leaks
Remove incorrect modulo in iterator increment
Fix iterator post-increment operator returning type
Use std::allocator for better alignment guarantees.
Use constraints (std::constructible_from) for push/emplace
Add size() and clear() methods
Intentionally opted to not handle head_/tail_ overflow, and move semantics to keep the implementation simple as it was.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/deepseek-ai/3FS/pull/383 **Author:** [@qieqieplus](https://github.com/qieqieplus) **Created:** 1/19/2026 **Status:** ✅ Merged **Merged:** 1/20/2026 **Merged by:** [@SF-Zhou](https://github.com/SF-Zhou) **Base:** `main` ← **Head:** `main` --- ### 📝 Commits (2) - [`66b050b`](https://github.com/deepseek-ai/3FS/commit/66b050b15e0bdb8d83570275fb245dfd17219116) Refactor SimpleRingBuffer - [`a317d45`](https://github.com/deepseek-ai/3FS/commit/a317d451f801f3ad41dfbc8c931c14284288957f) tests: fix build and avoid NodeInfoWrapper copy ### 📊 Changes **2 files changed** (+95 additions, -68 deletions) <details> <summary>View changed files</summary> 📝 `src/common/utils/SimpleRingBuffer.h` (+48 -21) 📝 `tests/mgmtd/TestMgmtdOperator.cc` (+47 -47) </details> ### 📄 Description There are major issuse/ bugs in current `SimpleRingBuffer` implementation. 1) Memory leak: When the object dies, the std::vector cleans up the raw bytes, but the destructors (~T()) of the objects living in that buffer are never called. 2) Iterator Logic is broken: operator++ performs pos_ %= rb_->cap_, which may lead to an infinite loop. ```cpp void test_iterator_infinite_loop() { hf3fs::SimpleRingBuffer<int> rb(2); rb.push(1); rb.push(2); // head_=2, tail_=0 rb.pop(); // head_=2, tail_=1 rb.push(3); // head_=3, tail_=1 (wraps to index 1) // BUG: begin().pos_=1, end().pos_=3, but ++pos_ wraps via %cap_ // Sequence: 1 -> 0 -> 1 -> 0 ... (never reaches 3) int count = 0; for (auto it = rb.begin(); it != rb.end() && count < 100; ++it, ++count) std::cout << *it << " "; std::cout << (count >= 100 ? "\n>>> FAIL: Infinite loop!\n" : "\n"); } ``` 3) operator++(int) should always return a value, return a dangling reference is UB. 4) std::vector<std::byte> guarantees an alignment of only 1 byte. (in practical it could be 8/ 16 bytes but still not safe for all types, e.g. SIMD vectors) The fixes: - Add destructor to properly destroy elements and prevent memory leaks - Remove incorrect modulo in iterator increment - Fix iterator post-increment operator returning type - Use std::allocator for better alignment guarantees. - Use constraints (std::constructible_from) for push/emplace - Add size() and clear() methods - Intentionally opted to not handle head_/tail_ overflow, and move semantics to keep the implementation simple as it was. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 09:49:57 +00:00