Skip to content

Ring.h — CRing pointer-state, access-guard, and defensive copy cleanup (CODEREV_003, 004, 011)

Historical reference only: stale-v0.72a-experimental-clean and analysis\stale-v0.72a-experimental-clean are retired reference sources, not active branch targets or current baselines. Use them only as provenance or idea-extraction sources; landed status is determined against main. See Historical References.

Experimental Reference Implementation

Status in stale-v0.72a-experimental-clean: Fixed in commit 2ee7bd7 (WIP: add emule-tests harness and fix CRing contracts). The fix replaced the pointer-based layout (m_pHead, m_pTail, m_pEnd) entirely with an index-based approach: - m_pHead / m_pTail / m_pEnd removed; replaced with UINT_PTR m_nHead (index of oldest element) - GetPhysicalIndex(UINT_PTR index) helper added: return (m_nHead + index) % m_nSize - operator[] now ASSERT(index < m_nCount) before indexing - AddTail uses GetPhysicalIndex(m_nCount) to find the slot - RemoveHead increments m_nHead = (m_nHead + 1) % m_nSize; resets to 0 when count drops to zero - SetBuffer rewrites copy logic using the new index arithmetic — no pointer-past-end is possible - ASSERT(expression) macro guard added (falls back to <cassert> outside MFC context) for test builds

Porting note: The index-based rewrite is a clean drop-in, but main does not need it to close the currently documented defects. The preferred mainline fix is the smaller pointer-based repair: keep the existing layout, add ASSERT guards to Head(), Tail(), and operator[], make the empty sentinel one-past-end only, and derive wrapped-copy counts from m_nCount.

Current Mainline Status

Done in main via commit 0d7b0fe (BUG-007 fix CRing pointer-state contracts).

The landed fix kept the smaller pointer-backed repair strategy already documented here:

  • empty-state pointer contracts were corrected
  • bounds checks were added for indexed access
  • wrapped SetBuffer copying now derives the split from the logical element count

Regression coverage is present in repos\emulebb-build-tests\src\ring.tests.cpp for first insert after construction, restart after RemoveAll(), restart after removing the final element, logical order across wrap-around, capacity growth from a wrapped state, and invalid logical access assertions.

Summary

srchybrid/ring.h had confirmed undefined-behavior pointer values (one at construction, one after RemoveAll) plus missing logical-access guards in operator[], Head(), and Tail(). The original audit also flagged wrapped SetBuffer copying. Later review found the specific stale-copy scenario to be unproven for valid ring states, because m_pHead > m_pTail cannot coexist with m_pTail == m_pEnd - 1. The mainline fix still derives wrapped-copy counts from m_nCount, which is the clearer defensive invariant.

Reviewed Mainline Fix Choice

Review against eMule-main concluded that the smallest safe fix is sufficient here:

  • keep the current pointer-backed representation
  • use m_pEnd as the empty-tail sentinel
  • special-case the first AddTail() after construction, RemoveAll(), or removing the last element
  • reset RemoveHead() to the same empty sentinel when count drops to zero
  • use m_nCount to derive wrapped-copy counts instead of relying solely on pointer-position arithmetic

This closes the UB and correctness bugs without taking the larger index-based rewrite from the experimental branch.

Bug 1 — CODEREV_003: UB pointer in SetBuffer and RemoveAll

SetBuffer (line 105): 

m_pTail = &dst[m_nCount - 1];   // when m_nCount == 0: &dst[-1] — UB
When the ring is freshly constructed (m_nCount == 0), m_pTail is set one before the start of the allocation. C++ only permits pointers from array[0] through array[N] (one-past-end); array[-1] is undefined behavior, flagged by ASan/UBSan.

RemoveAll (line 78): 

m_pTail = m_pEnd;   // one-past-end; next AddTail does ++m_pTail → m_pEnd+1 (two-past-end) — UB
The first AddTail after RemoveAll increments the already-one-past-end pointer to two-past-end before the wraparound check executes. The wraparound never fires because m_pEnd + 1 >= m_pEnd is trivially true only after dereferencing.

