Skip to content

BUG-127 - Fix Regressions From Recent C++ Hardening Review

Summary

Fix the concrete regressions found during the follow-up review of last week's app commits. Keep the fixes local and preserve protocol compatibility.

Findings To Address

  • Shared-files stale-row liveness checks can dereference freed row pointers.
  • Upload read lifetime uses a non-atomic counter and increments after dispatch.
  • Part-file shutdown can stop with queued-but-undispatched write buffers.
  • Volatile transfer-list sorts can run too often on large lists.
  • TCP send queue rejection diagnostics read queue state after releasing the queue lock, and rejected live packets need explicit failure behavior.
  • ZIP extraction needs a per-member output budget.
  • ICMP pinger fallback needs bind-interface-safe behavior.

Deferred Investigation

  • kMaxAcceptedClientThreads == 1 in the web/REST listener is not changed by this item. We need a separate review to determine whether it is intentionally serializing accepted clients for MFC/controller lifetime reasons before raising the concurrency cap.

Acceptance Criteria

  • [x] Shared-files stale-row checks prove ownership before dereference.
  • [x] Upload read lifetime accounting is atomic, ordered before dispatch, and bounded during waits.
  • [x] Part-file write shutdown handles queued and pending write buffers.
  • [x] Volatile transfer-list sorting is throttled or dirtied, not unconditional on every presentation tick.
  • [x] TCP queue rejection captures diagnostics under lock and handles rejected live packets explicitly.
  • [x] ZIP member extraction refuses excessive output.
  • [x] ICMP pinger fallback cannot silently bypass an explicit bind interface.

Implementation Evidence

  • 8becc4cb BUG-127 harden shared files row liveness
  • Added a live-row resolver and made shared-files liveness proof use owner container pointer equality before any virtual method or RTTI dereference.
  • 61b704ff BUG-127 guard upload read lifetimes
  • Moved upload-read lifetime acquisition before ReadFile, converted the counter to interlocked access, and bounded the duplicate-completion delete wait with an intentional preserve-object fallback instead of UAF.
  • 70c1d0df BUG-127 drain queued part writes on shutdown
  • Marked queued-but-undispatched part-file writes aborted during shutdown while leaving kernel-owned pending writes on the cancel/completion path.
  • 58f62782 BUG-127 throttle volatile transfer sorts
  • Added shared refresh-sort throttling for downloads, uploads, download clients, queue clients, and known clients.
  • a2c4c2a4 BUG-127 fail peers on TCP queue overflow
  • Captured send-queue diagnostics under lock and closed peers through the normal socket error path when a transferred packet is rejected.
  • e581eb31 BUG-127 cap ZIP member extraction
  • Rejected oversized ZIP members before creating the extraction target.
  • f257969f BUG-127 block unbound ICMP fallback
  • Blocked IcmpSendEcho fallback when a resolved explicit bind interface is active because that API cannot honor the bind.

Validation Evidence

Each app commit passed:

  • git diff --check
  • python -m emule_workspace build app --variant main --config Debug --platform x64 --build-output-mode ErrorsOnly
  • python -m emule_workspace build app --variant main --config Release --platform x64 --build-output-mode ErrorsOnly

Follow-up

The web/REST listener thread cap (kMaxAcceptedClientThreads == 1) remains unchanged by request. Review it separately before changing concurrency because the single-thread setting may be protecting controller or MFC lifetime assumptions.