diff options
| author | Bitflash <btf01@proton.me> | 2026-08-01 21:31:58 -0300 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2026-08-01 21:31:58 -0300 |
| commit | fc071066c9de259795a210e449b364ac126416ad (patch) | |
| tree | e9f9b98f12f9eae51ed7f91b82036bf53207429b /src/net.h | |
| parent | 5358e2396ca6cb371400f685181ef420dcf12382 (diff) | |
net: stop closing a peer's socket twice (#103)
CNode::Disconnect() closed the socket and left the handle number in the object; ~CNode() then closed it again with a raw closesocket() the accounting never saw. There is no assignment of INVALID_SOCKET to CNode::hSocket anywhere in this tree, so the destructor's guard never fired. Between the two closes the OS can reissue that number, and the second close then takes down somebody else's connection. The same omission left a closed handle in the select() set, which fails every iteration and drives the recovery path that prints 'dropping node: its socket is no longer valid'. Measured A/B on the real network at matched socket volume: unpatched 552 sockets, 4 closes failing with 10038, 741 invalid-socket drops (all one peer), 11 peers held; patched 547 sockets, 0, 0, and 21 peers held. No protocol change.
Diffstat (limited to 'src/net.h')
| -rw-r--r-- | src/net.h | 18 |
1 files changed, 17 insertions, 1 deletions
| @@ -638,10 +638,26 @@ public: | |||
| 638 | PushMessage("version", VERSION, nLocalServices, nTime, addr, nBestHeight); | 638 | PushMessage("version", VERSION, nLocalServices, nTime, addr, nBestHeight); |
| 639 | } | 639 | } |
| 640 | 640 | ||
| 641 | // Disconnect() closes the socket and sets this to INVALID_SOCKET, so by the | ||
| 642 | // time a node is deleted there is normally nothing left to do here. It used | ||
| 643 | // not to: Disconnect() closed the handle and left the number in place, and | ||
| 644 | // this destructor closed that same number a second time -- with a raw | ||
| 645 | // closesocket() that the socket accounting never saw, so neither the double | ||
| 646 | // close nor the stale claim showed up in any report. | ||
| 647 | // | ||
| 648 | // A double close is not a harmless no-op. Between the two, the operating | ||
| 649 | // system is free to hand that number to a new socket -- this node opens | ||
| 650 | // hundreds a minute -- and the second close then takes down a connection | ||
| 651 | // that belongs to somebody else, which the log can only report as a peer | ||
| 652 | // vanishing for no reason. `dropping node: its socket is no longer valid` | ||
| 653 | // is what that looks like from the outside. | ||
| 641 | ~CNode() | 654 | ~CNode() |
| 642 | { | 655 | { |
| 643 | if (hSocket != INVALID_SOCKET) | 656 | if (hSocket != INVALID_SOCKET) |
| 644 | closesocket(hSocket); | 657 | { |
| 658 | BtfCloseSocket(hSocket); | ||
| 659 | hSocket = INVALID_SOCKET; | ||
| 660 | } | ||
| 645 | } | 661 | } |
| 646 | 662 | ||
| 647 | private: | 663 | private: |