Skip to content

Upload queue lock inversion + socket I/O result mishandling + inflate buffer aliasing

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.

Summary

Three distinct but related hardening issues found in the upload / socket I/O paths:

Current-tree status: Done in main via commit b08a63c (BUG-021 harden upload and socket paths).

The broader upload-queue exploration was intentionally split away earlier so this item could land as a narrow stabilization fix. Only the three targeted hardening slices below were merged; the broader queue/session redesign remains out of scope.

Current Mainline Status

Done in main via the consolidated commit b08a63c.

The landed port preserves the intended narrow split:

  • socket I/O result handling hardened in CEMSocket
  • upload queue retirement locking hardened without replaying the broader parked queue refactor
  • packet inflate buffer ownership hardened at the parser handoff boundary

1. Upload queue retirement lock inversion (09b3808)

CUploadQueue::RemoveFromUploadQueue() and the disk I/O thread in CUploadDiskIOThread both interact with the upload slot list. The upload queue acquires its own lock and then calls into the disk I/O thread; the disk I/O thread can call back into the upload queue while holding its own lock — classic lock inversion.

Files: srchybrid/UploadQueue.cpp (+39 lines), srchybrid/UploadDiskIOThread.cpp (+10 lines), srchybrid/UploadQueue.h (+5 lines), srchybrid/UploadQueueSeams.h (+28 lines)

2. Socket I/O result mishandling (151cb46)

CAsyncSocketEx::OnReceive / OnSend error paths and CEMSocket::Send return values are not consistently checked. Specifically: - CAsyncSocketEx can return partial or error results that CEMSocket treats as success - EMSocket.cpp Send() can return a negative value (SOCKET_ERROR) that is compared unsigned, producing silent discard rather than error propagation

Files: srchybrid/EMSocket.cpp, srchybrid/SocketIoSeams.h

3. Packet inflate buffer ownership (236dc67)

In CClientUDPSocket::ProcessPacket() and CDownloadClient::ProcessPacket(), the inflate (decompression) buffer is shared between the UDP receive path and the protocol parser. If the parser holds a pointer into the buffer and the receive path reallocates for a larger packet before the parser finishes, the pointer becomes dangling.

Additionally, DownloadClient.cpp has several packet-processing paths that assumed an exclusively-owned buffer but received an alias into a shared inflate buffer.

Files: srchybrid/ClientUDPSocket.cpp, srchybrid/DownloadClient.cpp, srchybrid/UpdownClient.h, srchybrid/CompressionBufferSeams.h

Experimental Reference Implementation

Status: All three fixed in experimental: - 09b3808 FIX: harden upload queue retirement locking - 151cb46 FIX: harden socket IO result handling - 236dc67 FIX: harden packet inflate buffer ownership

These are independent fixes; each can be ported separately.

Porting Notes

  • 09b3808: Establish a consistent lock acquisition order: disk I/O thread must never call back into UploadQueue while holding its own lock. Introduce a deferred retirement queue or reverse the callback direction.
  • 151cb46: Audit all CEMSocket::Send() call sites for sign-comparison on the return value. On current main, the live port is intentionally narrowed to CEMSocket validation because the experimental AsyncSocketEx poll-loop shape is no longer present.
  • 236dc67: The inflate buffer must be either (a) copied before being handed to the parser, or (b) reference-counted. The experimental approach copies at the hand-off boundary.

Current Mainline Port

The active branch port keeps the experimental three-step split, but adapts it to the current tree:

  • the socket-I/O port uses the existing SocketIoSeams.h helpers already present in main
  • the upload queue port adds retired-entry bookkeeping to the current broadband queue implementation rather than replaying the broader parked queue/session refactor
  • the inflate-buffer port copies at the parser handoff boundary in both the UDP Kad path and CUpDownClient::ProcessBlockPacket() / unzip()