Skip to content

Ring.h — three UB and correctness bugs in CRing (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 copy exactly m_nCount elements during wrapped growth.

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 uses 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 has three distinct bugs in CRing<T>: two undefined-behavior pointer values (one at construction, one after RemoveAll), a missing bounds check in operator[], and an incorrect element-count copy in SetBuffer when the ring is fully wrapped. All three were fixed on the stale branch in commit 2ee7bd7 but that commit was never merged to main.

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 rather than m_pTail arithmetic when copying wrapped content

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);

When the ring is fully wrapped (m_pHead > m_pTail), the second memcpy copies m_pTail - m_pData + 1 elements from the start of the backing buffer. If m_pTail == m_pEnd - 1 (tail is at the last slot), this copies m_nSize elements instead of the correct m_nCount - (m_pEnd - m_pHead) elements, reading past the logical content into stale backing-store entries.

Fix: use m_nCount directly rather than recomputing the split from pointers:

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 copies exactly m_nCount elements in the wrapped case

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.