When SetSockAddr() fails in AcceptConnection(), the code logs a warning but continues processing the connection with a default empty CService address. This means ban checks, whitelist matching, and all downstream logic operate on an invalid address.
Return early on failure instead, consistent with other error paths in the same function.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | Bortlesboat |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Concept ACK on the AcceptConnection early return — continuing with a default empty CService after SetSockAddr failure means ban checks and whitelist matching run against an invalid address, which is clearly wrong.
One question: the diff also includes a change to the ConnectNode log line (removing ConnectionTypeAsString(conn_type) from the debug log). The PR description only mentions the AcceptConnection fix. Is the logging change intentional? If so, it might be cleaner as a separate commit since the two changes are unrelated.
Concept ACK on the
AcceptConnectionearly return — continuing with a default emptyCServiceafterSetSockAddrfailure means ban checks and whitelist matching run against an invalid address, which is clearly wrong.One question: the diff also includes a change to the
ConnectNodelog line (removingConnectionTypeAsString(conn_type)from the debug log). The PR description only mentions theAcceptConnectionfix. Is the logging change intentional? If so, it might be cleaner as a separate commit since the two changes are unrelated.
the only change is in AcceptConnection()
The fix is a straightforward control flow change I made to make this consistent with other error paths in the same function (e.g. the accept() failure check just above it uses the same pattern).
0 if (!sock) {
1 const int nErr = WSAGetLastError();
2 if (nErr != WSAEWOULDBLOCK) {
3 LogInfo("socket error accept failed: %s\n", NetworkErrorString(nErr));
4 }
5 return;
6 }
So, when SetSockAddr() fails, addr stays as a default-constructed CService - which is a zeroed-out address (0.0.0.0:0). Then CreateNodeFromAcceptedSocket() is called with that zeroed addr.
Testing this code path requires triggering an incoming connection, which isn’t easily reproducible in the test framework. It can’t be explicitly tested - there’s no test for AcceptConnection() in the test suite.
This is a low risk fix, the worst case outcome is a legitimate connection gets dropped and immediately reconnects.
1744@@ -1745,9 +1745,9 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
1745 CService addr;
1746 if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr, len)) {
1747 LogWarning("Unknown socket family\n");
LogWarning here predates this PR, but since you are already touching this block, would it be worth including sockaddr.ss_family in the message to make it more actionable for anyone debugging an unexpected socket family in the wild?