Merge pull request #3226

e4646379 keccak: fix mdlen bounds sanity checking (moneromooo-monero)
2e3e90ac pass large parameters by const ref, not value (moneromooo-monero)
61defd89 blockchain: sanity check number of precomputed hash of hash blocks (moneromooo-monero)
9af6b2d1 ringct: fix infinite loop in unused h2b function (moneromooo-monero)
8cea8d0c simplewallet: double check a new multisig wallet is multisig (moneromooo-monero)
9b98a6ac threadpool: catch exceptions in dtor, to avoid terminate (moneromooo-monero)
24803ed9 blockchain_export: fix buffer overflow in exporter (moneromooo-monero)
f3f7da62 perf_timer: rewrite to make it clear there is no division by zero (moneromooo-monero)
c6ea3df0 performance_tests: remove add_arg call stray extra param (moneromooo-monero)
fa6b4566 fuzz_tests: fix an uninitialized var in setup (moneromooo-monero)
03887f11 keccak: fix sanity check bounds test (moneromooo-monero)
ad11db91 blockchain_db: initialize m_open in base class ctor (moneromooo-monero)
bece67f9 miner: restore std::cout precision after modification (moneromooo-monero)
1aabd14c db_lmdb: check hard fork info drop succeeded (moneromooo-monero)

14 files changed, 56 insertions, 18 deletions

diff --git a/src/blockchain_db/berkeleydb/db_bdb.cpp b/src/blockchain_db/berkeleydb/db_bdb.cpp
index f540ce133..3a66ecb93 100644
--- a/src/blockchain_db/berkeleydb/db_bdb.cpp
+++ b/src/blockchain_db/berkeleydb/db_bdb.cpp
@@ -770,13 +770,13 @@ BlockchainBDB::~BlockchainBDB()
 }
 
 BlockchainBDB::BlockchainBDB(bool batch_transactions) :
+        BlockchainDB(),
         m_buffer(DB_BUFFER_COUNT, DB_BUFFER_LENGTH)
 {
     LOG_PRINT_L3("BlockchainBDB::" << __func__);
     // initialize folder to something "safe" just in case
     // someone accidentally misuses this class...
     m_folder = "thishsouldnotexistbecauseitisgibberish";
-    m_open = false;
     m_run_checkpoint = 0;
     m_batch_transactions = batch_transactions;
     m_write_txn = nullptr;
diff --git a/src/blockchain_db/blockchain_db.h b/src/blockchain_db/blockchain_db.h
index 227169614..cce288793 100644
--- a/src/blockchain_db/blockchain_db.h
+++ b/src/blockchain_db/blockchain_db.h
@@ -538,6 +538,11 @@ protected:
 public:
 
   /**
+   * @brief An empty constructor.
+   */
+  BlockchainDB(): m_open(false) { }
+
+  /**
    * @brief An empty destructor.
    */
   virtual ~BlockchainDB() { };
diff --git a/src/blockchain_db/lmdb/db_lmdb.cpp b/src/blockchain_db/lmdb/db_lmdb.cpp
index 5210fc3cd..fd8aad31d 100644
--- a/src/blockchain_db/lmdb/db_lmdb.cpp
+++ b/src/blockchain_db/lmdb/db_lmdb.cpp
@@ -1080,13 +1080,12 @@ BlockchainLMDB::~BlockchainLMDB()
     close();
 }
 
-BlockchainLMDB::BlockchainLMDB(bool batch_transactions)
+BlockchainLMDB::BlockchainLMDB(bool batch_transactions): BlockchainDB()
 {
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
   // initialize folder to something "safe" just in case
   // someone accidentally misuses this class...
   m_folder = "thishsouldnotexistbecauseitisgibberish";
-  m_open = false;
 
   m_batch_transactions = batch_transactions;
   m_write_txn = nullptr;
@@ -3147,8 +3146,12 @@ void BlockchainLMDB::drop_hard_fork_info()
 
   TXN_PREFIX(0);
 
-  mdb_drop(*txn_ptr, m_hf_starting_heights, 1);
-  mdb_drop(*txn_ptr, m_hf_versions, 1);
+  auto result = mdb_drop(*txn_ptr, m_hf_starting_heights, 1);
+  if (result)
+    throw1(DB_ERROR(lmdb_error("Error dropping hard fork starting heights db: ", result).c_str()));
+  result = mdb_drop(*txn_ptr, m_hf_versions, 1);
+  if (result)
+    throw1(DB_ERROR(lmdb_error("Error dropping hard fork versions db: ", result).c_str()));
 
   TXN_POSTFIX_SUCCESS();
 }
