Skip to content

Client socket teardown ordering — cross-link not cleared before Safe_Delete

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

CUpDownClient::~CUpDownClient() and CUpDownClient::Disconnected() call socket->Safe_Delete() without first clearing the socket->client back-pointer. Safe_Delete() defers actual deletion until the socket's event handler drains; if a socket event fires after Safe_Delete() but before the destructor completes, the socket can dereference socket->client which is in the process of being destroyed.

Current Mainline Status

Done in main via commit 499069f (BUG-020 harden client socket lifetime).

The landed port includes both planned facets:

  • teardown ordering hardening around Safe_Delete() and cross-link clearing
  • hello attach ownership handoff hardening on the accepted-socket path

Two manifestations:

1. Destructor ordering (54ab77a)

// BEFORE — back-pointer still live when Safe_Delete() runs:
if (socket) {
    socket->client = NULL;   // set to NULL but not atomic with Safe_Delete
    socket->Safe_Delete();
}

socket->client = NULL runs on one thread; Safe_Delete schedules deferred deletion; a concurrent socket event on the I/O thread can read socket->client as non-NULL between these two lines.

2. Hello packet attach ownership regression (5fb72db)

CListenSocket attaches a newly accepted CClientReqSocket to a CUpDownClient without properly transferring ownership in the seam boundary, creating a window where the socket's client pointer points to an object that has not fully committed to ownership. (CPP_037)

Files affected: srchybrid/BaseClient.cpp, srchybrid/ClientList.cpp, srchybrid/ListenSocket.cpp

Experimental Reference Implementation

Status in stale-v0.72a-experimental-clean: Both facets fixed.

Teardown ordering fix (54ab77a):

Introduces DetachClientSocketPair(client, socket) helper that atomically clears both client->socket and socket->client before calling Safe_Delete():

// AFTER — both back-pointers cleared atomically before deferred delete:
CClientReqSocket *pSocket = socket;
DetachClientSocketPair(this, pSocket);
pSocket->Safe_Delete();

Applied in three call sites in BaseClient.cpp: destructor, Disconnected(), and TryToConnect() stale-socket cleanup.

Files changed: srchybrid/BaseClient.cpp (+26 lines), srchybrid/ClientList.cpp (+12 lines), srchybrid/ClientSocketLifetimeSeams.h (+43 lines seam interface)

Hello attach fix (5fb72db):

ListenSocket.cpp attach path guarded with ResourceOwnershipSeams.h to enforce single-owner semantics on socket/client handoff.

Files changed: srchybrid/ListenSocket.cpp (+7 lines), srchybrid/ResourceOwnershipSeams.h (+14 lines)

Porting Note

The DetachClientSocketPair helper is the key abstraction. Port 54ab77a first — clear socket->client and client->socket simultaneously before Safe_Delete(). The seam headers are test infrastructure; the production logic is in BaseClient.cpp. The hello attach fix (5fb72db) can follow independently.

Landed Mainline Port

eMule-main is using the staged branch port rather than the original stale-branch shape:

  • teardown ordering landed via BaseClient.cpp and ClientList.cpp, reusing ClientSocketLifetimeSeams.h::DetachClientSocketPair(...)
  • hello attach ownership landed via ListenSocket.cpp, reusing the existing ResourceOwnershipSeams.h ownership-release helpers without adding a new seam surface
  • targeted regressions were added in client_socket_lifetime.tests.cpp and resource_ownership.tests.cpp

Relationship to Existing Items

  • BUG-012 (CPartFile destructor FlushBuffer): same class of destructor-ordering bug, already fixed. BUG-020 is the client-socket equivalent.
  • REF-008 (Boost.Asio migration): a full Asio migration would eliminate Safe_Delete deferred semantics entirely; BUG-020 is still relevant until then.