From 1bd4d8de71feafa56eb4408ecc2a38d39909ad1e Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 18 Nov 2020 20:07:58 +0800 Subject: [PATCH 01/10] Merge #20408: CConnman: move initialization to declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 9d09132be4ff99f98ca905c342347d5f35f13350 CConnman: initialise at declaration rather than in Start() (Anthony Towns) Pull request description: Ensure nMaxOutboundTotalBytesSentInCycle and nMaxOutboundCycleStartTime are initialized even if CConnman::Start() is not called. Prevents failures in test/fuzz/connman when run under valgrind. ACKs for top commit: practicalswift: ACK 9d09132be4ff99f98ca905c342347d5f35f13350: patch looks correct! MarcoFalke: review ACK 9d09132be4ff99f98ca905c342347d5f35f13350 , checked that we call Start only once and in the same scope where connman is constructed (AppInitMain) 💸 jnewbery: Code review ACK 9d09132be4 Tree-SHA512: 1c6c893e8c616a91947a8cc295b0ba508af3ecfcdcd94cdc5f95d808cc93c6d1a71fd24dcc194dc583854e9889fb522ca8523043367fb0263370fbcab08c6aaa --- src/net.cpp | 11 ----------- src/net.h | 4 ++-- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index be3147e160..73bb9ad628 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3080,17 +3080,6 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) { Init(connOptions); - { - LOCK(cs_totalBytesRecv); - nTotalBytesRecv = 0; - } - { - LOCK(cs_totalBytesSent); - nTotalBytesSent = 0; - nMaxOutboundTotalBytesSentInCycle = 0; - nMaxOutboundCycleStartTime = 0s; - } - #ifdef USE_KQUEUE if (socketEventsMode == SOCKETEVENTS_KQUEUE) { kqueuefd = kqueue(); diff --git a/src/net.h b/src/net.h index 5f159a0959..6ca18e4ee0 100644 --- a/src/net.h +++ b/src/net.h @@ -591,8 +591,8 @@ private: uint64_t nTotalBytesSent GUARDED_BY(cs_totalBytesSent) {0}; // outbound limit & stats - uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent); - std::chrono::seconds nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent); + uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent) {0}; + std::chrono::seconds nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent) {0}; uint64_t nMaxOutboundLimit GUARDED_BY(cs_totalBytesSent); // P2P timeout in seconds From afb8f35b51bacc4e070c683b5d8e2a51f990a328 Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 16 Oct 2020 11:31:12 +0800 Subject: [PATCH 02/10] Merge #19836: rpc: Properly deserialize txs with witness before signing 33330778230961cfbf2a24de36b5877e395cc596 rpc: Adjust witness-tx deserialize error message (MarcoFalke) cccc7525697e7b8d99b545e34f0f504c78ffdb94 rpc: Properly deserialize txs with witness before signing (MarcoFalke) Pull request description: Signing a transaction can only happen when the transaction has inputs. A transaction with inputs can always be deserialized as witness-transaction. If `try_no_witness` decoding is attempted, this will lead to rare intermittent failures. Fixes #18803 ACKs for top commit: achow101: ACK 33330778230961cfbf2a24de36b5877e395cc596 ajtowns: ACK 33330778230961cfbf2a24de36b5877e395cc596 Tree-SHA512: 73f8a5cdfe03fb0e68908d2fa09752c346406f455694a020ec0dd1267ef8f0a583b8e84063ea74aac127106dd193b72623ca6d81469a94b3f5b3c766ebf2c42b --- src/rpc/mining.cpp | 2 +- src/rpc/rawtransaction.cpp | 12 ++++++------ src/wallet/rpcdump.cpp | 5 +++-- src/wallet/rpcwallet.cpp | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 072d56d6e2..1e0797cd32 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -352,7 +352,7 @@ static UniValue generateblock(const JSONRPCRequest& request) txs.push_back(MakeTransactionRef(std::move(mtx))); } else { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("Transaction decode failed for %s", str)); + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("Transaction decode failed for %s. Make sure the tx has at least one input.", str)); } } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 0e2b34d60a..5d2f8334a1 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -636,7 +636,7 @@ static UniValue combinerawtransaction(const JSONRPCRequest& request) for (unsigned int idx = 0; idx < txs.size(); idx++) { if (!DecodeHexTx(txVariants[idx], txs[idx].get_str())) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed for tx %d", idx)); + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed for tx %d. Make sure the tx has at least one input.", idx)); } } @@ -757,7 +757,7 @@ static UniValue signrawtransactionwithkey(const JSONRPCRequest& request) CMutableTransaction mtx; if (!DecodeHexTx(mtx, request.params[0].get_str())) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); } FillableSigningProvider keystore; @@ -823,10 +823,10 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) UniValue::VBOOL }); - // parse hex string from parameter CMutableTransaction mtx; - if (!DecodeHexTx(mtx, request.params[0].get_str())) - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + if (!DecodeHexTx(mtx, request.params[0].get_str())) { + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); + } CTransactionRef tx(MakeTransactionRef(std::move(mtx))); const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ? @@ -898,7 +898,7 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) CMutableTransaction mtx; if (!DecodeHexTx(mtx, request.params[0].get_array()[0].get_str())) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); } CTransactionRef tx(MakeTransactionRef(std::move(mtx))); const uint256& tx_hash = tx->GetHash(); diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index c4d227b04f..d6706fd4aa 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -298,8 +298,9 @@ UniValue importprunedfunds(const JSONRPCRequest& request) CWallet* const pwallet = wallet.get(); CMutableTransaction tx; - if (!DecodeHexTx(tx, request.params[0].get_str())) - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + if (!DecodeHexTx(tx, request.params[0].get_str())) { + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); + } uint256 hashTx = tx.GetHash(); CWalletTx wtx(pwallet, MakeTransactionRef(std::move(tx))); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 3a6609bd60..ce09ecb75c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3464,7 +3464,7 @@ UniValue signrawtransactionwithwallet(const JSONRPCRequest& request) CMutableTransaction mtx; if (!DecodeHexTx(mtx, request.params[0].get_str())) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); } // Sign the transaction From 5e20e69415d391a6cbd3278986f4a08fc8a16ba2 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 20 Nov 2020 05:34:17 +0100 Subject: [PATCH 03/10] Merge #19851: refactor: Extract ParseOpCode from ParseScript MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit c92387232f750397da7d131f262c150a608408c2 refactor: Extract ParseOpCode from ParseScript (João Barbosa) Pull request description: Seems more natural to have `mapOpNames` "hidden" in `ParseOpCode` than in `ParseScript`. A second lookup in `mapOpNames` is also removed. ACKs for top commit: laanwj: ACK c92387232f750397da7d131f262c150a608408c2 theStack: re-ACK c92387232f750397da7d131f262c150a608408c2 Tree-SHA512: d59d1964760622cf365479d44e3e676aa0bf46b60e77160140d967e012042df92121d3224c7551dc96eff5ff3294598cc6bade82adb3f60d28810e18e60e1257 --- src/core_read.cpp | 25 ++++++++++++++++--------- test/util/data/bitcoin-util-test.json | 2 +- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/core_read.cpp b/src/core_read.cpp index e1f778cab4..6aa270962d 100644 --- a/src/core_read.cpp +++ b/src/core_read.cpp @@ -17,10 +17,10 @@ #include -CScript ParseScript(const std::string& s) -{ - CScript result; +namespace { +opcodetype ParseOpCode(const std::string& s) +{ static std::map mapOpNames; if (mapOpNames.empty()) @@ -42,6 +42,17 @@ CScript ParseScript(const std::string& s) } } } + auto it = mapOpNames.find(s); + if (it == mapOpNames.end()) throw std::runtime_error("script parse error: unknown opcode"); + return it->second; +} + +} // namespace + +CScript ParseScript(const std::string& s) +{ + CScript result; + std::vector words = SplitString(s, " \t\n"); @@ -79,14 +90,10 @@ CScript ParseScript(const std::string& s) std::vector value(w->begin()+1, w->end()-1); result << value; } - else if (mapOpNames.count(*w)) - { - // opcode, e.g. OP_ADD or ADD: - result << mapOpNames[*w]; - } else { - throw std::runtime_error("script parse error"); + // opcode, e.g. OP_ADD or ADD: + result << ParseOpCode(*w); } } diff --git a/test/util/data/bitcoin-util-test.json b/test/util/data/bitcoin-util-test.json index a84b7c9ea1..db9622d371 100644 --- a/test/util/data/bitcoin-util-test.json +++ b/test/util/data/bitcoin-util-test.json @@ -202,7 +202,7 @@ { "exec": "./dash-tx", "args": ["-create", "outscript=0:123badscript"], "return_code": 1, - "error_txt": "error: script parse error", + "error_txt": "error: script parse error: unknown opcode", "description": "Create a new transaction with an invalid output script" }, { "exec": "./dash-tx", From aec81cde466b6400d57c55832781820b351af492 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 15 Mar 2021 17:36:13 +0100 Subject: [PATCH 04/10] (partial) Merge #21424: Net processing: Tidy up CNodeState ctor 6927933782acb9b158787e6f35debb916793f6b1 [net processing] Add ChainSyncTimeoutState default initializers (John Newbery) 55966e0cc03f0e380d21a9434b048d4d515b6729 [net processing] Remove CNodeState ctor body (John Newbery) Pull request description: This addresses the two outstanding review comments from #21370. ACKs for top commit: practicalswift: cr ACK 6927933782acb9b158787e6f35debb916793f6b1: patch looks correct hebasto: ACK 6927933782acb9b158787e6f35debb916793f6b1, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: b3ef5c8a096e447887df255406b3a760f01c73e2b942374595416b4b4031fc69b89cd93168c45040489d581f340b2a62d3fbabd207d4307f587c00a7a7daacd1 --- src/net_processing.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0c8ae1f490..a53d9ee917 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -589,13 +589,13 @@ struct CNodeState { */ struct ChainSyncTimeoutState { //! A timeout used for checking whether our peer has sufficiently synced - int64_t m_timeout; + int64_t m_timeout{0}; //! A header with the work we require on our peer's chain - const CBlockIndex * m_work_header; + const CBlockIndex* m_work_header{nullptr}; //! After timeout is reached, set to true after sending getheaders - bool m_sent_getheaders; + bool m_sent_getheaders{false}; //! Whether this peer is protected from disconnection due to a bad/slow chain - bool m_protect; + bool m_protect{false}; }; ChainSyncTimeoutState m_chain_sync; From 994ce0102374367adbd047ed37fdef7341e22972 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 15 May 2021 09:54:54 +0200 Subject: [PATCH 05/10] Merge bitcoin/bitcoin#21948: test: Fix off-by-one in mockscheduler test RPC fa2e614d16af84327adf1c02746d0f73e0f48111 test: Fix off-by-one in mockscheduler test RPC (MarcoFalke) Pull request description: Fixes: ``` fuzz: scheduler.cpp:83: void CScheduler::MockForward(std::chrono::seconds): Assertion `delta_seconds.count() > 0 && delta_seconds < std::chrono::hours{1}' failed. ==1059066== ERROR: libFuzzer: deadly signal #0 0x558f75449c10 in __sanitizer_print_stack_trace (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x5fec10) #1 0x558f753f32b8 in fuzzer::PrintStackTrace() fuzzer.o #2 0x558f753d68d3 in fuzzer::Fuzzer::CrashCallback() fuzzer.o #3 0x7f4a3cbbb3bf (/lib/x86_64-linux-gnu/libpthread.so.0+0x153bf) #4 0x7f4a3c7ff18a in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4618a) #5 0x7f4a3c7de858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x25858) #6 0x7f4a3c7de728 (/lib/x86_64-linux-gnu/libc.so.6+0x25728) #7 0x7f4a3c7eff35 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x36f35) #8 0x558f7588a913 in CScheduler::MockForward(std::chrono::duration >) scheduler.cpp:83:5 #9 0x558f75b0e5b1 in mockscheduler()::$_7::operator()(RPCHelpMan const&, JSONRPCRequest const&) const rpc/misc.cpp:435:30 #10 0x558f75b0e5b1 in std::_Function_handler::_M_invoke(std::_Any_data const&, RPCHelpMan const&, JSONRPCRequest const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9 #11 0x558f7587a141 in std::function::operator()(RPCHelpMan const&, JSONRPCRequest const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14 #12 0x558f7587a141 in RPCHelpMan::HandleRequest(JSONRPCRequest const&) const rpc/util.cpp:565:26 #13 0x558f756c0086 in CRPCCommand::CRPCCommand(std::__cxx11::basic_string, std::allocator >, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)::operator()(JSONRPCRequest const&, UniValue&, bool) const ./rpc/server.h:110:91 #14 0x558f756c0086 in std::_Function_handler, std::allocator >, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)>::_M_invoke(std::_Any_data const&, JSONRPCRequest const&, UniValue&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9 #15 0x558f756b8592 in std::function::operator()(JSONRPCRequest const&, UniValue&, bool) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14 #16 0x558f756b8592 in ExecuteCommand(CRPCCommand const&, JSONRPCRequest const&, UniValue&, bool) rpc/server.cpp:480:20 #17 0x558f756b8592 in ExecuteCommands(std::vector > const&, JSONRPCRequest const&, UniValue&) rpc/server.cpp:444:13 #18 0x558f756b8017 in CRPCTable::execute(JSONRPCRequest const&) const rpc/server.cpp:464:13 #19 0x558f7552457a in (anonymous namespace)::RPCFuzzTestingSetup::CallRPC(std::__cxx11::basic_string, std::allocator > const&, std::vector, std::allocator >, std::allocator, std::allocator > > > const&) test/fuzz/rpc.cpp:50:25 #20 0x558f7552457a in rpc_fuzz_target(Span) test/fuzz/rpc.cpp:354:28 #21 0x558f7544cf0f in std::_Function_handler), void (*)(Span)>::_M_invoke(std::_Any_data const&, Span&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:300:2 #22 0x558f75c05197 in std::function)>::operator()(Span) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14 #23 0x558f75c05197 in LLVMFuzzerTestOneInput test/fuzz/fuzz.cpp:74:5 #24 0x558f753d8073 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o #25 0x558f753c1f72 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) fuzzer.o #26 0x558f753c7d6a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o #27 0x558f753f3a92 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x5a8a92) #28 0x7f4a3c7e00b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2) #29 0x558f7539cc9d in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x551c9d) ACKs for top commit: practicalswift: cr ACK fa2e614d16af84327adf1c02746d0f73e0f48111 Tree-SHA512: cfa120265261f0ad019b46c426b915c1c007806b37aecb27016ce780a0ddea5e6fc9b09065fd40684b11183dcd3bf543558d7a655e604695021653540266baf7 --- src/rpc/misc.cpp | 2 +- src/scheduler.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index 993368f7a9..6bec4dfec8 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -1097,7 +1097,7 @@ static UniValue mockscheduler(const JSONRPCRequest& request) // check params are valid values RPCTypeCheck(request.params, {UniValue::VNUM}); int64_t delta_seconds = request.params[0].get_int64(); - if ((delta_seconds <= 0) || (delta_seconds > 3600)) { + if (delta_seconds <= 0 || delta_seconds > 3600) { throw std::runtime_error("delta_time must be between 1 and 3600 seconds (1 hr)"); } diff --git a/src/scheduler.cpp b/src/scheduler.cpp index 0b08ff2d8b..a3a37e7c15 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -79,7 +80,7 @@ void CScheduler::schedule(CScheduler::Function f, std::chrono::system_clock::tim void CScheduler::MockForward(std::chrono::seconds delta_seconds) { - assert(delta_seconds.count() > 0 && delta_seconds < std::chrono::hours{1}); + assert(delta_seconds > 0s && delta_seconds <= 1h); { LOCK(newTaskMutex); From 21a6b171dcc6161d2b3973f287a9f1f83a04a9f0 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 20 Nov 2020 06:05:42 +0100 Subject: [PATCH 06/10] Merge #20056: net: Use Span in ReceiveMsgBytes fa5ed3b4ca609426b2622cad235e107d33db7b30 net: Use Span in ReceiveMsgBytes (MarcoFalke) Pull request description: Pass a data pointer and a size as span in `ReceiveMsgBytes` to get the benefits of a span ACKs for top commit: jonatack: ACK fa5ed3b4ca609426b2622cad235e107d33db7b30 code review, rebased to current master 12a1c3ad1a43634, debug build, unit tests, ran bitcoind/-netinfo/getpeerinfo theStack: ACK fa5ed3b4ca609426b2622cad235e107d33db7b30 Tree-SHA512: 89bf111323148d6e6e50185ad20ab39f73ab3a58a27e46319e3a08bcf5dcf9d6aa84faff0fd6afb90cb892ac2f557a237c144560986063bc736a69ace353ab9d --- src/net.cpp | 37 +++++++------------- src/net.h | 30 +++++++++++----- src/test/fuzz/net.cpp | 2 +- src/test/fuzz/p2p_transport_deserializer.cpp | 9 ++--- src/test/util/net.cpp | 8 ++--- src/test/util/net.h | 2 +- 6 files changed, 43 insertions(+), 45 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 73bb9ad628..3f63316140 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -676,34 +676,21 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) } #undef X -/** - * Receive bytes from the buffer and deserialize them into messages. - * - * @param[in] pch A pointer to the raw data - * @param[in] nBytes Size of the data - * @param[out] complete Set True if at least one message has been - * deserialized and is ready to be processed - * @return True if the peer should stay connected, - * False if the peer should be disconnected from. - */ -bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete) +bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) { complete = false; int64_t nTimeMicros = GetTimeMicros(); LOCK(cs_vRecv); nLastRecv = nTimeMicros / 1000000; - nRecvBytes += nBytes; - while (nBytes > 0) { + nRecvBytes += msg_bytes.size(); + while (msg_bytes.size() > 0) { // absorb network data - int handled = m_deserializer->Read(pch, nBytes); + int handled = m_deserializer->Read(msg_bytes); if (handled < 0) { // Serious header problem, disconnect from the peer. return false; } - pch += handled; - nBytes -= handled; - if (m_deserializer->Complete()) { // decompose a transport agnostic CNetMessage from the deserializer uint32_t out_err_raw_size{0}; @@ -760,13 +747,13 @@ int CNode::GetSendVersion() const return nSendVersion; } -int V1TransportDeserializer::readHeader(const char *pch, unsigned int nBytes) +int V1TransportDeserializer::readHeader(Span msg_bytes) { // copy data to temporary parsing buffer unsigned int nRemaining = 24 - nHdrPos; - unsigned int nCopy = std::min(nRemaining, nBytes); + unsigned int nCopy = std::min(nRemaining, msg_bytes.size()); - memcpy(&hdrbuf[nHdrPos], pch, nCopy); + memcpy(&hdrbuf[nHdrPos], msg_bytes.data(), nCopy); nHdrPos += nCopy; // if header incomplete, exit @@ -800,18 +787,18 @@ int V1TransportDeserializer::readHeader(const char *pch, unsigned int nBytes) return nCopy; } -int V1TransportDeserializer::readData(const char *pch, unsigned int nBytes) +int V1TransportDeserializer::readData(Span msg_bytes) { unsigned int nRemaining = hdr.nMessageSize - nDataPos; - unsigned int nCopy = std::min(nRemaining, nBytes); + unsigned int nCopy = std::min(nRemaining, msg_bytes.size()); if (vRecv.size() < nDataPos + nCopy) { // Allocate up to 256 KiB ahead, but never more than the total message size. vRecv.resize(std::min(hdr.nMessageSize, nDataPos + nCopy + 256 * 1024)); } - hasher.Write({(const unsigned char*)pch, nCopy}); - memcpy(&vRecv[nDataPos], pch, nCopy); + hasher.Write(MakeUCharSpan(msg_bytes.first(nCopy))); + memcpy(&vRecv[nDataPos], msg_bytes.data(), nCopy); nDataPos += nCopy; return nCopy; @@ -1939,7 +1926,7 @@ size_t CConnman::SocketRecvData(CNode *pnode) if (nBytes > 0) { bool notify = false; - if (!pnode->ReceiveMsgBytes(pchBuf, nBytes, notify)) { + if (!pnode->ReceiveMsgBytes(Span(pchBuf, nBytes), notify)) { LOCK(cs_vNodes); pnode->CloseSocketDisconnect(this); } diff --git a/src/net.h b/src/net.h index 6ca18e4ee0..138a228621 100644 --- a/src/net.h +++ b/src/net.h @@ -933,8 +933,8 @@ public: virtual bool Complete() const = 0; // set the serialization context version virtual void SetVersion(int version) = 0; - // read and deserialize data - virtual int Read(const char *data, unsigned int bytes) = 0; + /** read and deserialize data, advances msg_bytes data pointer */ + virtual int Read(Span& msg_bytes) = 0; // decomposes a message from the context virtual std::optional GetMessage(int64_t time, uint32_t& out_err) = 0; virtual ~TransportDeserializer() {} @@ -955,8 +955,8 @@ private: unsigned int nDataPos; const uint256& GetMessageHash() const; - int readHeader(const char *pch, unsigned int nBytes); - int readData(const char *pch, unsigned int nBytes); + int readHeader(Span msg_bytes); + int readData(Span msg_bytes); void Reset() { vRecv.clear(); @@ -990,9 +990,14 @@ public: hdrbuf.SetVersion(nVersionIn); vRecv.SetVersion(nVersionIn); } - int Read(const char *pch, unsigned int nBytes) override { - int ret = in_data ? readData(pch, nBytes) : readHeader(pch, nBytes); - if (ret < 0) Reset(); + int Read(Span& msg_bytes) override + { + int ret = in_data ? readData(msg_bytes) : readHeader(msg_bytes); + if (ret < 0) { + Reset(); + } else { + msg_bytes = msg_bytes.subspan(ret); + } return ret; } std::optional GetMessage(int64_t time, uint32_t& out_err_raw_size) override; @@ -1271,7 +1276,16 @@ public: return nRefCount; } - bool ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete); + /** + * Receive bytes from the buffer and deserialize them into messages. + * + * @param[in] msg_bytes The raw data + * @param[out] complete Set True if at least one message has been + * deserialized and is ready to be processed + * @return True if the peer should stay connected, + * False if the peer should be disconnected from. + */ + bool ReceiveMsgBytes(Span msg_bytes, bool& complete); void SetRecvVersion(int nVersionIn) { diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 3bd2369f2c..6df0b19cd7 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -129,7 +129,7 @@ FUZZ_TARGET_INIT(net, initialize_net) [&] { const std::vector b = ConsumeRandomLengthByteVector(fuzzed_data_provider); bool complete; - node.ReceiveMsgBytes((const char*)b.data(), b.size(), complete); + node.ReceiveMsgBytes({(const char*)b.data(), b.size()}, complete); }); } diff --git a/src/test/fuzz/p2p_transport_deserializer.cpp b/src/test/fuzz/p2p_transport_deserializer.cpp index 0786fdb73c..ad96d896e3 100644 --- a/src/test/fuzz/p2p_transport_deserializer.cpp +++ b/src/test/fuzz/p2p_transport_deserializer.cpp @@ -22,15 +22,12 @@ FUZZ_TARGET_INIT(p2p_transport_deserializer, initialize_p2p_transport_deserializ { // Construct deserializer, with a dummy NodeId V1TransportDeserializer deserializer{Params(), (NodeId)0, SER_NETWORK, INIT_PROTO_VERSION}; - const char* pch = (const char*)buffer.data(); - size_t n_bytes = buffer.size(); - while (n_bytes > 0) { - const int handled = deserializer.Read(pch, n_bytes); + Span msg_bytes{(const char*)buffer.data(), buffer.size()}; + while (msg_bytes.size() > 0) { + const int handled = deserializer.Read(msg_bytes); if (handled < 0) { break; } - pch += handled; - n_bytes -= handled; if (deserializer.Complete()) { const int64_t m_time = std::numeric_limits::max(); uint32_t out_err_raw_size{0}; diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 09f2f1807f..3b31ec4031 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -7,9 +7,9 @@ #include #include -void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, const char* pch, unsigned int nBytes, bool& complete) const +void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const { - assert(node.ReceiveMsgBytes(pch, nBytes, complete)); + assert(node.ReceiveMsgBytes(msg_bytes, complete)); if (complete) { size_t nSizeAdded = 0; auto it(node.vRecvMsg.begin()); @@ -33,7 +33,7 @@ bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) con node.m_serializer->prepareForTransport(ser_msg, ser_msg_header); bool complete; - NodeReceiveMsgBytes(node, (const char*)ser_msg_header.data(), ser_msg_header.size(), complete); - NodeReceiveMsgBytes(node, (const char*)ser_msg.data.data(), ser_msg.data.size(), complete); + NodeReceiveMsgBytes(node, {(const char*)ser_msg_header.data(), ser_msg_header.size()}, complete); + NodeReceiveMsgBytes(node, {(const char*)ser_msg.data.data(), ser_msg.data.size()}, complete); return complete; } diff --git a/src/test/util/net.h b/src/test/util/net.h index 8403ea9608..607a754f60 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -31,7 +31,7 @@ struct ConnmanTestMsg : public CConnman { void ProcessMessagesOnce(CNode& node) { m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); } - void NodeReceiveMsgBytes(CNode& node, const char* pch, unsigned int nBytes, bool& complete) const; + void NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const; bool ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const; }; From c1b5ec6360a932413aaba73bbbbad4c4605d59fe Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 23 Nov 2020 10:25:56 +0100 Subject: [PATCH 07/10] Merge #20432: net: Treat raw message bytes as uint8_t fabecce71909c984504c21fa05f91d5f1b471e8c net: Treat raw message bytes as uint8_t (MarcoFalke) Pull request description: Using `uint8_t` from the beginning when messages are `recv`ed has two style benefits: * The signedness is clear from reading the code, as it does not depend on the architecture * When passing the bytes on, the need for static signedness casts is dropped, making the code a bit less verbose and more coherent ACKs for top commit: laanwj: Code review ACK fabecce71909c984504c21fa05f91d5f1b471e8c theStack: Code Review ACK fabecce71909c984504c21fa05f91d5f1b471e8c jonatack: Tested ACK fabecce71909c984504c21fa05f91d5f1b471e8c Tree-SHA512: e6d9803c78633fde3304faf592afa961ff9462a7912d1da97a24720265274aa10ab4168d71b6ec2756b7448dd42585321afee0e5c889e705be778ce9a330d145 --- src/net.cpp | 14 +++++++------- src/net.h | 10 +++++----- src/test/fuzz/net.cpp | 2 +- src/test/fuzz/p2p_transport_deserializer.cpp | 2 +- src/test/util/net.cpp | 8 ++++---- src/test/util/net.h | 2 +- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 3f63316140..71843e8ded 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -676,7 +676,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) } #undef X -bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) +bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) { complete = false; int64_t nTimeMicros = GetTimeMicros(); @@ -747,7 +747,7 @@ int CNode::GetSendVersion() const return nSendVersion; } -int V1TransportDeserializer::readHeader(Span msg_bytes) +int V1TransportDeserializer::readHeader(Span msg_bytes) { // copy data to temporary parsing buffer unsigned int nRemaining = 24 - nHdrPos; @@ -787,7 +787,7 @@ int V1TransportDeserializer::readHeader(Span msg_bytes) return nCopy; } -int V1TransportDeserializer::readData(Span msg_bytes) +int V1TransportDeserializer::readData(Span msg_bytes) { unsigned int nRemaining = hdr.nMessageSize - nDataPos; unsigned int nCopy = std::min(nRemaining, msg_bytes.size()); @@ -797,7 +797,7 @@ int V1TransportDeserializer::readData(Span msg_bytes) vRecv.resize(std::min(hdr.nMessageSize, nDataPos + nCopy + 256 * 1024)); } - hasher.Write(MakeUCharSpan(msg_bytes.first(nCopy))); + hasher.Write(msg_bytes.first(nCopy)); memcpy(&vRecv[nDataPos], msg_bytes.data(), nCopy); nDataPos += nCopy; @@ -1912,13 +1912,13 @@ void CConnman::SocketHandler() size_t CConnman::SocketRecvData(CNode *pnode) { // typical socket buffer is 8K-64K - char pchBuf[0x10000]; + uint8_t pchBuf[0x10000]; int nBytes = 0; { LOCK(pnode->cs_hSocket); if (pnode->hSocket == INVALID_SOCKET) return 0; - nBytes = recv(pnode->hSocket, pchBuf, sizeof(pchBuf), MSG_DONTWAIT); + nBytes = recv(pnode->hSocket, (char*)pchBuf, sizeof(pchBuf), MSG_DONTWAIT); if (nBytes < (int)sizeof(pchBuf)) { pnode->fHasRecvData = false; } @@ -1926,7 +1926,7 @@ size_t CConnman::SocketRecvData(CNode *pnode) if (nBytes > 0) { bool notify = false; - if (!pnode->ReceiveMsgBytes(Span(pchBuf, nBytes), notify)) { + if (!pnode->ReceiveMsgBytes(Span(pchBuf, nBytes), notify)) { LOCK(cs_vNodes); pnode->CloseSocketDisconnect(this); } diff --git a/src/net.h b/src/net.h index 138a228621..359c00b795 100644 --- a/src/net.h +++ b/src/net.h @@ -934,7 +934,7 @@ public: // set the serialization context version virtual void SetVersion(int version) = 0; /** read and deserialize data, advances msg_bytes data pointer */ - virtual int Read(Span& msg_bytes) = 0; + virtual int Read(Span& msg_bytes) = 0; // decomposes a message from the context virtual std::optional GetMessage(int64_t time, uint32_t& out_err) = 0; virtual ~TransportDeserializer() {} @@ -955,8 +955,8 @@ private: unsigned int nDataPos; const uint256& GetMessageHash() const; - int readHeader(Span msg_bytes); - int readData(Span msg_bytes); + int readHeader(Span msg_bytes); + int readData(Span msg_bytes); void Reset() { vRecv.clear(); @@ -990,7 +990,7 @@ public: hdrbuf.SetVersion(nVersionIn); vRecv.SetVersion(nVersionIn); } - int Read(Span& msg_bytes) override + int Read(Span& msg_bytes) override { int ret = in_data ? readData(msg_bytes) : readHeader(msg_bytes); if (ret < 0) { @@ -1285,7 +1285,7 @@ public: * @return True if the peer should stay connected, * False if the peer should be disconnected from. */ - bool ReceiveMsgBytes(Span msg_bytes, bool& complete); + bool ReceiveMsgBytes(Span msg_bytes, bool& complete); void SetRecvVersion(int nVersionIn) { diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 6df0b19cd7..56ecab8d51 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -129,7 +129,7 @@ FUZZ_TARGET_INIT(net, initialize_net) [&] { const std::vector b = ConsumeRandomLengthByteVector(fuzzed_data_provider); bool complete; - node.ReceiveMsgBytes({(const char*)b.data(), b.size()}, complete); + node.ReceiveMsgBytes(b, complete); }); } diff --git a/src/test/fuzz/p2p_transport_deserializer.cpp b/src/test/fuzz/p2p_transport_deserializer.cpp index ad96d896e3..599dc0d044 100644 --- a/src/test/fuzz/p2p_transport_deserializer.cpp +++ b/src/test/fuzz/p2p_transport_deserializer.cpp @@ -22,7 +22,7 @@ FUZZ_TARGET_INIT(p2p_transport_deserializer, initialize_p2p_transport_deserializ { // Construct deserializer, with a dummy NodeId V1TransportDeserializer deserializer{Params(), (NodeId)0, SER_NETWORK, INIT_PROTO_VERSION}; - Span msg_bytes{(const char*)buffer.data(), buffer.size()}; + Span msg_bytes{buffer}; while (msg_bytes.size() > 0) { const int handled = deserializer.Read(msg_bytes); if (handled < 0) { diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 3b31ec4031..847a490e03 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -7,7 +7,7 @@ #include #include -void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const +void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const { assert(node.ReceiveMsgBytes(msg_bytes, complete)); if (complete) { @@ -29,11 +29,11 @@ void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_bytes bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const { - std::vector ser_msg_header; + std::vector ser_msg_header; node.m_serializer->prepareForTransport(ser_msg, ser_msg_header); bool complete; - NodeReceiveMsgBytes(node, {(const char*)ser_msg_header.data(), ser_msg_header.size()}, complete); - NodeReceiveMsgBytes(node, {(const char*)ser_msg.data.data(), ser_msg.data.size()}, complete); + NodeReceiveMsgBytes(node, ser_msg_header, complete); + NodeReceiveMsgBytes(node, ser_msg.data, complete); return complete; } diff --git a/src/test/util/net.h b/src/test/util/net.h index 607a754f60..0cdebe4678 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -31,7 +31,7 @@ struct ConnmanTestMsg : public CConnman { void ProcessMessagesOnce(CNode& node) { m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); } - void NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const; + void NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const; bool ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const; }; From 8fee13ef70fe7f7d1757ee5474767cc80f6b9820 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 25 Nov 2020 17:01:57 +0100 Subject: [PATCH 08/10] Merge #19337: sync: detect double lock from the same thread 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6 sync: detect double lock from the same thread (Vasil Dimov) 4df6567e4cbb4677e8048de2f8008612e1b860b9 sync: make EnterCritical() & push_lock() type safe (Vasil Dimov) Pull request description: Double lock of the same (non-recursive) mutex from the same thread would produce an undefined behavior. Detect this from `DEBUG_LOCKORDER` and react similarly to the deadlock detection. This came up during discussion in another, related PR: https://github.com/bitcoin/bitcoin/pull/19238#discussion_r442394521. ACKs for top commit: laanwj: code review ACK 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6 hebasto: re-ACK 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6 Tree-SHA512: 375c62db7819e348bfaecc3bd82a7907fcd8f5af24f7d637ac82f3f16789da9fc127dbd0e37158a08e0dcbba01a55c6635caf1d8e9e827cf5a3747f7690a498e --- src/sync.cpp | 53 +++++++++++++++++++++++++++++++++++---- src/sync.h | 14 ++++++----- src/test/sync_tests.cpp | 55 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 11 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index b9bc05c8ea..e641716eb8 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -13,11 +13,14 @@ #include #include +#include #include +#include #include #include #include +#include #include #include #include @@ -139,16 +142,50 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac throw std::logic_error(strprintf("potential deadlock detected: %s -> %s -> %s", mutex_b, mutex_a, mutex_b)); } -static void push_lock(void* c, const CLockLocation& locklocation) +static void double_lock_detected(const void* mutex, LockStack& lock_stack) { + LogPrintf("DOUBLE LOCK DETECTED\n"); + LogPrintf("Lock order:\n"); + for (const LockStackItem& i : lock_stack) { + if (i.first == mutex) { + LogPrintf(" (*)"); /* Continued */ + } + LogPrintf(" %s\n", i.second.ToString()); + } + if (g_debug_lockorder_abort) { + tfm::format(std::cerr, "Assertion failed: detected double lock at %s:%i, details in debug log.\n", __FILE__, __LINE__); + abort(); + } + throw std::logic_error("double lock detected"); +} + +template +static void push_lock(MutexType* c, const CLockLocation& locklocation) +{ + constexpr bool is_recursive_mutex = + std::is_base_of::value || + std::is_base_of::value; + LockData& lockdata = GetLockData(); std::lock_guard lock(lockdata.dd_mutex); LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; lock_stack.emplace_back(c, locklocation); - for (const LockStackItem& i : lock_stack) { - if (i.first == c) - break; + for (size_t j = 0; j < lock_stack.size() - 1; ++j) { + const LockStackItem& i = lock_stack[j]; + if (i.first == c) { + if (is_recursive_mutex) { + break; + } + // It is not a recursive mutex and it appears in the stack two times: + // at position `j` and at the end (which we added just before this loop). + // Can't allow locking the same (non-recursive) mutex two times from the + // same thread as that results in an undefined behavior. + auto lock_stack_copy = lock_stack; + lock_stack.pop_back(); + double_lock_detected(c, lock_stack_copy); + // double_lock_detected() does not return. + } const LockPair p1 = std::make_pair(i.first, c); if (lockdata.lockorders.count(p1)) @@ -179,10 +216,16 @@ static void pop_lock() } } -void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) +template +void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry) { push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry, util::ThreadGetInternalName())); } +template void EnterCritical(const char*, const char*, int, Mutex*, bool); +template void EnterCritical(const char*, const char*, int, RecursiveMutex*, bool); +template void EnterCritical(const char*, const char*, int, std::mutex*, bool); +template void EnterCritical(const char*, const char*, int, std::recursive_mutex*, bool); +template void EnterCritical(const char*, const char*, int, boost::mutex*, bool); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) { diff --git a/src/sync.h b/src/sync.h index a5a583810d..aed8c697b5 100644 --- a/src/sync.h +++ b/src/sync.h @@ -48,7 +48,8 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII /////////////////////////////// #ifdef DEBUG_LOCKORDER -void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false); +template +void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false); void LeaveCritical(); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line); std::string LocksHeld(); @@ -65,7 +66,8 @@ bool LockStackEmpty(); */ extern bool g_debug_lockorder_abort; #else -inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {} +template +inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false) {} inline void LeaveCritical() {} inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {} template @@ -133,7 +135,7 @@ class SCOPED_LOCKABLE UniqueLock : public Base private: void Enter(const char* pszName, const char* pszFile, int nLine) { - EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex())); + EnterCritical(pszName, pszFile, nLine, Base::mutex()); #ifdef DEBUG_LOCKCONTENTION if (!Base::try_lock()) { PrintLockContention(pszName, pszFile, nLine); @@ -146,7 +148,7 @@ private: bool TryEnter(const char* pszName, const char* pszFile, int nLine) { - EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()), true); + EnterCritical(pszName, pszFile, nLine, Base::mutex(), true); Base::try_lock(); if (!Base::owns_lock()) LeaveCritical(); @@ -203,7 +205,7 @@ public: ~reverse_lock() { templock.swap(lock); - EnterCritical(lockname.c_str(), file.c_str(), line, (void*)lock.mutex()); + EnterCritical(lockname.c_str(), file.c_str(), line, lock.mutex()); lock.lock(); } @@ -234,7 +236,7 @@ using DebugLock = UniqueLock #include +#include + +#include namespace { template @@ -42,6 +45,38 @@ void TestInconsistentLockOrderDetected(MutexType& mutex1, MutexType& mutex2) NO_ LEAVE_CRITICAL_SECTION(mutex1); BOOST_CHECK(LockStackEmpty()); } + +#ifdef DEBUG_LOCKORDER +template +void TestDoubleLock2(MutexType& m) +{ + ENTER_CRITICAL_SECTION(m); + LEAVE_CRITICAL_SECTION(m); +} + +template +void TestDoubleLock(bool should_throw) +{ + const bool prev = g_debug_lockorder_abort; + g_debug_lockorder_abort = false; + + MutexType m; + ENTER_CRITICAL_SECTION(m); + if (should_throw) { + BOOST_CHECK_EXCEPTION( + TestDoubleLock2(m), std::logic_error, [](const std::logic_error& e) { + return strcmp(e.what(), "double lock detected") == 0; + }); + } else { + BOOST_CHECK_NO_THROW(TestDoubleLock2(m)); + } + LEAVE_CRITICAL_SECTION(m); + + BOOST_CHECK(LockStackEmpty()); + + g_debug_lockorder_abort = prev; +} +#endif /* DEBUG_LOCKORDER */ } // namespace BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup) @@ -92,4 +127,24 @@ BOOST_AUTO_TEST_CASE(inconsistent_lock_order_detected) #endif // DEBUG_LOCKORDER } +/* Double lock would produce an undefined behavior. Thus, we only do that if + * DEBUG_LOCKORDER is activated to detect it. We don't want non-DEBUG_LOCKORDER + * build to produce tests that exhibit known undefined behavior. */ +#ifdef DEBUG_LOCKORDER +BOOST_AUTO_TEST_CASE(double_lock_mutex) +{ + TestDoubleLock(true /* should throw */); +} + +BOOST_AUTO_TEST_CASE(double_lock_boost_mutex) +{ + TestDoubleLock(true /* should throw */); +} + +BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex) +{ + TestDoubleLock(false /* should not throw */); +} +#endif /* DEBUG_LOCKORDER */ + BOOST_AUTO_TEST_SUITE_END() From 8b4982bc0bb2733eedcb7060bd76a7188b47ef33 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 28 Nov 2020 10:14:43 +0100 Subject: [PATCH 09/10] Merge #20448: RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC endpoint wallet 89bdad5b25ae4ac03a486f729a5b58ae6f21946d RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC endpoint (Luke Dashjr) Pull request description: Allow specifying the `wallet_name` param to `unloadwallet` on RPC wallet endpoints, so long as it matches the endpoint wallet. ACKs for top commit: jonatack: ACK 89bdad5b25ae4ac03a486f729a5b58ae6f21946d MarcoFalke: review ACK 89bdad5b25ae4ac03a486f729a5b58ae6f21946d Tree-SHA512: efb399c33f7b5596870a26a8680f453ca47aa7a6db4e550f9435d13044f1c4bad0ae11e8f0205213409d08b75c4188c3be782e54aafab1f65b97eb8cf5c252a9 --- src/wallet/rpcwallet.cpp | 6 +++--- test/functional/wallet_multiwallet.py | 9 +++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index ce09ecb75c..a19490c298 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2961,7 +2961,7 @@ static UniValue unloadwallet(const JSONRPCRequest& request) "Unloads the wallet referenced by the request endpoint otherwise unloads the wallet specified in the argument.\n" "Specifying the wallet name on a wallet endpoint is invalid.", { - {"wallet_name", RPCArg::Type::STR, /* default */ "the wallet name from the RPC endpoint", "The name of the wallet to unload. Must be provided in the RPC endpoint or this parameter (but not both)."}, + {"wallet_name", RPCArg::Type::STR, /* default */ "the wallet name from the RPC endpoint", "The name of the wallet to unload. If provided both here and in the RPC endpoint, the two must be identical."}, {"load_on_startup", RPCArg::Type::BOOL, /* default */ "null", "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."}, }, RPCResult{RPCResult::Type::OBJ, "", "", { @@ -2975,8 +2975,8 @@ static UniValue unloadwallet(const JSONRPCRequest& request) std::string wallet_name; if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) { - if (!request.params[0].isNull()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Both the RPC endpoint wallet and wallet_name parameter were provided (only one allowed)"); + if (!(request.params[0].isNull() || request.params[0].get_str() == wallet_name)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "RPC endpoint wallet and wallet_name parameter specify different wallets"); } } else { wallet_name = request.params[0].get_str(); diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 3752ddf520..f9c98caba5 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -339,13 +339,18 @@ class MultiWalletTest(BitcoinTestFramework): assert_raises_rpc_error(-1, "JSON value is not a string as expected", self.nodes[0].unloadwallet) assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", self.nodes[0].unloadwallet, "dummy") assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", node.get_wallet_rpc("dummy").unloadwallet) - assert_raises_rpc_error(-8, "Both the RPC endpoint wallet and wallet_name parameter were provided (only one allowed)", w1.unloadwallet, "w2"), - assert_raises_rpc_error(-8, "Both the RPC endpoint wallet and wallet_name parameter were provided (only one allowed)", w1.unloadwallet, "w1"), + assert_raises_rpc_error(-8, "RPC endpoint wallet and wallet_name parameter specify different wallets", w1.unloadwallet, "w2"), # Successfully unload the specified wallet name self.nodes[0].unloadwallet("w1") assert 'w1' not in self.nodes[0].listwallets() + # Unload w1 again, this time providing the wallet name twice + self.nodes[0].loadwallet("w1") + assert 'w1' in self.nodes[0].listwallets() + w1.unloadwallet("w1") + assert 'w1' not in self.nodes[0].listwallets() + # Successfully unload the wallet referenced by the request endpoint # Also ensure unload works during walletpassphrase timeout w2.encryptwallet('test') From 5d917340652e878f247cfcbdcbb39ca05a97c9ba Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 26 Jan 2021 09:10:15 +0100 Subject: [PATCH 10/10] Merge #21010: refactor: remove straggling boost::mutex usage f827e151a2ce96e14aadb9e7d25045fe0a8afbd2 refactor: remove straggling boost::mutex usage (fanquake) Pull request description: After the merge of #18710, the linter is warning: ```bash A new Boost dependency in the form of "boost/thread/mutex.hpp" appears to have been introduced: src/sync.cpp:#include src/test/sync_tests.cpp:#include ^---- failure generated from test/lint/lint-includes.sh ``` #18710 removed `boost/thread/mutex.hpp` from lint-includes, however in the interim #19337 was merged, which introduced more `boost::mutex` usage. Given we no longer use `boost::mutex`, just remove the double lock test and remaining includes. ACKs for top commit: laanwj: Code review ACK f827e151a2ce96e14aadb9e7d25045fe0a8afbd2 hebasto: ACK f827e151a2ce96e14aadb9e7d25045fe0a8afbd2 Tree-SHA512: f738b12189fe5b39db3e8f8231e9002714413a962eaf98adc84a6614fa474df5616358cfb1c89b92a2b0564efa9b704a774c49d4a25dca18a0ccc3cd9eabfc0a --- src/sync.cpp | 3 --- src/test/sync_tests.cpp | 6 ------ 2 files changed, 9 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index e641716eb8..d57bd79e7e 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -13,8 +13,6 @@ #include #include -#include - #include #include #include @@ -225,7 +223,6 @@ template void EnterCritical(const char*, const char*, int, Mutex*, bool); template void EnterCritical(const char*, const char*, int, RecursiveMutex*, bool); template void EnterCritical(const char*, const char*, int, std::mutex*, bool); template void EnterCritical(const char*, const char*, int, std::recursive_mutex*, bool); -template void EnterCritical(const char*, const char*, int, boost::mutex*, bool); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) { diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp index bc14213ae8..25fca556a7 100644 --- a/src/test/sync_tests.cpp +++ b/src/test/sync_tests.cpp @@ -6,7 +6,6 @@ #include #include -#include #include @@ -136,11 +135,6 @@ BOOST_AUTO_TEST_CASE(double_lock_mutex) TestDoubleLock(true /* should throw */); } -BOOST_AUTO_TEST_CASE(double_lock_boost_mutex) -{ - TestDoubleLock(true /* should throw */); -} - BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex) { TestDoubleLock(false /* should not throw */);