diff --git a/src/blockchain_utilities/blocksdat_file.cpp b/src/blockchain_utilities/blocksdat_file.cpp
index 2bad86dfd..45ef33acb 100644
--- a/src/blockchain_utilities/blocksdat_file.cpp
+++ b/src/blockchain_utilities/blocksdat_file.cpp
@@ -106,7 +106,7 @@ void BlocksdatFile::write_block(const crypto::hash& block_hash)
   {
     crypto::hash hash;
     crypto::cn_fast_hash(m_hashes.data(), HASH_OF_HASHES_STEP * sizeof(crypto::hash), hash);
-    memmove(m_hashes.data(), m_hashes.data() + HASH_OF_HASHES_STEP * sizeof(crypto::hash), (m_hashes.size() - HASH_OF_HASHES_STEP) * sizeof(crypto::hash));
+    memmove(m_hashes.data(), m_hashes.data() + HASH_OF_HASHES_STEP, (m_hashes.size() - HASH_OF_HASHES_STEP) * sizeof(crypto::hash));
     m_hashes.resize(m_hashes.size() - HASH_OF_HASHES_STEP);
     const std::string data(hash.data, sizeof(hash));
     *m_raw_data_file << data;
diff --git a/src/common/perf_timer.cpp b/src/common/perf_timer.cpp
index 41e23130d..16abdfd99 100644
--- a/src/common/perf_timer.cpp
+++ b/src/common/perf_timer.cpp
@@ -49,16 +49,15 @@ namespace
 #ifdef __x86_64__
   uint64_t get_ticks_per_ns()
   {
-    uint64_t t0 = epee::misc_utils::get_ns_count();
+    uint64_t t0 = epee::misc_utils::get_ns_count(), t1;
     uint64_t r0 = get_tick_count();
 
     while (1)
     {
-      uint64_t t = epee::misc_utils::get_ns_count();
-      if (t - t0 > 1*1000000000) break; // work one second
+      t1 = epee::misc_utils::get_ns_count();
+      if (t1 - t0 > 1*1000000000) break; // work one second
     }
 
-    uint64_t t1 = epee::misc_utils::get_ns_count();
     uint64_t r1 = get_tick_count();
     uint64_t tpns256 = 256 * (r1 - r0) / (t1 - t0);
     return tpns256 ? tpns256 : 1;
diff --git a/src/common/threadpool.cpp b/src/common/threadpool.cpp
index 7fd16ceaf..51e071577 100644
--- a/src/common/threadpool.cpp
+++ b/src/common/threadpool.cpp
@@ -25,6 +25,7 @@
 // INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
 // STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
 // THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#include "misc_log_ex.h"
 #include "common/threadpool.h"
 
 #include <cassert>
@@ -81,6 +82,23 @@ int threadpool::get_max_concurrency() {
   return max;
 }
 
+threadpool::waiter::~waiter()
+{
+  {
+    boost::unique_lock<boost::mutex> lock(mt);
+    if (num)
+      MERROR("wait should have been called before waiter dtor - waiting now");
+  }
+  try
+  {
+    wait();
+  }
+  catch (const std::exception &e)
+  {
+    /* ignored */
+  }
+}
+
 void threadpool::waiter::wait() {
   boost::unique_lock<boost::mutex> lock(mt);
   while(num) cv.wait(lock);
diff --git a/src/common/threadpool.h b/src/common/threadpool.h
index a0e53b011..34152541c 100644
--- a/src/common/threadpool.h
+++ b/src/common/threadpool.h
@@ -34,6 +34,7 @@
 #include <functional>
 #include <utility>
 #include <vector>
+#include <stdexcept>
 
 namespace tools
 {
@@ -57,7 +58,7 @@ public:
     void dec();
     void wait();  //! Wait for a set of tasks to finish.
     waiter() : num(0){}
-    ~waiter() { wait(); }
+    ~waiter();
   };
 
   // Submit a task to the pool. The waiter pointer may be
diff --git a/src/crypto/keccak.c b/src/crypto/keccak.c
index 528a5406b..de8e2a5b3 100644
--- a/src/crypto/keccak.c
+++ b/src/crypto/keccak.c
@@ -92,7 +92,8 @@ void keccak(const uint8_t *in, size_t inlen, uint8_t *md, int mdlen)
     uint8_t temp[144];
     size_t i, rsiz, rsizw;
 
-    if (mdlen <= 0 || mdlen > 200 || sizeof(st) != 200)
+    static_assert(HASH_DATA_AREA <= sizeof(temp), "Bad keccak preconditions");
+    if (mdlen <= 0 || (mdlen > 100 && sizeof(st) != (size_t)mdlen))
     {
       local_abort("Bad keccak use");
     }
@@ -109,7 +110,7 @@ void keccak(const uint8_t *in, size_t inlen, uint8_t *md, int mdlen)
     }
     
     // last block and padding
-    if (inlen >= sizeof(temp) || inlen > rsiz || rsiz - inlen + inlen + 1 >= sizeof(temp) || rsiz == 0 || rsiz - 1 >= sizeof(temp) || rsizw * 8 > sizeof(temp))
+    if (inlen + 1 >= sizeof(temp) || inlen > rsiz || rsiz - inlen + inlen + 1 >= sizeof(temp) || rsiz == 0 || rsiz - 1 >= sizeof(temp) || rsizw * 8 > sizeof(temp))
     {
       local_abort("Bad keccak use");
     }
diff --git a/src/cryptonote_basic/miner.cpp b/src/cryptonote_basic/miner.cpp
index 6c4ecf58c..b322383a9 100644
--- a/src/cryptonote_basic/miner.cpp
+++ b/src/cryptonote_basic/miner.cpp
@@ -198,7 +198,8 @@ namespace cryptonote
       {
         uint64_t total_hr = std::accumulate(m_last_hash_rates.begin(), m_last_hash_rates.end(), 0);
         float hr = static_cast<float>(total_hr)/static_cast<float>(m_last_hash_rates.size());
-        std::cout << "hashrate: " << std::setprecision(4) << std::fixed << hr << ENDL;
+        const auto precision = std::cout.precision();
+        std::cout << "hashrate: " << std::setprecision(4) << std::fixed << hr << precision << ENDL;
       }
     }
     m_last_hr_merge_time = misc_utils::get_tick_count();
diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp
index f165aa46c..fe4004caa 100644
--- a/src/cryptonote_core/blockchain.cpp
+++ b/src/cryptonote_core/blockchain.cpp
@@ -4348,8 +4348,13 @@ void Blockchain::load_compiled_in_block_hashes()
     {
       const unsigned char *p = get_blocks_dat_start(m_testnet);
       const uint32_t nblocks = *p | ((*(p+1))<<8) | ((*(p+2))<<16) | ((*(p+3))<<24);
+      if (nblocks > (std::numeric_limits<uint32_t>::max() - 4) / sizeof(hash))
+      {
+        MERROR("Block hash data is too large");
+        return;
+      }
       const size_t size_needed = 4 + nblocks * sizeof(crypto::hash);
-      if(nblocks > 0 && nblocks * HASH_OF_HASHES_STEP > m_db->height() && get_blocks_dat_size(m_testnet) >= size_needed)
+      if(nblocks > 0 && nblocks > (m_db->height() + HASH_OF_HASHES_STEP - 1) / HASH_OF_HASHES_STEP && get_blocks_dat_size(m_testnet) >= size_needed)
       {
         p += sizeof(uint32_t);
         m_blocks_hash_of_hashes.reserve(nblocks);
diff --git a/src/p2p/net_node.h b/src/p2p/net_node.h
index 568c650cc..9ebefbca6 100644
--- a/src/p2p/net_node.h
+++ b/src/p2p/net_node.h
@@ -217,7 +217,7 @@ namespace nodetool
     void add_upnp_port_mapping(uint32_t port);
     void delete_upnp_port_mapping(uint32_t port);
     template<class t_callback>
-    bool try_ping(basic_node_data& node_data, p2p_connection_context& context, t_callback cb);
+    bool try_ping(basic_node_data& node_data, p2p_connection_context& context, const t_callback &cb);
     bool try_get_support_flags(const p2p_connection_context& context, std::function<void(p2p_connection_context&, const uint32_t&)> f);
     bool make_expected_connections_count(PeerType peer_type, size_t expected_connections);
     void cache_connect_fail_info(const epee::net_utils::network_address& addr);
diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl
index ee7e62768..ea7fc51ee 100644
--- a/src/p2p/net_node.inl
+++ b/src/p2p/net_node.inl
@@ -1505,7 +1505,7 @@ namespace nodetool
   }
   //-----------------------------------------------------------------------------------
   template<class t_payload_net_handler> template<class t_callback>
-  bool node_server<t_payload_net_handler>::try_ping(basic_node_data& node_data, p2p_connection_context& context, t_callback cb)
+  bool node_server<t_payload_net_handler>::try_ping(basic_node_data& node_data, p2p_connection_context& context, const t_callback &cb)
   {
     if(!node_data.my_port)
       return false;
diff --git a/src/ringct/rctTypes.cpp b/src/ringct/rctTypes.cpp
index 1526dcf7c..5650b3ba1 100644
--- a/src/ringct/rctTypes.cpp
+++ b/src/ringct/rctTypes.cpp
@@ -178,6 +178,7 @@ namespace rct {
             }
             while (i < 8 * (j + 1)) {
                 amountb2[i] = 0;
+                i++;
             }
         }
     }
diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp
index 4ad1b83ec..2d8eb97e0 100644
--- a/src/simplewallet/simplewallet.cpp
+++ b/src/simplewallet/simplewallet.cpp
@@ -817,7 +817,11 @@ bool simple_wallet::make_multisig(const std::vector<std::string> &args)
   }
 
   uint32_t total;
-  m_wallet->multisig(NULL, &threshold, &total);
+  if (!m_wallet->multisig(NULL, &threshold, &total))
+  {
+    fail_msg_writer() << tr("Error creating multisig: new wallet is not multisig");
+    return true;
+  }
   success_msg_writer() << std::to_string(threshold) << "/" << total << tr(" multisig address: ")
       << m_wallet->get_account().get_public_address_str(m_wallet->testnet());