In practice on x86/x64 both work by accident (wrapping arithmetic), but both are flagged by ASan/UBSan and violate the standard.

Fix chosen for main: 

// SetBuffer: empty rings keep the one-past-end sentinel
m_pTail = m_nCount > 0 ? &dst[m_nCount - 1] : m_pEnd;

// RemoveAll: reset to the same one-past-end sentinel
m_pHead = m_pData;
m_pTail = m_pEnd;

// AddTail: handle the empty sentinel before incrementing
if (m_nCount == 0) {
    m_pHead = m_pTail = m_pData;
    m_nCount = 1;
    *m_pTail = newElement;
    return;
}

Bug 2 — CODEREV_004: Missing bounds check in operator[]

Line 38: 

const TYPE& operator[](UINT_PTR index) const {
    return m_pData[(index + (m_pHead - m_pData)) % m_nSize];
}

index is not validated against m_nCount. ring[i] where i >= ring.Count() reads stale or uninitialized elements from the backing array without any diagnostic. TYPE is TransferredData (uint64 datalen + DWORD timestamp), so out-of-bounds reads produce incorrect bandwidth calculation values — silent wrong numbers, not a crash.

Fix: 

const TYPE& operator[](UINT_PTR index) const {
    ASSERT(index < m_nCount);
    return m_pData[(index + (m_pHead - m_pData)) % m_nSize];
}

Bug 3 — CODEREV_011: Wrong element count in SetBuffer when fully wrapped

Lines 96–100: 

if (m_pHead > m_pTail) {
    memcpy(dst, m_pHead, (m_pEnd - m_pHead) * sizeof TYPE);
    memcpy(&dst[m_pEnd - m_pHead], m_pData, (m_pTail - m_pData + 1) * sizeof TYPE);
} else
    memcpy(dst, m_pHead, (m_pTail - m_pHead + 1) * sizeof TYPE);

Earlier notes claimed that, when the ring is fully wrapped (m_pHead > m_pTail), m_pTail == m_pEnd - 1 could make the second memcpy copy stale backing-store entries. That specific state is not reachable through valid ring operations: m_pEnd - 1 is the highest valid slot, so no valid m_pHead can be greater than it. For valid wrapped states, the old pointer split adds up to the logical count.

The mainline code still keeps the defensive cleanup from this item: derive the tail-side count from m_nCount after computing the head-side count. That makes the invariant explicit and keeps the copy bounded by logical content even if this particular stale-copy scenario was not reproduced.

Fix: use m_nCount directly when deriving the wrapped split: 

const UINT_PTR nTail = m_nCount - (m_pEnd - m_pHead);
memcpy(dst, m_pHead, (m_pEnd - m_pHead) * sizeof(TYPE));
memcpy(dst + (m_pEnd - m_pHead), m_pData, nTail * sizeof(TYPE));

Acceptance Criteria

  • [x] SetBuffer does not produce &dst[-1] when m_nCount == 0
  • [x] RemoveAll leaves m_pTail in the same one-past-end empty sentinel state used after construction
  • [x] RemoveHead resets the ring to that same empty sentinel state when the last element is removed
  • [x] operator[] has ASSERT(index < m_nCount) guard
  • [x] Head() and Tail() assert on empty logical access
  • [x] SetBuffer derives wrapped-copy counts from m_nCount

Sanitizer Evidence

No ASan result is claimed for this item yet. The active workspace does not have an ASan build lane, and that build-policy work remains tracked by CI-006. BUG-007 is closed on the code and regression-test evidence above; CI-006 owns future sanitizer enablement and recurring ASan evidence.

Note

If REF-014 (replace CRing<T> with boost::circular_buffer) is completed first, these bugs become moot. Fix this if REF-014 is not imminent; track together otherwise.