From 490a5d41ca2d6acdd6aa2fe643ef2773578e6910 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Thu, 7 Dec 2017 21:33:20 +0000 Subject: [PATCH 01/24] rpc: do not try to use an invalid txid in relay_tx --- src/rpc/core_rpc_server.cpp | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp index 803588cbd..a1d4d2d38 100644 --- a/src/rpc/core_rpc_server.cpp +++ b/src/rpc/core_rpc_server.cpp @@ -1436,19 +1436,25 @@ namespace cryptonote { failed = true; } - crypto::hash txid = *reinterpret_cast(txid_data.data()); - txids.push_back(txid); + else + { + crypto::hash txid = *reinterpret_cast(txid_data.data()); + txids.push_back(txid); + } } } if (!m_core.get_blockchain_storage().flush_txes_from_pool(txids)) { - res.status = "Failed to remove one more tx"; + res.status = "Failed to remove one or more tx(es)"; return false; } if (failed) { - res.status = "Failed to parse txid"; + if (txids.empty()) + res.status = "Failed to parse txid"; + else + res.status = "Failed to parse some of the txids"; return false; } @@ -1705,13 +1711,16 @@ namespace cryptonote PERF_TIMER(on_relay_tx); bool failed = false; + res.status = ""; for (const auto &str: req.txids) { cryptonote::blobdata txid_data; if(!epee::string_tools::parse_hexstr_to_binbuff(str, txid_data)) { - res.status = std::string("Invalid transaction id: ") + str; + if (!res.status.empty()) res.status += ", "; + res.status += std::string("invalid transaction id: ") + str; failed = true; + continue; } crypto::hash txid = *reinterpret_cast(txid_data.data()); @@ -1727,8 +1736,10 @@ namespace cryptonote } else { - res.status = std::string("Transaction not found in pool: ") + str; + if (!res.status.empty()) res.status += ", "; + res.status += std::string("transaction not found in pool: ") + str; failed = true; + continue; } } From 25584f86390513aa15c0b596699f2f3b20875b14 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Thu, 7 Dec 2017 21:39:48 +0000 Subject: [PATCH 02/24] cryptonote_protocol: print peer versions when unexpected also avoid integer underflow on zero height --- .../cryptonote_protocol_handler.inl | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index 389e8ba84..dbcc4eba1 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -266,13 +266,17 @@ namespace cryptonote return true; // from v6, if the peer advertises a top block version, reject if it's not what it should be (will only work if no voting) - const uint8_t version = m_core.get_ideal_hard_fork_version(hshd.current_height - 1); - if (version >= 6 && version != hshd.top_version) + if (hshd.current_height > 0) { - if (version < hshd.top_version) - MCLOG_RED(el::Level::Warning, "global", context << " peer claims higher version that we think - we may be forked from the network and a software upgrade may be needed"); - LOG_DEBUG_CC(context, "Ignoring due to wrong top version for block " << (hshd.current_height - 1) << ": " << (unsigned)hshd.top_version << ", expected " << (unsigned)version); - return false; + const uint8_t version = m_core.get_ideal_hard_fork_version(hshd.current_height - 1); + if (version >= 6 && version != hshd.top_version) + { + if (version < hshd.top_version) + MCLOG_RED(el::Level::Warning, "global", context << " peer claims higher version that we think (" << + (unsigned)hshd.top_version << " for " << (hshd.current_height - 1) << "instead of " << (unsigned)version << + ") - we may be forked from the network and a software upgrade may be needed"); + return false; + } } context.m_remote_blockchain_height = hshd.current_height; From 46d6fa35c9c3f5257a31bc4dae89a102a441831b Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Thu, 7 Dec 2017 22:22:14 +0000 Subject: [PATCH 03/24] cryptonote_protocol: sanity check chain hashes from peer --- src/cryptonote_protocol/cryptonote_protocol_handler.inl | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index dbcc4eba1..de30df5d7 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -1585,6 +1585,12 @@ skip: drop_connection(context, true, false); return 1; } + if (arg.total_height < arg.m_block_ids.size() || arg.start_height > arg.total_height - arg.m_block_ids.size()) + { + LOG_ERROR_CCONTEXT("sent invalid start/nblocks/height, dropping connection"); + drop_connection(context, true, false); + return 1; + } context.m_remote_blockchain_height = arg.total_height; context.m_last_response_height = arg.start_height + arg.m_block_ids.size()-1; From fe568db83d8e0196acb6dabf60fff39543783e12 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Thu, 7 Dec 2017 22:44:55 +0000 Subject: [PATCH 04/24] p2p: use size_t for arbitrary counters instead of uint8_t --- src/p2p/net_node.inl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl index 55be7c2cb..b6ef530d1 100644 --- a/src/p2p/net_node.inl +++ b/src/p2p/net_node.inl @@ -1856,8 +1856,8 @@ namespace nodetool template bool node_server::has_too_many_connections(const epee::net_utils::network_address &address) { - const uint8_t max_connections = 1; - uint8_t count = 0; + const size_t max_connections = 1; + size_t count = 0; m_net_server.get_config_object().foreach_connection([&](const p2p_connection_context& cntxt) { From d753d716a6288ea9ecb4c9262b63ad804fa0a6e0 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sat, 9 Dec 2017 12:56:27 +0000 Subject: [PATCH 05/24] fix a few leaks by throwing objects, not newed pointers to objects --- src/blockchain_db/blockchain_db.cpp | 10 +++++----- src/cryptonote_core/blockchain.cpp | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/blockchain_db/blockchain_db.cpp b/src/blockchain_db/blockchain_db.cpp index 07b2451b0..7e77953c8 100644 --- a/src/blockchain_db/blockchain_db.cpp +++ b/src/blockchain_db/blockchain_db.cpp @@ -197,7 +197,7 @@ uint64_t BlockchainDB::add_block( const block& blk { // sanity if (blk.tx_hashes.size() != txs.size()) - throw new std::runtime_error("Inconsistent tx/hashes sizes"); + throw std::runtime_error("Inconsistent tx/hashes sizes"); block_txn_start(false); @@ -283,7 +283,7 @@ block BlockchainDB::get_block_from_height(const uint64_t& height) const blobdata bd = get_block_blob_from_height(height); block b; if (!parse_and_validate_block_from_blob(bd, b)) - throw new DB_ERROR("Failed to parse block from blob retrieved from the db"); + throw DB_ERROR("Failed to parse block from blob retrieved from the db"); return b; } @@ -293,7 +293,7 @@ block BlockchainDB::get_block(const crypto::hash& h) const blobdata bd = get_block_blob(h); block b; if (!parse_and_validate_block_from_blob(bd, b)) - throw new DB_ERROR("Failed to parse block from blob retrieved from the db"); + throw DB_ERROR("Failed to parse block from blob retrieved from the db"); return b; } @@ -304,7 +304,7 @@ bool BlockchainDB::get_tx(const crypto::hash& h, cryptonote::transaction &tx) co if (!get_tx_blob(h, bd)) return false; if (!parse_and_validate_tx_from_blob(bd, tx)) - throw new DB_ERROR("Failed to parse transaction from blob retrieved from the db"); + throw DB_ERROR("Failed to parse transaction from blob retrieved from the db"); return true; } @@ -313,7 +313,7 @@ transaction BlockchainDB::get_tx(const crypto::hash& h) const { transaction tx; if (!get_tx(h, tx)) - throw new TX_DNE(std::string("tx with hash ").append(epee::string_tools::pod_to_hex(h)).append(" not found in db").c_str()); + throw TX_DNE(std::string("tx with hash ").append(epee::string_tools::pod_to_hex(h)).append(" not found in db").c_str()); return tx; } diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 123bd194b..2fd61e1d2 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -471,7 +471,7 @@ bool Blockchain::deinit() // memory operation), otherwise we may cause a loop. if (m_db == NULL) { - throw new DB_ERROR("The db pointer is null in Blockchain, the blockchain may be corrupt!"); + throw DB_ERROR("The db pointer is null in Blockchain, the blockchain may be corrupt!"); } try From 38c8f4e0a3d7331b7fab1bd9e167b230edacc4ef Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sat, 9 Dec 2017 19:33:26 +0000 Subject: [PATCH 06/24] mlog: terminate a string at last char, just in case --- contrib/epee/src/mlog.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/epee/src/mlog.cpp b/contrib/epee/src/mlog.cpp index a30efbc6a..5b9472006 100644 --- a/contrib/epee/src/mlog.cpp +++ b/contrib/epee/src/mlog.cpp @@ -59,6 +59,7 @@ static std::string generate_log_filename(const char *base) strcpy(tmp, "unknown"); else strftime(tmp, sizeof(tmp), "%Y-%m-%d-%H-%M-%S", &tm); + tmp[sizeof(tmp) - 1] = 0; filename += "-"; filename += tmp; return filename; From ba2fefb9a4280c26f3046da78aaee812228ee7a5 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sun, 10 Dec 2017 15:05:48 +0000 Subject: [PATCH 07/24] checkpoints: pass std::string by const ref, not const value --- src/checkpoints/checkpoints.cpp | 4 ++-- src/checkpoints/checkpoints.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/checkpoints/checkpoints.cpp b/src/checkpoints/checkpoints.cpp index c66c4f5d6..e6253c73d 100644 --- a/src/checkpoints/checkpoints.cpp +++ b/src/checkpoints/checkpoints.cpp @@ -204,7 +204,7 @@ namespace cryptonote return true; } - bool checkpoints::load_checkpoints_from_json(const std::string json_hashfile_fullpath) + bool checkpoints::load_checkpoints_from_json(const std::string &json_hashfile_fullpath) { boost::system::error_code errcode; if (! (boost::filesystem::exists(json_hashfile_fullpath, errcode))) @@ -286,7 +286,7 @@ namespace cryptonote return true; } - bool checkpoints::load_new_checkpoints(const std::string json_hashfile_fullpath, bool testnet, bool dns) + bool checkpoints::load_new_checkpoints(const std::string &json_hashfile_fullpath, bool testnet, bool dns) { bool result; diff --git a/src/checkpoints/checkpoints.h b/src/checkpoints/checkpoints.h index 3e034f6f0..83969f7b8 100644 --- a/src/checkpoints/checkpoints.h +++ b/src/checkpoints/checkpoints.h @@ -165,7 +165,7 @@ namespace cryptonote * * @return true if loading successful and no conflicts */ - bool load_new_checkpoints(const std::string json_hashfile_fullpath, bool testnet=false, bool dns=true); + bool load_new_checkpoints(const std::string &json_hashfile_fullpath, bool testnet=false, bool dns=true); /** * @brief load new checkpoints from json @@ -174,7 +174,7 @@ namespace cryptonote * * @return true if loading successful and no conflicts */ - bool load_checkpoints_from_json(const std::string json_hashfile_fullpath); + bool load_checkpoints_from_json(const std::string &json_hashfile_fullpath); /** * @brief load new checkpoints from DNS From 061789b5dd8a84e6914ed8536e8bd66c656a63e5 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sun, 10 Dec 2017 15:06:04 +0000 Subject: [PATCH 08/24] checkpoints: trap failure to load JSON checkpoints --- src/checkpoints/checkpoints.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/checkpoints/checkpoints.cpp b/src/checkpoints/checkpoints.cpp index e6253c73d..67a313bc2 100644 --- a/src/checkpoints/checkpoints.cpp +++ b/src/checkpoints/checkpoints.cpp @@ -218,7 +218,11 @@ namespace cryptonote uint64_t prev_max_height = get_max_height(); LOG_PRINT_L1("Hard-coded max checkpoint height is " << prev_max_height); t_hash_json hashes; - epee::serialization::load_t_from_json_file(hashes, json_hashfile_fullpath); + if (!epee::serialization::load_t_from_json_file(hashes, json_hashfile_fullpath)) + { + MERROR("Error loading checkpoints from " << json_hashfile_fullpath); + return false; + } for (std::vector::const_iterator it = hashes.hashlines.begin(); it != hashes.hashlines.end(); ) { uint64_t height; From 187a6ab2d24df7e85c85c19d83c4820878a713cc Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sun, 10 Dec 2017 15:06:23 +0000 Subject: [PATCH 09/24] epee: trap failure to parse URI from request --- contrib/epee/include/net/http_protocol_handler.inl | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contrib/epee/include/net/http_protocol_handler.inl b/contrib/epee/include/net/http_protocol_handler.inl index c555707ac..c18f7f706 100644 --- a/contrib/epee/include/net/http_protocol_handler.inl +++ b/contrib/epee/include/net/http_protocol_handler.inl @@ -345,7 +345,12 @@ namespace net_utils { analize_http_method(result, m_query_info.m_http_method, m_query_info.m_http_ver_hi, m_query_info.m_http_ver_hi); m_query_info.m_URI = result[10]; - parse_uri(m_query_info.m_URI, m_query_info.m_uri_content); + if (!parse_uri(m_query_info.m_URI, m_query_info.m_uri_content)) + { + m_state = http_state_error; + MERROR("Failed to parse URI: m_query_info.m_URI"); + return false; + } m_query_info.m_http_method_str = result[2]; m_query_info.m_full_request_str = result[0]; From c2ed8618e489ee79ff9073e601f44c71d0ad987e Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sun, 10 Dec 2017 15:32:42 +0000 Subject: [PATCH 10/24] easylogging++: avoid buffer underflow --- external/easylogging++/easylogging++.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/external/easylogging++/easylogging++.cc b/external/easylogging++/easylogging++.cc index 31b201897..57742b2e5 100644 --- a/external/easylogging++/easylogging++.cc +++ b/external/easylogging++/easylogging++.cc @@ -1016,8 +1016,9 @@ const std::string OS::getBashOutput(const char* command) { char hBuff[4096]; if (fgets(hBuff, sizeof(hBuff), proc) != nullptr) { pclose(proc); - if (hBuff[strlen(hBuff) - 1] == '\n') { - hBuff[strlen(hBuff) - 1] = '\0'; + const size_t len = strlen(hBuff); + if (len > 0 && hBuff[len - 1] == '\n') { + hBuff[len- 1] = '\0'; } return std::string(hBuff); } From b4524892fbb79a03f9a000ee7407743b07343b33 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sun, 10 Dec 2017 15:36:15 +0000 Subject: [PATCH 11/24] rpc: guard against json parsing a non object --- src/rpc/message.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/message.cpp b/src/rpc/message.cpp index d6da124d1..98b40e667 100644 --- a/src/rpc/message.cpp +++ b/src/rpc/message.cpp @@ -111,7 +111,7 @@ FullMessage::FullMessage(Message* message) FullMessage::FullMessage(const std::string& json_string, bool request) { doc.Parse(json_string.c_str()); - if (doc.HasParseError()) + if (doc.HasParseError() || !doc.IsObject()) { throw cryptonote::json::PARSE_FAIL(); } From 56fa6ce15f39f565060ab2b006b770057b5da99e Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Mon, 11 Dec 2017 18:37:05 +0000 Subject: [PATCH 12/24] tests: fix a buffer overread in a unit test and remove a leftover debugging sanity check --- tests/unit_tests/slow_memmem.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit_tests/slow_memmem.cpp b/tests/unit_tests/slow_memmem.cpp index 0312019be..6e1dcb85f 100644 --- a/tests/unit_tests/slow_memmem.cpp +++ b/tests/unit_tests/slow_memmem.cpp @@ -81,7 +81,6 @@ static const struct { {1,"x",1,"x",0}, {2,"x",1,"",1}, {1,"x",1,"",0}, - {1,"x",2,"",0}, {1,"x",2,"x",0}, {2,"ax",2,"x",0}, {1,"xx",2,"xx",0}, @@ -103,7 +102,7 @@ static const struct { {8,"xxxxxxab",3,"xyz",0}, {8,"xxxxxxab",6,"abcdef",0}, {9,"\0xxxxxab",3,"ab",6}, - {4,"\0\0a",3,"\0a",1}, + {4,"\0\0a",3,"\0a",1}, // }; TEST(slowmem,Success) @@ -122,7 +121,6 @@ TEST(slowmem,Success) free(pat); free(buf); ASSERT_EQ(res,T[n].res); -ASSERT_EQ(1,1); #ifdef VERBOSE if (res!=T[n].res) printf("failed (got %zu, expected %zu)",res,T[n].res); else printf("ok"); printf("\n"); From 45a1c4c0885584554668c89bd6976474af01e2f5 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Mon, 11 Dec 2017 22:36:58 +0000 Subject: [PATCH 13/24] add empty container sanity checks when using front() and back() --- contrib/epee/include/net/abstract_tcp_server.h | 2 +- src/cryptonote_protocol/block_queue.cpp | 1 + .../cryptonote_protocol_handler.inl | 6 ++++++ src/mnemonics/electrum-words.cpp | 2 ++ src/rpc/core_rpc_server.cpp | 11 ++++++++--- src/rpc/daemon_handler.cpp | 4 ++++ src/simplewallet/simplewallet.cpp | 4 ++-- src/wallet/wallet2.cpp | 1 + src/wallet/wallet_rpc_server.cpp | 7 +++++++ 9 files changed, 32 insertions(+), 6 deletions(-) diff --git a/contrib/epee/include/net/abstract_tcp_server.h b/contrib/epee/include/net/abstract_tcp_server.h index 000305cfa..cbad1717c 100644 --- a/contrib/epee/include/net/abstract_tcp_server.h +++ b/contrib/epee/include/net/abstract_tcp_server.h @@ -305,7 +305,7 @@ namespace net_utils m_connections.back().powner = this; m_connections.back().m_self_it = --m_connections.end(); m_connections.back().m_context.m_remote_address = remote_address; - m_connections.back().m_htread = threads_helper::create_thread(ConnectionHandlerProc, &m_connections.back()); + m_connections.back().m_htread = threads_helper::create_thread(ConnectionHandlerProc, &m_connections.back()); // ugh, seems very risky return true; } diff --git a/src/cryptonote_protocol/block_queue.cpp b/src/cryptonote_protocol/block_queue.cpp index bfff35456..9e38ffbcb 100644 --- a/src/cryptonote_protocol/block_queue.cpp +++ b/src/cryptonote_protocol/block_queue.cpp @@ -62,6 +62,7 @@ void block_queue::add_blocks(uint64_t height, std::list 0, "Empty span"); boost::unique_lock lock(mutex); blocks.insert(span(height, nblocks, connection_id, time)); } diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index de30df5d7..8aef31a5a 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -1003,6 +1003,11 @@ skip: MDEBUG(context << " next span in the queue has blocks " << start_height << "-" << (start_height + blocks.size() - 1) << ", we need " << previous_height); + if (blocks.empty()) + { + MERROR("Next span has no blocks"); + break; + } block new_block; if (!parse_and_validate_block_from_blob(blocks.front().block, new_block)) @@ -1498,6 +1503,7 @@ skip: NOTIFY_REQUEST_CHAIN::request r = boost::value_initialized(); m_core.get_short_chain_history(r.block_ids); + CHECK_AND_ASSERT_MES(!r.block_ids.empty(), false, "Short chain history is empty"); if (!start_from_current_chain) { diff --git a/src/mnemonics/electrum-words.cpp b/src/mnemonics/electrum-words.cpp index 1b14905f6..ba67952aa 100644 --- a/src/mnemonics/electrum-words.cpp +++ b/src/mnemonics/electrum-words.cpp @@ -205,6 +205,8 @@ namespace */ bool checksum_test(std::vector seed, uint32_t unique_prefix_length) { + if (seed.empty()) + return false; // The last word is the checksum. std::string last_word = seed.back(); seed.pop_back(); diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp index a1d4d2d38..a6109cb89 100644 --- a/src/rpc/core_rpc_server.cpp +++ b/src/rpc/core_rpc_server.cpp @@ -492,6 +492,11 @@ namespace cryptonote { if (std::find(missed_txs.begin(), missed_txs.end(), h) == missed_txs.end()) { + if (txs.empty()) + { + res.status = "Failed: internal error - txs is empty"; + return true; + } // core returns the ones it finds in the right order if (get_transaction_hash(txs.front()) != h) { @@ -1150,7 +1155,7 @@ namespace cryptonote error_resp.message = "Internal error: can't get block by hash. Hash = " + req.hash + '.'; return false; } - if (blk.miner_tx.vin.front().type() != typeid(txin_gen)) + if (blk.miner_tx.vin.size() != 1 || blk.miner_tx.vin.front().type() != typeid(txin_gen)) { error_resp.code = CORE_RPC_ERROR_CODE_INTERNAL_ERROR; error_resp.message = "Internal error: coinbase transaction in the block has the wrong type"; @@ -1188,7 +1193,7 @@ namespace cryptonote error_resp.message = "Internal error: can't get block by height. Height = " + boost::lexical_cast(h) + ". Hash = " + epee::string_tools::pod_to_hex(block_hash) + '.'; return false; } - if (blk.miner_tx.vin.front().type() != typeid(txin_gen)) + if (blk.miner_tx.vin.size() != 1 || blk.miner_tx.vin.front().type() != typeid(txin_gen)) { error_resp.code = CORE_RPC_ERROR_CODE_INTERNAL_ERROR; error_resp.message = "Internal error: coinbase transaction in the block has the wrong type"; @@ -1274,7 +1279,7 @@ namespace cryptonote error_resp.message = "Internal error: can't get block by hash. Hash = " + req.hash + '.'; return false; } - if (blk.miner_tx.vin.front().type() != typeid(txin_gen)) + if (blk.miner_tx.vin.size() != 1 || blk.miner_tx.vin.front().type() != typeid(txin_gen)) { error_resp.code = CORE_RPC_ERROR_CODE_INTERNAL_ERROR; error_resp.message = "Internal error: coinbase transaction in the block has the wrong type"; diff --git a/src/rpc/daemon_handler.cpp b/src/rpc/daemon_handler.cpp index 6643ce4e4..908f9e187 100644 --- a/src/rpc/daemon_handler.cpp +++ b/src/rpc/daemon_handler.cpp @@ -799,6 +799,10 @@ namespace rpc } header.hash = hash_in; + if (b.miner_tx.vin.size() != 1 || b.miner_tx.vin.front().type() != typeid(txin_gen)) + { + return false; + } header.height = boost::get(b.miner_tx.vin.front()).height; header.major_version = b.major_version; diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp index 64e665fb3..e34e718d2 100644 --- a/src/simplewallet/simplewallet.cpp +++ b/src/simplewallet/simplewallet.cpp @@ -4321,9 +4321,9 @@ bool simple_wallet::sweep_single(const std::vector &args_) fail_msg_writer() << tr("Multiple transactions are created, which is not supposed to happen"); return true; } - if (ptx_vector[0].selected_transfers.size() > 1) + if (ptx_vector[0].selected_transfers.size() != 1) { - fail_msg_writer() << tr("The transaction uses multiple inputs, which is not supposed to happen"); + fail_msg_writer() << tr("The transaction uses multiple or no inputs, which is not supposed to happen"); return true; } diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 04c6ee236..88f9e0934 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -4950,6 +4950,7 @@ bool wallet2::tx_add_fake_output(std::vectoradjust_mixin(req.mixin); std::vector ptx_vector = m_wallet->create_transactions_2(dsts, mixin, req.unlock_time, req.priority, extra, req.account_index, req.subaddr_indices, m_trusted_daemon); + if (ptx_vector.empty()) + { + er.code = WALLET_RPC_ERROR_CODE_TX_NOT_POSSIBLE; + er.message = "No transaction created"; + return false; + } + // reject proposed transactions if there are more than one. see on_transfer_split below. if (ptx_vector.size() != 1) { From a4240d9ffcef137a273f6042332a62cca43e0983 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Mon, 11 Dec 2017 23:04:57 +0000 Subject: [PATCH 14/24] catch const exceptions --- contrib/epee/include/string_tools.h | 2 +- src/blockchain_db/lmdb/db_lmdb.cpp | 2 +- src/daemon/command_parser_executor.cpp | 6 +++--- src/simplewallet/simplewallet.cpp | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/contrib/epee/include/string_tools.h b/contrib/epee/include/string_tools.h index 307323aa1..5a1ef0743 100644 --- a/contrib/epee/include/string_tools.h +++ b/contrib/epee/include/string_tools.h @@ -161,7 +161,7 @@ DISABLE_GCC_WARNING(maybe-uninitialized) val = boost::lexical_cast(str_id); return true; } - catch(std::exception& /*e*/) + catch(const std::exception& /*e*/) { //const char* pmsg = e.what(); return false; diff --git a/src/blockchain_db/lmdb/db_lmdb.cpp b/src/blockchain_db/lmdb/db_lmdb.cpp index 07a0e67b1..ee4368e86 100644 --- a/src/blockchain_db/lmdb/db_lmdb.cpp +++ b/src/blockchain_db/lmdb/db_lmdb.cpp @@ -2894,7 +2894,7 @@ uint64_t BlockchainLMDB::add_block(const block& blk, const size_t& block_size, c { BlockchainDB::add_block(blk, block_size, cumulative_difficulty, coins_generated, txs); } - catch (DB_ERROR_TXN_START& e) + catch (const DB_ERROR_TXN_START& e) { throw; } diff --git a/src/daemon/command_parser_executor.cpp b/src/daemon/command_parser_executor.cpp index 5307b2472..8970f9407 100644 --- a/src/daemon/command_parser_executor.cpp +++ b/src/daemon/command_parser_executor.cpp @@ -173,7 +173,7 @@ bool t_command_parser_executor::print_block(const std::vector& args uint64_t height = boost::lexical_cast(arg); return m_executor.print_block_by_height(height); } - catch (boost::bad_lexical_cast&) + catch (const boost::bad_lexical_cast&) { crypto::hash block_hash; if (parse_hash256(arg, block_hash)) @@ -420,7 +420,7 @@ bool t_command_parser_executor::out_peers(const std::vector& args) limit = std::stoi(args[0]); } - catch(std::exception& ex) { + catch(const std::exception& ex) { _erro("stoi exception"); return false; } @@ -450,7 +450,7 @@ bool t_command_parser_executor::hard_fork_info(const std::vector& a try { version = std::stoi(args[0]); } - catch(std::exception& ex) { + catch(const std::exception& ex) { return false; } if (version <= 0 || version > 255) diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp index e34e718d2..0a5c9e531 100644 --- a/src/simplewallet/simplewallet.cpp +++ b/src/simplewallet/simplewallet.cpp @@ -5175,7 +5175,7 @@ bool simple_wallet::show_transfers(const std::vector &args_) try { min_height = boost::lexical_cast(local_args[0]); } - catch (boost::bad_lexical_cast &) { + catch (const boost::bad_lexical_cast &) { fail_msg_writer() << tr("bad min_height parameter:") << " " << local_args[0]; return true; } @@ -5187,7 +5187,7 @@ bool simple_wallet::show_transfers(const std::vector &args_) try { max_height = boost::lexical_cast(local_args[0]); } - catch (boost::bad_lexical_cast &) { + catch (const boost::bad_lexical_cast &) { fail_msg_writer() << tr("bad max_height parameter:") << " " << local_args[0]; return true; } From 2305bf260d852135fab53a0a2e95dce8994d7afc Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Tue, 12 Dec 2017 10:58:27 +0000 Subject: [PATCH 15/24] check return value for generate_key_derivation and derive_public_key --- .../cryptonote_format_utils.cpp | 12 +++++++---- src/wallet/wallet2.cpp | 21 ++++++++++++------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/cryptonote_basic/cryptonote_format_utils.cpp b/src/cryptonote_basic/cryptonote_format_utils.cpp index 8f7ab94db..21fa63842 100644 --- a/src/cryptonote_basic/cryptonote_format_utils.cpp +++ b/src/cryptonote_basic/cryptonote_format_utils.cpp @@ -630,17 +630,21 @@ namespace cryptonote bool is_out_to_acc(const account_keys& acc, const txout_to_key& out_key, const crypto::public_key& tx_pub_key, const std::vector& additional_tx_pub_keys, size_t output_index) { crypto::key_derivation derivation; - generate_key_derivation(tx_pub_key, acc.m_view_secret_key, derivation); + bool r = generate_key_derivation(tx_pub_key, acc.m_view_secret_key, derivation); + CHECK_AND_ASSERT_MES(r, false, "Failed to generate key derivation"); crypto::public_key pk; - derive_public_key(derivation, output_index, acc.m_account_address.m_spend_public_key, pk); + r = derive_public_key(derivation, output_index, acc.m_account_address.m_spend_public_key, pk); + CHECK_AND_ASSERT_MES(r, false, "Failed to derive public key"); if (pk == out_key.key) return true; // try additional tx pubkeys if available if (!additional_tx_pub_keys.empty()) { CHECK_AND_ASSERT_MES(output_index < additional_tx_pub_keys.size(), false, "wrong number of additional tx pubkeys"); - generate_key_derivation(additional_tx_pub_keys[output_index], acc.m_view_secret_key, derivation); - derive_public_key(derivation, output_index, acc.m_account_address.m_spend_public_key, pk); + r = generate_key_derivation(additional_tx_pub_keys[output_index], acc.m_view_secret_key, derivation); + CHECK_AND_ASSERT_MES(r, false, "Failed to generate key derivation"); + r = derive_public_key(derivation, output_index, acc.m_account_address.m_spend_public_key, pk); + CHECK_AND_ASSERT_MES(r, false, "Failed to derive public key"); return pk == out_key.key; } return false; diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 88f9e0934..6746f376d 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -6224,7 +6224,8 @@ bool wallet2::light_wallet_parse_rct_str(const std::string& rct_string, const cr if (decrypt) { // Decrypt the mask crypto::key_derivation derivation; - generate_key_derivation(tx_pub_key, get_account().get_keys().m_view_secret_key, derivation); + bool r = generate_key_derivation(tx_pub_key, get_account().get_keys().m_view_secret_key, derivation); + THROW_WALLET_EXCEPTION_IF(!r, error::wallet_internal_error, "Failed to generate key derivation"); crypto::secret_key scalar; crypto::derivation_to_scalar(derivation, internal_output_index, scalar); sc_sub(decrypted_mask.bytes,encrypted_mask.bytes,rct::hash_to_scalar(rct::sk2rct(scalar)).bytes); @@ -7414,12 +7415,14 @@ void wallet2::check_tx_key_helper(const crypto::hash &txid, const crypto::key_de continue; crypto::public_key derived_out_key; - derive_public_key(derivation, n, address.m_spend_public_key, derived_out_key); + bool r = derive_public_key(derivation, n, address.m_spend_public_key, derived_out_key); + THROW_WALLET_EXCEPTION_IF(!r, error::wallet_internal_error, "Failed to derive public key"); bool found = out_key->key == derived_out_key; crypto::key_derivation found_derivation = derivation; if (!found && !additional_derivations.empty()) { - derive_public_key(additional_derivations[n], n, address.m_spend_public_key, derived_out_key); + r = derive_public_key(additional_derivations[n], n, address.m_spend_public_key, derived_out_key); + THROW_WALLET_EXCEPTION_IF(!r, error::wallet_internal_error, "Failed to derive public key"); found = out_key->key == derived_out_key; found_derivation = additional_derivations[n]; } @@ -7884,13 +7887,15 @@ crypto::public_key wallet2::get_tx_pub_key_from_received_outs(const tools::walle for (size_t i = 0; i < additional_tx_pub_keys.size(); ++i) { additional_derivations.push_back({}); - generate_key_derivation(additional_tx_pub_keys[i], keys.m_view_secret_key, additional_derivations.back()); + bool r = generate_key_derivation(additional_tx_pub_keys[i], keys.m_view_secret_key, additional_derivations.back()); + THROW_WALLET_EXCEPTION_IF(!r, error::wallet_internal_error, "Failed to generate key derivation"); } while (find_tx_extra_field_by_type(tx_extra_fields, pub_key_field, pk_index++)) { const crypto::public_key tx_pub_key = pub_key_field.pub_key; crypto::key_derivation derivation; - generate_key_derivation(tx_pub_key, keys.m_view_secret_key, derivation); + bool r = generate_key_derivation(tx_pub_key, keys.m_view_secret_key, derivation); + THROW_WALLET_EXCEPTION_IF(!r, error::wallet_internal_error, "Failed to generate key derivation"); for (size_t i = 0; i < td.m_tx.vout.size(); ++i) { @@ -8177,13 +8182,15 @@ uint64_t wallet2::import_key_images(const std::vector additional_tx_pub_keys = get_additional_tx_pub_keys_from_extra(spent_tx); std::vector additional_derivations; for (size_t i = 0; i < additional_tx_pub_keys.size(); ++i) { additional_derivations.push_back({}); - generate_key_derivation(additional_tx_pub_keys[i], keys.m_view_secret_key, additional_derivations.back()); + r = generate_key_derivation(additional_tx_pub_keys[i], keys.m_view_secret_key, additional_derivations.back()); + THROW_WALLET_EXCEPTION_IF(!r, error::wallet_internal_error, "Failed to generate key derivation"); } size_t output_index = 0; for (const cryptonote::tx_out& out : spent_tx.vout) From b49ddc766de47b40aa705a0a0c0bb901c743908d Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Tue, 12 Dec 2017 11:08:11 +0000 Subject: [PATCH 16/24] check accessing an element past the end of a container --- src/cryptonote_basic/cryptonote_basic.h | 2 +- src/cryptonote_core/blockchain.cpp | 10 ++++++---- src/cryptonote_core/cryptonote_tx_utils.cpp | 6 ++++++ src/ringct/rctOps.cpp | 3 +++ src/wallet/api/unsigned_transaction.cpp | 4 ++++ src/wallet/wallet2.cpp | 14 +++++++++++++- src/wallet/wallet_rpc_server.cpp | 17 ++++++++++++----- src/wallet/wallet_rpc_server.h | 2 +- 8 files changed, 46 insertions(+), 12 deletions(-) diff --git a/src/cryptonote_basic/cryptonote_basic.h b/src/cryptonote_basic/cryptonote_basic.h index 89dda8c3d..821c21d84 100644 --- a/src/cryptonote_basic/cryptonote_basic.h +++ b/src/cryptonote_basic/cryptonote_basic.h @@ -259,7 +259,7 @@ namespace cryptonote ar.tag("rctsig_prunable"); ar.begin_object(); r = rct_signatures.p.serialize_rctsig_prunable(ar, rct_signatures.type, vin.size(), vout.size(), - vin[0].type() == typeid(txin_to_key) ? boost::get(vin[0]).key_offsets.size() - 1 : 0); + vin.size() > 0 && vin[0].type() == typeid(txin_to_key) ? boost::get(vin[0]).key_offsets.size() - 1 : 0); if (!r || !ar.stream().good()) return false; ar.end_object(); } diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 2fd61e1d2..e774dd794 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -2338,7 +2338,7 @@ void Blockchain::on_new_tx_from_block(const cryptonote::transaction &tx) TIME_MEASURE_FINISH(a); if(m_show_time_stats) { - size_t ring_size = tx.vin[0].type() == typeid(txin_to_key) ? boost::get(tx.vin[0]).key_offsets.size() : 0; + size_t ring_size = !tx.vin.empty() && tx.vin[0].type() == typeid(txin_to_key) ? boost::get(tx.vin[0]).key_offsets.size() : 0; MINFO("HASH: " << "-" << " I/M/O: " << tx.vin.size() << "/" << ring_size << "/" << tx.vout.size() << " H: " << 0 << " chcktx: " << a); } } @@ -2373,7 +2373,7 @@ bool Blockchain::check_tx_inputs(transaction& tx, uint64_t& max_used_block_heigh TIME_MEASURE_FINISH(a); if(m_show_time_stats) { - size_t ring_size = tx.vin[0].type() == typeid(txin_to_key) ? boost::get(tx.vin[0]).key_offsets.size() : 0; + size_t ring_size = !tx.vin.empty() && tx.vin[0].type() == typeid(txin_to_key) ? boost::get(tx.vin[0]).key_offsets.size() : 0; MINFO("HASH: " << get_transaction_hash(tx) << " I/M/O: " << tx.vin.size() << "/" << ring_size << "/" << tx.vout.size() << " H: " << max_used_block_height << " ms: " << a + m_fake_scan_time << " B: " << get_object_blobsize(tx)); } if (!res) @@ -2466,6 +2466,7 @@ bool Blockchain::expand_transaction_2(transaction &tx, const crypto::hash &tx_pr // mixRing - full and simple store it in opposite ways if (rv.type == rct::RCTTypeFull || rv.type == rct::RCTTypeFullBulletproof) { + CHECK_AND_ASSERT_MES(!pubkeys.empty() && !pubkeys[0].empty(), false, "empty pubkeys"); rv.mixRing.resize(pubkeys[0].size()); for (size_t m = 0; m < pubkeys[0].size(); ++m) rv.mixRing[m].clear(); @@ -2480,6 +2481,7 @@ bool Blockchain::expand_transaction_2(transaction &tx, const crypto::hash &tx_pr } else if (rv.type == rct::RCTTypeSimple || rv.type == rct::RCTTypeSimpleBulletproof) { + CHECK_AND_ASSERT_MES(!pubkeys.empty() && !pubkeys[0].empty(), false, "empty pubkeys"); rv.mixRing.resize(pubkeys.size()); for (size_t n = 0; n < pubkeys.size(); ++n) { @@ -2811,7 +2813,7 @@ bool Blockchain::check_tx_inputs(transaction& tx, tx_verification_context &tvc, } for (size_t n = 0; n < tx.vin.size(); ++n) { - if (memcmp(&boost::get(tx.vin[n]).k_image, &rv.p.MGs[n].II[0], 32)) + if (rv.p.MGs[n].II.empty() || memcmp(&boost::get(tx.vin[n]).k_image, &rv.p.MGs[n].II[0], 32)) { MERROR_VER("Failed to check ringct signatures: mismatched key image"); return false; @@ -2864,7 +2866,7 @@ bool Blockchain::check_tx_inputs(transaction& tx, tx_verification_context &tvc, MERROR_VER("Failed to check ringct signatures: Bad MGs size"); return false; } - if (rv.p.MGs[0].II.size() != tx.vin.size()) + if (rv.p.MGs.empty() || rv.p.MGs[0].II.size() != tx.vin.size()) { MERROR_VER("Failed to check ringct signatures: mismatched II/vin sizes"); return false; diff --git a/src/cryptonote_core/cryptonote_tx_utils.cpp b/src/cryptonote_core/cryptonote_tx_utils.cpp index 89f24a4d4..916b1e05a 100644 --- a/src/cryptonote_core/cryptonote_tx_utils.cpp +++ b/src/cryptonote_core/cryptonote_tx_utils.cpp @@ -190,6 +190,12 @@ namespace cryptonote //--------------------------------------------------------------- bool construct_tx_with_tx_key(const account_keys& sender_account_keys, const std::unordered_map& subaddresses, std::vector& sources, const std::vector& destinations, const boost::optional& change_addr, std::vector extra, transaction& tx, uint64_t unlock_time, const crypto::secret_key &tx_key, const std::vector &additional_tx_keys, bool rct, bool bulletproof, rct::multisig_out *msout) { + if (sources.empty()) + { + LOG_ERROR("Empty sources"); + return false; + } + std::vector amount_keys; tx.set_null(); amount_keys.clear(); diff --git a/src/ringct/rctOps.cpp b/src/ringct/rctOps.cpp index a7311482c..cc46d0aa7 100644 --- a/src/ringct/rctOps.cpp +++ b/src/ringct/rctOps.cpp @@ -76,6 +76,7 @@ namespace rct { //Generates a vector of secret key //Mainly used in testing keyV skvGen(size_t rows ) { + CHECK_AND_ASSERT_THROW_MES(rows > 0, "0 keys requested"); keyV rv(rows); size_t i = 0; crypto::rand(rows * sizeof(key), (uint8_t*)&rv[0]); @@ -351,6 +352,7 @@ namespace rct { //This takes the outputs and commitments //and hashes them into a 32 byte sized key key cn_fast_hash(const ctkeyV &PC) { + if (PC.empty()) return rct::hash2rct(crypto::cn_fast_hash("", 0)); key rv; cn_fast_hash(rv, &PC[0], 64*PC.size()); return rv; @@ -367,6 +369,7 @@ namespace rct { //put them in the key vector and it concatenates them //and then hashes them key cn_fast_hash(const keyV &keys) { + if (keys.empty()) return rct::hash2rct(crypto::cn_fast_hash("", 0)); key rv; cn_fast_hash(rv, &keys[0], keys.size() * sizeof(keys[0])); //dp(rv); diff --git a/src/wallet/api/unsigned_transaction.cpp b/src/wallet/api/unsigned_transaction.cpp index 4c8c5ade2..d22719189 100644 --- a/src/wallet/api/unsigned_transaction.cpp +++ b/src/wallet/api/unsigned_transaction.cpp @@ -293,6 +293,10 @@ std::vector UnsignedTransactionImpl::recipientAddress() const // TODO: return integrated address if short payment ID exists std::vector result; for (const auto &utx: m_unsigned_tx_set.txes) { + if (utx.dests.empty()) { + MERROR("empty destinations, skipped"); + continue; + } result.push_back(cryptonote::get_account_address_as_str(m_wallet.m_wallet->testnet(), utx.dests[0].is_subaddress, utx.dests[0].addr)); } return result; diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 6746f376d..e818f576a 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -540,6 +540,11 @@ crypto::hash8 get_short_payment_id(const tools::wallet2::pending_tx &ptx) { if(get_encrypted_payment_id_from_tx_extra_nonce(extra_nonce.nonce, payment_id8)) { + if (ptx.dests.empty()) + { + MWARNING("Encrypted payment id found, but no destinations public key, cannot decrypt"); + return crypto::null_hash8; + } decrypt_payment_id(payment_id8, ptx.dests[0].addr.m_view_public_key, ptx.tx_key); } } @@ -4050,6 +4055,11 @@ crypto::hash wallet2::get_payment_id(const pending_tx &ptx) const crypto::hash8 payment_id8 = null_hash8; if(get_encrypted_payment_id_from_tx_extra_nonce(extra_nonce.nonce, payment_id8)) { + if (ptx.dests.empty()) + { + MWARNING("Encrypted payment id found, but no destinations public key, cannot decrypt"); + return crypto::null_hash; + } if (decrypt_payment_id(payment_id8, ptx.dests[0].addr.m_view_public_key, ptx.tx_key)) { memcpy(payment_id.data, payment_id8.data, 8); @@ -4274,6 +4284,7 @@ bool wallet2::sign_tx(unsigned_tx_set &exported_txs, const std::string &signed_f for (size_t n = 0; n < exported_txs.txes.size(); ++n) { tools::wallet2::tx_construction_data &sd = exported_txs.txes[n]; + THROW_WALLET_EXCEPTION_IF(sd.sources.empty(), error::wallet_internal_error, "Empty sources"); LOG_PRINT_L1(" " << (n+1) << ": " << sd.sources.size() << " inputs, ring size " << sd.sources[0].outputs.size()); signed_txes.ptx.push_back(pending_tx()); tools::wallet2::pending_tx &ptx = signed_txes.ptx.back(); @@ -6637,7 +6648,7 @@ std::vector wallet2::create_transactions_2(std::vector available_for_fee && dsts[0].amount > 0) + if (needed_fee > available_for_fee && !dsts.empty() && dsts[0].amount > 0) { // we don't have enough for the fee, but we've only partially paid the current address, // so we can take the fee from the paid amount, since we'll have to make another tx anyway @@ -8912,6 +8923,7 @@ uint64_t wallet2::get_blockchain_height_by_date(uint16_t year, uint8_t month, ui throw std::runtime_error(oss.str()); } cryptonote::block blk_min, blk_mid, blk_max; + if (res.blocks.size() < 3) throw std::runtime_error("Not enough blocks returned from daemon"); if (!parse_and_validate_block_from_blob(res.blocks[0].block, blk_min)) throw std::runtime_error("failed to parse blob at height " + std::to_string(height_min)); if (!parse_and_validate_block_from_blob(res.blocks[1].block, blk_mid)) throw std::runtime_error("failed to parse blob at height " + std::to_string(height_mid)); if (!parse_and_validate_block_from_blob(res.blocks[2].block, blk_max)) throw std::runtime_error("failed to parse blob at height " + std::to_string(height_max)); diff --git a/src/wallet/wallet_rpc_server.cpp b/src/wallet/wallet_rpc_server.cpp index baf8086f8..ccb7b4bc7 100644 --- a/src/wallet/wallet_rpc_server.cpp +++ b/src/wallet/wallet_rpc_server.cpp @@ -486,7 +486,7 @@ namespace tools return true; } //------------------------------------------------------------------------------------------------------------------------------ - bool wallet_rpc_server::validate_transfer(const std::list& destinations, const std::string& payment_id, std::vector& dsts, std::vector& extra, epee::json_rpc::error& er) + bool wallet_rpc_server::validate_transfer(const std::list& destinations, const std::string& payment_id, std::vector& dsts, std::vector& extra, bool at_least_one_destination, epee::json_rpc::error& er) { crypto::hash8 integrated_payment_id = crypto::null_hash8; std::string extra_nonce; @@ -541,6 +541,13 @@ namespace tools } } + if (at_least_one_destination && dsts.empty()) + { + er.code = WALLET_RPC_ERROR_CODE_ZERO_DESTINATION; + er.message = "No destinations for this transfer"; + return false; + } + if (!payment_id.empty()) { @@ -592,7 +599,7 @@ namespace tools } // validate the transfer requested and populate dsts & extra - if (!validate_transfer(req.destinations, req.payment_id, dsts, extra, er)) + if (!validate_transfer(req.destinations, req.payment_id, dsts, extra, true, er)) { return false; } @@ -690,7 +697,7 @@ namespace tools } // validate the transfer requested and populate dsts & extra; RPC_TRANSFER::request and RPC_TRANSFER_SPLIT::request are identical types. - if (!validate_transfer(req.destinations, req.payment_id, dsts, extra, er)) + if (!validate_transfer(req.destinations, req.payment_id, dsts, extra, true, er)) { return false; } @@ -887,7 +894,7 @@ namespace tools destination.push_back(wallet_rpc::transfer_destination()); destination.back().amount = 0; destination.back().address = req.address; - if (!validate_transfer(destination, req.payment_id, dsts, extra, er)) + if (!validate_transfer(destination, req.payment_id, dsts, extra, true, er)) { return false; } @@ -986,7 +993,7 @@ namespace tools destination.push_back(wallet_rpc::transfer_destination()); destination.back().amount = 0; destination.back().address = req.address; - if (!validate_transfer(destination, req.payment_id, dsts, extra, er)) + if (!validate_transfer(destination, req.payment_id, dsts, extra, true, er)) { return false; } diff --git a/src/wallet/wallet_rpc_server.h b/src/wallet/wallet_rpc_server.h index 79f589623..b2061fa35 100644 --- a/src/wallet/wallet_rpc_server.h +++ b/src/wallet/wallet_rpc_server.h @@ -137,7 +137,7 @@ namespace tools bool on_create_account(const wallet_rpc::COMMAND_RPC_CREATE_ACCOUNT::request& req, wallet_rpc::COMMAND_RPC_CREATE_ACCOUNT::response& res, epee::json_rpc::error& er); bool on_label_account(const wallet_rpc::COMMAND_RPC_LABEL_ACCOUNT::request& req, wallet_rpc::COMMAND_RPC_LABEL_ACCOUNT::response& res, epee::json_rpc::error& er); bool on_getheight(const wallet_rpc::COMMAND_RPC_GET_HEIGHT::request& req, wallet_rpc::COMMAND_RPC_GET_HEIGHT::response& res, epee::json_rpc::error& er); - bool validate_transfer(const std::list& destinations, const std::string& payment_id, std::vector& dsts, std::vector& extra, epee::json_rpc::error& er); + bool validate_transfer(const std::list& destinations, const std::string& payment_id, std::vector& dsts, std::vector& extra, bool at_least_one_destination, epee::json_rpc::error& er); bool on_transfer(const wallet_rpc::COMMAND_RPC_TRANSFER::request& req, wallet_rpc::COMMAND_RPC_TRANSFER::response& res, epee::json_rpc::error& er); bool on_transfer_split(const wallet_rpc::COMMAND_RPC_TRANSFER_SPLIT::request& req, wallet_rpc::COMMAND_RPC_TRANSFER_SPLIT::response& res, epee::json_rpc::error& er); bool on_sweep_dust(const wallet_rpc::COMMAND_RPC_SWEEP_DUST::request& req, wallet_rpc::COMMAND_RPC_SWEEP_DUST::response& res, epee::json_rpc::error& er); From f0568ca6ac63450f388d50a1ee887c21ec0258f0 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 13 Dec 2017 11:04:34 +0000 Subject: [PATCH 17/24] net_parse_helpers: fix regex error checking --- contrib/epee/include/net/net_parse_helpers.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/epee/include/net/net_parse_helpers.h b/contrib/epee/include/net/net_parse_helpers.h index 08d2a2000..708cce0ff 100644 --- a/contrib/epee/include/net/net_parse_helpers.h +++ b/contrib/epee/include/net/net_parse_helpers.h @@ -103,7 +103,7 @@ namespace net_utils STATIC_REGEXP_EXPR_1(rexp_match_uri, "^([^?#]*)(\\?([^#]*))?(#(.*))?", boost::regex::icase | boost::regex::normal); boost::smatch result; - if(!boost::regex_search(uri, result, rexp_match_uri, boost::match_default) && result[0].matched) + if(!(boost::regex_search(uri, result, rexp_match_uri, boost::match_default) && result[0].matched)) { LOG_PRINT_L1("[PARSE URI] regex not matched for uri: " << uri); content.m_path = uri; @@ -139,7 +139,7 @@ namespace net_utils // 12 34 5 6 7 content.port = 0; boost::smatch result; - if(!boost::regex_search(url_str, result, rexp_match_uri, boost::match_default) && result[0].matched) + if(!(boost::regex_search(url_str, result, rexp_match_uri, boost::match_default) && result[0].matched)) { LOG_PRINT_L1("[PARSE URI] regex not matched for uri: " << rexp_match_uri); //content.m_path = uri; From b51dc5668714a88d79ddb410d0dcab1559a4bc98 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 13 Dec 2017 11:21:18 +0000 Subject: [PATCH 18/24] use const refs in for loops for non tiny types --- src/blockchain_utilities/blockchain_import.cpp | 2 +- src/cryptonote_protocol/block_queue.cpp | 2 +- src/daemon/rpc_command_executor.cpp | 2 +- src/debug_utilities/object_sizes.cpp | 2 +- src/wallet/wallet2.cpp | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/blockchain_utilities/blockchain_import.cpp b/src/blockchain_utilities/blockchain_import.cpp index 70d1dd696..758deb7e4 100644 --- a/src/blockchain_utilities/blockchain_import.cpp +++ b/src/blockchain_utilities/blockchain_import.cpp @@ -459,7 +459,7 @@ int import_from_file(cryptonote::core& core, const std::string& import_file_path // tx number 1: coinbase tx // tx number 2 onwards: archived_txs - for (transaction tx : archived_txs) + for (const transaction &tx : archived_txs) { // add blocks with verification. // for Blockchain and blockchain_storage add_new_block(). diff --git a/src/cryptonote_protocol/block_queue.cpp b/src/cryptonote_protocol/block_queue.cpp index 9e38ffbcb..3844d3751 100644 --- a/src/cryptonote_protocol/block_queue.cpp +++ b/src/cryptonote_protocol/block_queue.cpp @@ -385,7 +385,7 @@ float block_queue::get_speed(const boost::uuids::uuid &connection_id) const i->second = (i->second + span.rate) / 2; } float conn_rate = -1, best_rate = 0; - for (auto i: speeds) + for (const auto &i: speeds) { if (i.first == connection_id) conn_rate = i.second; diff --git a/src/daemon/rpc_command_executor.cpp b/src/daemon/rpc_command_executor.cpp index d7ee28baa..8aca668ad 100644 --- a/src/daemon/rpc_command_executor.cpp +++ b/src/daemon/rpc_command_executor.cpp @@ -1611,7 +1611,7 @@ bool t_rpc_command_executor::alt_chain_info() } tools::msg_writer() << boost::lexical_cast(res.chains.size()) << " alternate chains found:"; - for (const auto chain: res.chains) + for (const auto &chain: res.chains) { uint64_t start_height = (chain.height - chain.length + 1); tools::msg_writer() << chain.length << " blocks long, from height " << start_height << " (" << (ires.height - start_height - 1) diff --git a/src/debug_utilities/object_sizes.cpp b/src/debug_utilities/object_sizes.cpp index 82d8a4add..967742229 100644 --- a/src/debug_utilities/object_sizes.cpp +++ b/src/debug_utilities/object_sizes.cpp @@ -51,7 +51,7 @@ class size_logger public: ~size_logger() { - for (const auto i: types) + for (const auto &i: types) std::cout << std::to_string(i.first) << "\t" << i.second << std::endl; } void add(const char *type, size_t size) { types.insert(std::make_pair(size, type)); } diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index e818f576a..169c572bd 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -5126,7 +5126,7 @@ void wallet2::get_outs(std::vector> // if there are just enough outputs to mix with, use all of them. // Eventually this should become impossible. uint64_t num_outs = 0, num_recent_outs = 0; - for (auto he: resp_t.result.histogram) + for (const auto &he: resp_t.result.histogram) { if (he.amount == amount) { From 213e326cc903521639177a639cfacfb9157340d9 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 13 Dec 2017 14:03:46 +0000 Subject: [PATCH 19/24] abstract_tcp_server2: log init_server errors as fatal so they show up by default --- contrib/epee/include/net/abstract_tcp_server2.inl | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/contrib/epee/include/net/abstract_tcp_server2.inl b/contrib/epee/include/net/abstract_tcp_server2.inl index 04d884af2..870f6c2b2 100644 --- a/contrib/epee/include/net/abstract_tcp_server2.inl +++ b/contrib/epee/include/net/abstract_tcp_server2.inl @@ -735,7 +735,17 @@ PRAGMA_WARNING_DISABLE_VS(4355) boost::asio::placeholders::error)); return true; - CATCH_ENTRY_L0("boosted_tcp_server::init_server", false); + } + catch (const std::exception &e) + { + MFATAL("Error starting server: " << e.what()); + return false; + } + catch (...) + { + MFATAL("Error starting server"); + return false; + } } //----------------------------------------------------------------------------- PUSH_WARNINGS From 8e60b81c48f68a2ecd8aa3cc116ae3c7b7065fef Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 13 Dec 2017 14:04:19 +0000 Subject: [PATCH 20/24] cryptonote_core: fix db leak on error --- src/cryptonote_core/blockchain.cpp | 1 + src/cryptonote_core/cryptonote_core.cpp | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index e774dd794..6e45417c0 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -321,6 +321,7 @@ bool Blockchain::init(BlockchainDB* db, const bool testnet, bool offline, const if (!db->is_open()) { LOG_ERROR("Attempted to init Blockchain with unopened DB"); + delete db; return false; } diff --git a/src/cryptonote_core/cryptonote_core.cpp b/src/cryptonote_core/cryptonote_core.cpp index 415657f9c..470a99aa4 100644 --- a/src/cryptonote_core/cryptonote_core.cpp +++ b/src/cryptonote_core/cryptonote_core.cpp @@ -377,7 +377,7 @@ namespace cryptonote // folder might not be a directory, etc, etc catch (...) { } - BlockchainDB* db = new_db(db_type); + std::unique_ptr db(new_db(db_type)); if (db == NULL) { LOG_ERROR("Attempted to use non-existent database type"); @@ -468,7 +468,7 @@ namespace cryptonote m_blockchain_storage.set_user_options(blocks_threads, blocks_per_sync, sync_mode, fast_sync); - r = m_blockchain_storage.init(db, m_testnet, m_offline, test_options); + r = m_blockchain_storage.init(db.release(), m_testnet, m_offline, test_options); r = m_mempool.init(); CHECK_AND_ASSERT_MES(r, false, "Failed to initialize memory pool"); From b1634aa3e8d5feb9dc443d1c0edaeaa64225fe3c Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 13 Dec 2017 18:44:18 +0000 Subject: [PATCH 21/24] blockchain: don't leave dangling pointers in this --- src/cryptonote_core/blockchain.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 6e45417c0..c16d56568 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -490,7 +490,9 @@ bool Blockchain::deinit() } delete m_hardfork; + m_hardfork = NULL; delete m_db; + m_db = NULL; return true; } //------------------------------------------------------------------ From 24f584d90d2b38e0e61e8139d21f84dc087239cb Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 13 Dec 2017 18:45:00 +0000 Subject: [PATCH 22/24] cryptonote_core: remove unused functions with off by one bugs --- src/cryptonote_core/blockchain.cpp | 43 ------------------------- src/cryptonote_core/blockchain.h | 26 --------------- src/cryptonote_core/cryptonote_core.cpp | 15 --------- src/cryptonote_core/cryptonote_core.h | 21 ------------ 4 files changed, 105 deletions(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index c16d56568..709c5e852 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -2053,49 +2053,6 @@ bool Blockchain::get_transactions(const t_ids_container& txs_ids, t_tx_container return true; } //------------------------------------------------------------------ -void Blockchain::print_blockchain(uint64_t start_index, uint64_t end_index) const -{ - LOG_PRINT_L3("Blockchain::" << __func__); - std::stringstream ss; - CRITICAL_REGION_LOCAL(m_blockchain_lock); - auto h = m_db->height(); - if(start_index > h) - { - MERROR("Wrong starter index set: " << start_index << ", expected max index " << h); - return; - } - - for(size_t i = start_index; i <= h && i != end_index; i++) - { - ss << "height " << i << ", timestamp " << m_db->get_block_timestamp(i) << ", cumul_dif " << m_db->get_block_cumulative_difficulty(i) << ", size " << m_db->get_block_size(i) << "\nid\t\t" << m_db->get_block_hash_from_height(i) << "\ndifficulty\t\t" << m_db->get_block_difficulty(i) << ", nonce " << m_db->get_block_from_height(i).nonce << ", tx_count " << m_db->get_block_from_height(i).tx_hashes.size() << std::endl; - } - MCINFO("globlal", "Current blockchain:" << std::endl << ss.str()); -} -//------------------------------------------------------------------ -void Blockchain::print_blockchain_index() const -{ - LOG_PRINT_L3("Blockchain::" << __func__); - std::stringstream ss; - CRITICAL_REGION_LOCAL(m_blockchain_lock); - auto height = m_db->height(); - if (height != 0) - { - for(uint64_t i = 0; i <= height; i++) - { - ss << "height: " << i << ", hash: " << m_db->get_block_hash_from_height(i); - } - } - - MINFO("Current blockchain index:" << std::endl << ss.str()); -} -//------------------------------------------------------------------ -//TODO: remove this function and references to it -void Blockchain::print_blockchain_outs(const std::string& file) const -{ - LOG_PRINT_L3("Blockchain::" << __func__); - return; -} -//------------------------------------------------------------------ // Find the split point between us and foreign blockchain and return // (by reference) the most recent common block hash along with up to // BLOCKS_IDS_SYNCHRONIZING_DEFAULT_COUNT additional (more recent) hashes. diff --git a/src/cryptonote_core/blockchain.h b/src/cryptonote_core/blockchain.h index b76d0555f..2d5307ac0 100644 --- a/src/cryptonote_core/blockchain.h +++ b/src/cryptonote_core/blockchain.h @@ -679,32 +679,6 @@ namespace cryptonote //debug functions - /** - * @brief prints data about a snippet of the blockchain - * - * if start_index is greater than the blockchain height, do nothing - * - * @param start_index height on chain to start at - * @param end_index height on chain to end at - */ - void print_blockchain(uint64_t start_index, uint64_t end_index) const; - - /** - * @brief prints every block's hash - * - * WARNING: This function will absolutely crush a terminal in prints, so - * it is recommended to redirect this output to a log file (or null sink - * if a log file is already set up, as should be the default) - */ - void print_blockchain_index() const; - - /** - * @brief currently does nothing, candidate for removal - * - * @param file - */ - void print_blockchain_outs(const std::string& file) const; - /** * @brief check the blockchain against a set of checkpoints * diff --git a/src/cryptonote_core/cryptonote_core.cpp b/src/cryptonote_core/cryptonote_core.cpp index 470a99aa4..da8be18f2 100644 --- a/src/cryptonote_core/cryptonote_core.cpp +++ b/src/cryptonote_core/cryptonote_core.cpp @@ -1052,21 +1052,6 @@ namespace cryptonote return m_blockchain_storage.find_blockchain_supplement(req_start_block, qblock_ids, blocks, total_height, start_height, max_count); } //----------------------------------------------------------------------------------------------- - void core::print_blockchain(uint64_t start_index, uint64_t end_index) const - { - m_blockchain_storage.print_blockchain(start_index, end_index); - } - //----------------------------------------------------------------------------------------------- - void core::print_blockchain_index() const - { - m_blockchain_storage.print_blockchain_index(); - } - //----------------------------------------------------------------------------------------------- - void core::print_blockchain_outs(const std::string& file) - { - m_blockchain_storage.print_blockchain_outs(file); - } - //----------------------------------------------------------------------------------------------- bool core::get_random_outs_for_amounts(const COMMAND_RPC_GET_RANDOM_OUTPUTS_FOR_AMOUNTS::request& req, COMMAND_RPC_GET_RANDOM_OUTPUTS_FOR_AMOUNTS::response& res) const { return m_blockchain_storage.get_random_outs_for_amounts(req, res); diff --git a/src/cryptonote_core/cryptonote_core.h b/src/cryptonote_core/cryptonote_core.h index 9f84ed303..adc201fb5 100644 --- a/src/cryptonote_core/cryptonote_core.h +++ b/src/cryptonote_core/cryptonote_core.h @@ -600,20 +600,6 @@ namespace cryptonote */ const Blockchain& get_blockchain_storage()const{return m_blockchain_storage;} - /** - * @copydoc Blockchain::print_blockchain - * - * @note see Blockchain::print_blockchain - */ - void print_blockchain(uint64_t start_index, uint64_t end_index) const; - - /** - * @copydoc Blockchain::print_blockchain_index - * - * @note see Blockchain::print_blockchain_index - */ - void print_blockchain_index() const; - /** * @copydoc tx_memory_pool::print_pool * @@ -621,13 +607,6 @@ namespace cryptonote */ std::string print_pool(bool short_format) const; - /** - * @copydoc Blockchain::print_blockchain_outs - * - * @note see Blockchain::print_blockchain_outs - */ - void print_blockchain_outs(const std::string& file); - /** * @copydoc miner::on_synchronized * From e0a61299fbd0ba9ea8dc3d30ac08f41fceab4df6 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 13 Dec 2017 18:45:43 +0000 Subject: [PATCH 23/24] network_throttle: remove unused xxx static member --- contrib/epee/include/net/network_throttle.hpp | 2 -- contrib/epee/src/network_throttle.cpp | 3 --- 2 files changed, 5 deletions(-) diff --git a/contrib/epee/include/net/network_throttle.hpp b/contrib/epee/include/net/network_throttle.hpp index 464b34726..fffd22a6a 100644 --- a/contrib/epee/include/net/network_throttle.hpp +++ b/contrib/epee/include/net/network_throttle.hpp @@ -121,8 +121,6 @@ class network_throttle_manager { friend class connection_basic; // FRIEND - to directly access global throttle-s. !! REMEMBER TO USE LOCKS! friend class connection_basic_pimpl; // ditto - static int xxx; - public: static i_network_throttle & get_global_throttle_in(); ///< singleton ; for friend class ; caller MUST use proper locks! like m_lock_get_global_throttle_in static i_network_throttle & get_global_throttle_inreq(); ///< ditto ; use lock ... use m_lock_get_global_throttle_inreq obviously diff --git a/contrib/epee/src/network_throttle.cpp b/contrib/epee/src/network_throttle.cpp index afacc3e96..dd1640a2e 100644 --- a/contrib/epee/src/network_throttle.cpp +++ b/contrib/epee/src/network_throttle.cpp @@ -71,9 +71,6 @@ boost::mutex network_throttle_manager::m_lock_get_global_throttle_in; boost::mutex network_throttle_manager::m_lock_get_global_throttle_inreq; boost::mutex network_throttle_manager::m_lock_get_global_throttle_out; -int network_throttle_manager::xxx; - - // ================================================================================================ // methods: i_network_throttle & network_throttle_manager::get_global_throttle_in() { From bd5cce07b32efade012a4a8872ea7dd6d3abf1ed Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 13 Dec 2017 18:46:03 +0000 Subject: [PATCH 24/24] network_throttle: fix ineffective locking --- contrib/epee/src/network_throttle-detail.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/contrib/epee/src/network_throttle-detail.cpp b/contrib/epee/src/network_throttle-detail.cpp index 317dde8e0..ed6bc07ed 100644 --- a/contrib/epee/src/network_throttle-detail.cpp +++ b/contrib/epee/src/network_throttle-detail.cpp @@ -231,8 +231,10 @@ network_time_seconds network_throttle::get_sleep_time_after_tick(size_t packet_s } void network_throttle::logger_handle_net(const std::string &filename, double time, size_t size) { - boost::mutex mutex; - mutex.lock(); { + static boost::mutex mutex; + + boost::lock_guard lock(mutex); + { std::fstream file; file.open(filename.c_str(), std::ios::app | std::ios::out ); file.precision(6); @@ -240,7 +242,7 @@ void network_throttle::logger_handle_net(const std::string &filename, double tim _warn("Can't open file " << filename); file << static_cast(time) << " " << static_cast(size/1024) << "\n"; file.close(); - } mutex.unlock(); + } } // fine tune this to decide about sending speed: