diff -uNr a/bitcoin/manifest b/bitcoin/manifest --- a/bitcoin/manifest 5c6656b37a48c56e035188ad4ed3770cf897bb7457c79ef0646dd29cafa19745e684f2d2930636a78529a127c6f0c46032654764b394035564cc1319d96262f6 +++ b/bitcoin/manifest c6493a2ebcfebf531a9ef2340355d5f6638b5436a77991c54a07c1ea80fac7c1c71a4525e07af9f36346c9f0a850a35be2389650a6172fb04d6b90d76eeb919c @@ -47,3 +47,4 @@ 735223 bitcoin_response_size_limits_2 jfw getblocks: switch from nonsensical block count based limit (which didn't really limit at all) and heavy-weight yet still inexact bytes based limit to a simple conservative bound. getheaders: switch from nonsensical limit (which didn't really limit at all) to a sending buffer size derived limit to prevent flooding or at least give peer the chance to do so. 735324 bitcoin_pushmessage_cleanup_1 jfw Tame the PushMessage template explosion by pushing CDataStream serialization out to callers. Permanently disable undocumented -allowreceivebyip and otherwise dead code turned up when auditing PushMessage callers, namely generic network request tracking and publish/subscribe infrastructure. Simplify initialization of vSend/vRecv fields: type (since SER_NETWORK is the default) and version (since the indicated flag day is long past; it wasn't quite clear to remove the field altogether). Add unit test for assumption noted in one of the PushMessage calls. 735336 bitcoin_pushmessage_cleanup_2 jfw With PushMessage deduplicated, we can integrate BeginMessage, AbortMessage and EndMessage for major simplification. While we're here, fix those infernal torn send/receive debug messages with their redundant timestamps and no clue as to sender or recipient. +735393 bitcoin_enforce_buffer_limits jfw Finally we can move send and receive flood control logic to where the buffers get filled, so that the rest of the code can count on their not exceeding the size limits. Passing the error upstack is needed in the sending case to stop whatever was generating the output. While we're here, fix an erroneous fcntl flag bit inversion and kill a 64k stack buffer. diff -uNr a/bitcoin/src/main.cpp b/bitcoin/src/main.cpp --- a/bitcoin/src/main.cpp 9f194a61889c5bbd661aa84cb485ade9fc6705752e928b9230e9293851a7449d03d9859ca226f3f2c485ec8b506f11436d60123038d20551c63acf1546436284 +++ b/bitcoin/src/main.cpp ae05983cfd5fabb8bc16e6e6ce5dfd302be9a4546302837de972ea92600f296312fe4535c84da0a991bf34fbe02ee24399201b439ee7eb33f20ed1581d7c9e34 @@ -2228,14 +2228,9 @@ } else { - PrintExceptionContinue(&e, "ProcessMessage()"); + throw; } } - catch (std::exception& e) { - PrintExceptionContinue(&e, "ProcessMessage()"); - } catch (...) { - PrintExceptionContinue(NULL, "ProcessMessage()"); - } if (!fRet) printf("ProcessMessage(%s, %u bytes) FAILED\n", strCommand.c_str(), nMessageSize); diff -uNr a/bitcoin/src/net.cpp b/bitcoin/src/net.cpp --- a/bitcoin/src/net.cpp 2a3a5df4dbdaa976df3229923e229c0c1ffbc2d654b16b3953c5fcc4d8330ae2df5b66a3431d42891ec214710bbdede093c57ac0988b255ba092cafbcae94882 +++ b/bitcoin/src/net.cpp 73d3ab5906c00b2e7fc030b8664404d91ddea987ca9899dc30abda12e9e9e17966ad1abfbf5587753053231f8125e75a38f5f8d9d68c023dc92110ebfb1cd398 @@ -143,7 +143,7 @@ but we'll turn it back to blocking just in case */ fFlags = fcntl(hSocket, F_GETFL, 0); - if (fcntl(hSocket, F_SETFL, fFlags & !O_NONBLOCK) == SOCKET_ERROR) + if (fcntl(hSocket, F_SETFL, fFlags & ~O_NONBLOCK) == SOCKET_ERROR) { closesocket(hSocket); return false; @@ -667,17 +667,21 @@ CDataStream& vRecv = pnode->vRecv; unsigned int nPos = vRecv.size(); - if (nPos > ReceiveBufferSize()) { - if (!pnode->fDisconnect) - printf("socket recv flood control disconnect (%d bytes)\n", vRecv.size()); - pnode->CloseSocketDisconnect(); - } - else { + { + static CCriticalSection cs_recv_buffer; + SCOPED_LOCK(cs_recv_buffer); + // Static storage is OK because there's only one ThreadSocketHandler; the lock is just for safety in case this were to change. Large stack buffers on the other hand can be risky, especially with multithreading since address space for guard pages is more limited. // typical socket buffer is 8K-64K - char pchBuf[0x10000]; + static char pchBuf[0x10000]; int nBytes = recv(pnode->hSocket, pchBuf, sizeof(pchBuf), MSG_DONTWAIT); if (nBytes > 0) { + if ((unsigned) nBytes > ReceiveBufferSize() - nPos) { // this form rules out addition overflow + if (!pnode->fDisconnect) + printf("socket recv flood control disconnect (%d B)\n", nPos + nBytes); + pnode->CloseSocketDisconnect(); + continue; + } vRecv.resize(nPos + nBytes); memcpy(&vRecv[nPos], pchBuf, nBytes); pnode->nLastRecv = GetTime(); @@ -732,11 +736,6 @@ pnode->CloseSocketDisconnect(); } } - if (vSend.size() > SendBufferSize()) { - if (!pnode->fDisconnect) - printf("socket send flood control disconnect (%d bytes)\n", vSend.size()); - pnode->CloseSocketDisconnect(); - } } } } @@ -1004,16 +1003,40 @@ BOOST_FOREACH(CNode* pnode, vNodesCopy) { // Receive messages - TRY_CRITICAL_BLOCK(pnode->cs_vRecv) - ProcessMessages(pnode); if (fShutdown) return; + try + { + TRY_CRITICAL_BLOCK(pnode->cs_vRecv) + ProcessMessages(pnode); + } + catch (peer_disconnected_error& e) + { + printf("ProcessMessages() : peer disconnected\n"); + continue; + } + catch (std::exception& e) + { + PrintExceptionContinue(&e, "ProcessMessages()"); + } + catch (...) + { + PrintExceptionContinue(NULL, "ProcessMessages()"); + } // Send messages - TRY_CRITICAL_BLOCK(pnode->cs_vSend) - SendMessages(pnode, pnode == pnodeTrickle); if (fShutdown) return; + try + { + TRY_CRITICAL_BLOCK(pnode->cs_vSend) + SendMessages(pnode, pnode == pnodeTrickle); + } + catch (peer_disconnected_error& e) + { + printf("SendMessages() : peer disconnected\n"); + continue; + } } CRITICAL_BLOCK(cs_vNodes) diff -uNr a/bitcoin/src/net.h b/bitcoin/src/net.h --- a/bitcoin/src/net.h 24f6a340b277bf9b2cc6e13e3d5d65d6324ddd538e121a5fa05dd4c31aeb5bca41c6eaf114a483e1943c7b6f9448afdd174806e81740ad9fa92b8515fd26bbac +++ b/bitcoin/src/net.h 245392b337f179b07eb142d705f6d90e7cc376d0158c80c156eb6b7aee8adcf90e58eca4982266a7ebcae458a27e02da0e8ef54b279dcf2b1856b29b15e2222e @@ -61,6 +61,11 @@ extern int fUseProxy; extern CAddress addrProxy; +class peer_disconnected_error : public std::runtime_error +{ +public: + explicit peer_disconnected_error() : std::runtime_error("write to disconnected peer") {} +}; @@ -142,7 +147,16 @@ // Be shy and don't send version until we hear if (!fInbound) - PushVersion(); + { + try + { + PushVersion(); + } + catch (peer_disconnected_error& e) + { + // safe to ignore for this single message, and unclear that callers are prepared to handle + } + } } ~CNode() @@ -266,7 +280,12 @@ printf("dropmessages DROPPING SEND MESSAGE %s %s\n", addr.ToString().c_str(), pszCommand); return; } + SCOPED_LOCK(cs_vSend); + if (fDisconnect) + // Keeping this check inside the lock seems preferable as it assures that past this point, the peer was not disconnected due to flood control (though it may still be for other reasons as fDisconnect isn't strictly guarded by cs_vSend). + throw peer_disconnected_error(); + unsigned int nHeaderStart = vSend.size(); try { @@ -274,6 +293,7 @@ uint256 hash = Hash(payload.begin(), payload.end()); memcpy(&(header.nChecksum), &hash, sizeof header.nChecksum); // If the peer speaks an old protocol version (<209) as indicated by vSend.nVersion, header.nChecksum is excluded from serialization (see CMessageHeader); no need for special handling here as in prior revisions. + // The following may temporarily overflow the sending buffer size limit; that's OK because we rectify it before releasing cs_vSend. vSend << header; vSend += payload; } @@ -283,6 +303,14 @@ printf("aborted send %s %s\n", addr.ToString().c_str(), pszCommand); throw; } + + if (vSend.size() > SendBufferSize()) + { + printf("socket send flood control disconnect (%d B)\n", vSend.size()); + vSend.clear(); + CloseSocketDisconnect(); + throw peer_disconnected_error(); + } } void PushGetBlocks(CBlockIndex* pindexBegin, uint256 hashEnd);