Skip to content

fix: replace manual destructor calls in constructors with explicit cleanup - #114

Open
JoeSergen wants to merge 2 commits into
DKapture:mainfrom
JoeSergen:fix/manual-dtor-in-ctor
Open

fix: replace manual destructor calls in constructors with explicit cleanup#114
JoeSergen wants to merge 2 commits into
DKapture:mainfrom
JoeSergen:fix/manual-dtor-in-ctor

Conversation

@JoeSergen

@JoeSergen JoeSergen commented Jul 26, 2026

Copy link
Copy Markdown

Summary

Replace this->~ClassName() calls in constructor catch blocks with explicit member cleanup. 5 call sites across 3 files.

Root Cause

The Pattern (all 3 constructors)

Calling a destructor from a constructor is fragile: if the destructor is later modified to use members that haven't been initialized yet, undefined behavior follows.

The Concrete Crash Bug (RingBuffer Normal constructor)

In the Normal constructor, type = RING_BUF_TYPE_NORMAL was set after the first try block. The destructor dispatches on type:

// Destructor — line 150
RingBuffer::~RingBuffer()
{
    if (type == RING_BUF_TYPE_NORMAL)
    {
        if (spinlock)   { delete spinlock; }    // null-safe
        if (mirror_shm) { delete mirror_shm; }
        if (shm_ctl)    { delete shm_ctl; }
    }
    else  // type == 0 (BPF) — WRONG PATH taken before this fix
    {
        if ((ulong)comsumer_index > 0)   // garbage pointer dereference
            munmap((void *)comsumer_index, page_size);
        if ((ulong)producer_index > 0)   // garbage
            munmap((void *)producer_index, page_size + 2 * bsz);
        if (epoll_fd > 0)                // garbage
            close(epoll_fd);
    }
}

Before this fix:

RingBuffer::RingBuffer(size_t bsz) : mirror_shm(nullptr)
{
    try { ... }
    catch (...) {
        this->~RingBuffer();  // type is still 0 → takes BPF path → CRASH
        throw;
    }
    type = RING_BUF_TYPE_NORMAL;  // assigned too late!
    try { ... }
    catch (...) { this->~RingBuffer(); }  // type is now 3, OK
}

If new SharedMemory() succeeds but new SpinLock() fails, comsumer_index points to a valid shared-memory address. (ulong)comsumer_index > 0 evaluates to TRUE, and munmap() is called on a kernel pointer → SIGSEGV.

Changes

1. ring-buffer.cpp — BPF constructor err_out label

Replaced this->~RingBuffer() with explicit cleanup using proper MAP_FAILED checks (instead of fragile (ulong)ptr > 0):

err_out:
    if (producer_index && producer_index != MAP_FAILED) { munmap(...); }
    if (comsumer_index && comsumer_index != MAP_FAILED) { munmap(...); }
    if (epoll_fd >= 0)                                  { close(epoll_fd); }
    throw exc;

2. ring-buffer.cpp — Normal constructor

  • Moved type = RING_BUF_TYPE_NORMAL; before the first try block (fixes the crash)
  • Both catch blocks use SAFE_DELETE on heap-allocated members only
RingBuffer::RingBuffer(size_t bsz) : mirror_shm(nullptr)
{
    type = RING_BUF_TYPE_NORMAL;  // ← moved here
    try {
        shm_ctl = new SharedMemory();
        spinlock = new SpinLock(&shm_ctl->ring_buffer_lock);
        ...
    } catch (...) {
        SAFE_DELETE(spinlock);
        SAFE_DELETE(shm_ctl);
        throw;
    }
    try {
        mirror_shm = new MirrorMemory(bsz, key);
    } catch (...) {
        SAFE_DELETE(spinlock);
        SAFE_DELETE(shm_ctl);
        throw;
    }
    ...
}

3. bpf-manager.cpp

catch (...) {
    SAFE_DELETE(m_bpf_lock);
    SAFE_DELETE(m_shm);
    throw;
}

4. data-map.cpp

catch (...) {
    SAFE_DELETE(m_bpf_rb);   // reverse init order
    SAFE_DELETE(m_lock);
    SAFE_DELETE(m_rb);
    SAFE_DELETE(m_bpf);
    SAFE_DELETE(m_shm);
    throw;
}

Non-heap Members

These members are pointer assignments into shared memory — they point to fields inside heap-allocated objects, not independently allocated memory. They are NOT freed by cleanup:

  • bpf_ref_cnt&m_shm->bpf_ref_cnt
  • comsumer_index&shm_ctl->rdi
  • producer_index&shm_ctl->wri
  • m_entrysm_rb->buf()
  • m_idx&m_shm->data_map_idx

Verification

  • SAFE_DELETE is the project's established pattern (include/com.h:432)
  • Only heap-allocated members are cleaned up
  • Cleanup order is reverse of allocation order
  • No ABI or header changes

Closes #113

…ath argument

When fs_watch() is called with a non-null path, the else branch
incorrectly calls trace_file_init() instead of mountsnoop_init().
This is a copy-paste error from file_watch(). The correct behavior
is to initialize mountsnoop for filesystem event monitoring.

Signed-off-by: JoeSergen <jxq142857@163.com>
…eanup

Calling this->~ClassName() from constructor catch blocks is fragile
and violates C++ best practices. In RingBuffer's Normal constructor,
type was set after the first try block, causing the destructor to run
the BPF cleanup path on uninitialized data.

Changes:
- ring-buffer.cpp: replace this->~RingBuffer() with explicit munmap/close
  in BPF constructor; move type=RING_BUF_TYPE_NORMAL before try block
  and replace destructor calls with SAFE_DELETE in Normal constructor
- bpf-manager.cpp: replace this->~BPFManager() with SAFE_DELETE
- data-map.cpp: replace this->~DataMap() with SAFE_DELETE in reverse
  init order

Signed-off-by: JoeSergen <jxq142857@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: replace manual destructor calls in constructors with explicit cleanup

1 participant