Merge pull request #2424

28b72b6e tx_pool: pre-init tvc.m_verifivation_failed before processing (moneromooo-monero)
50a629b2 core_tests: catch (impossible in practice) tx extra api failure (moneromooo-monero)
fee15ef1 wallet2: catch failure to parse address (moneromooo-monero)
1399e26d net_peerlist: remove dead code (moneromooo-monero)
50e09698 tx_pool: guard against failure getting tx hash (moneromooo-monero)
54cc209a wallet_rpc_server: catch failure to create directory (moneromooo-monero)
3e55099c wallet_rpc_server: init m_vm to NULL in ctor (moneromooo-monero)
7d0dde5e wallet_args: remove redundant default value for --log-file (moneromooo-monero)
ed4a3350 wallet2: catch failure to save keys file (moneromooo-monero)
44434c8a wallet2_api: check whether dynamic_cast returns NULL (moneromooo-monero)
92f2f687 core: check return value from parse_hexstr_to_binbuff (moneromooo-monero)
5475692e wallet2_api: remove an unused, uninitialized, field (moneromooo-monero)
a7ba3de1 libwallet_api_tests: initialize newblock_triggered on reset (moneromooo-monero)
b2763ace wallet2_api: init error code to "no error" in the ctor (moneromooo-monero)
b5faac53 get_blockchain_top now returns void (moneromooo-monero)
2e44d8f2 wallet_rpc_server: guard against exceptions (moneromooo-monero)
4230876b simplewallet: guard against I/O exceptions (moneromooo-monero)
06c1e057 daemon: initialize decode_as_json in RPC request (moneromooo-monero)
11f71af5 http_base: init size_t in http_request_info ctor (moneromooo-monero)

contrib/epee/include/net/http_base.h

@@ -155,7 +155,8 @@ namespace net_utils
 			http_request_info():m_http_method(http_method_unknown), 
 				m_http_ver_hi(0), 
 				m_http_ver_lo(0), 
-				m_have_to_block(false)
+				m_have_to_block(false),
+				m_full_request_buf_size(0)
 			{}
 
 			http_method			  m_http_method;

src/cryptonote_core/cryptonote_core.cpp

@@ -211,10 +211,9 @@ namespace cryptonote
     return m_blockchain_storage.get_current_blockchain_height();
   }
   //-----------------------------------------------------------------------------------------------
-  bool core::get_blockchain_top(uint64_t& height, crypto::hash& top_id) const
+  void core::get_blockchain_top(uint64_t& height, crypto::hash& top_id) const
   {
     top_id = m_blockchain_storage.get_tail_id(height);
-    return true;
   }
   //-----------------------------------------------------------------------------------------------
   bool core::get_blocks(uint64_t start_offset, size_t count, std::list<std::pair<cryptonote::blobdata,block>>& blocks, std::list<cryptonote::blobdata>& txs) const

src/cryptonote_core/cryptonote_core.h

@@ -299,10 +299,8 @@ namespace cryptonote
       *
       * @param height return-by-reference height of the block
       * @param top_id return-by-reference hash of the block
-      *
-      * @return true
       */
-     bool get_blockchain_top(uint64_t& height, crypto::hash& top_id) const;
+     void get_blockchain_top(uint64_t& height, crypto::hash& top_id) const;
 
      /**
       * @copydoc Blockchain::get_blocks(uint64_t, size_t, std::list<std::pair<cryptonote::blobdata,block>>&, std::list<transaction>&) const

src/cryptonote_core/cryptonote_tx_utils.cpp

@@ -510,8 +510,9 @@ namespace cryptonote
     std::string genesis_coinbase_tx_hex = config::GENESIS_TX;
 
     blobdata tx_bl;
-    string_tools::parse_hexstr_to_binbuff(genesis_coinbase_tx_hex, tx_bl);
-    bool r = parse_and_validate_tx_from_blob(tx_bl, bl.miner_tx);
+    bool r = string_tools::parse_hexstr_to_binbuff(genesis_coinbase_tx_hex, tx_bl);
+    CHECK_AND_ASSERT_MES(r, false, "failed to parse coinbase tx from hard coded blob");
+    r = parse_and_validate_tx_from_blob(tx_bl, bl.miner_tx);
     CHECK_AND_ASSERT_MES(r, false, "failed to parse coinbase tx from hard coded blob");
     bl.major_version = CURRENT_BLOCK_MAJOR_VERSION;
     bl.minor_version = CURRENT_BLOCK_MINOR_VERSION;

src/cryptonote_core/tx_pool.cpp

@@ -202,6 +202,9 @@ namespace cryptonote
       return false;
     }
 
+    // assume failure during verification steps until success is certain
+    tvc.m_verifivation_failed = true;
+
     time_t receive_time = time(nullptr);
 
     crypto::hash max_used_block_id = null_hash;
@@ -285,9 +288,6 @@ namespace cryptonote
         tvc.m_should_be_relayed = true;
     }
 
-    // assume failure during verification steps until success is certain
-    tvc.m_verifivation_failed = true;
-
     tvc.m_verifivation_failed = false;
 
     MINFO("Transaction added to pool: txid " << id << " bytes: " << blob_size << " fee/byte: " << (fee / (double)blob_size));
@@ -298,7 +298,8 @@ namespace cryptonote
   {
     crypto::hash h = null_hash;
     size_t blob_size = 0;
-    get_transaction_hash(tx, h, blob_size);
+    if (!get_transaction_hash(tx, h, blob_size) || blob_size == 0)
+      return false;
     return add_tx(tx, h, blob_size, tvc, keeped_by_block, relayed, do_not_relay, version);
   }
   //---------------------------------------------------------------------------------

src/daemon/rpc_command_executor.cpp

@@ -700,6 +700,7 @@ bool t_rpc_command_executor::print_transaction(crypto::hash transaction_hash) {
   std::string fail_message = "Problem fetching transaction";
 
   req.txs_hashes.push_back(epee::string_tools::pod_to_hex(transaction_hash));
+  req.decode_as_json = false;
   if (m_is_rpc)
   {
     if (!m_rpc_client->rpc_request(req, res, "/gettransactions", fail_message.c_str()))

src/p2p/net_peerlist.h

@@ -190,35 +190,16 @@ namespace nodetool
       if (ver < 6)
         return;
 
-      if(ver < 3)
-        return;
       CRITICAL_REGION_LOCAL(m_peerlist_lock);
-      if(ver < 4)
-      {
-        //loading data from old storage
-        peers_indexed_old pio; 
-        a & pio;
-        peers_indexed_from_old(pio, m_peers_white);
-        return;
-      }
 
 #if 0
       // trouble loading more than one peer, can't find why
       a & m_peers_white;
       a & m_peers_gray;
+      a & m_peers_anchor;
 #else
       serialize_peers(a, m_peers_white, peerlist_entry(), ver);
       serialize_peers(a, m_peers_gray, peerlist_entry(), ver);
-#endif
-
-      if(ver < 5) {
-        return;
-      }
-
-#if 0
-      // trouble loading more than one peer, can't find why
-      a & m_peers_anchor;
-#else
       serialize_peers(a, m_peers_anchor, anchor_peerlist_entry(), ver);
 #endif
     }

src/rpc/core_rpc_server.cpp

@@ -128,11 +128,7 @@ namespace cryptonote
   {
     CHECK_CORE_BUSY();
     crypto::hash top_hash;
-    if (!m_core.get_blockchain_top(res.height, top_hash))
-    {
-      res.status = "Failed";
-      return false;
-    }
+    m_core.get_blockchain_top(res.height, top_hash);
     ++res.height; // turn top block height into blockchain height
     res.top_block_hash = string_tools::pod_to_hex(top_hash);
     res.target_height = m_core.get_target_blockchain_height();
@@ -1061,13 +1057,7 @@ namespace cryptonote
     }
     uint64_t last_block_height;
     crypto::hash last_block_hash;
-    bool have_last_block_hash = m_core.get_blockchain_top(last_block_height, last_block_hash);
-    if (!have_last_block_hash)
-    {
-      error_resp.code = CORE_RPC_ERROR_CODE_INTERNAL_ERROR;
-      error_resp.message = "Internal error: can't get last block hash.";
-      return false;
-    }
+    m_core.get_blockchain_top(last_block_height, last_block_hash);
     block last_block;
     bool have_last_block = m_core.get_block_by_hash(last_block_hash, last_block);
     if (!have_last_block)
@@ -1300,11 +1290,7 @@ namespace cryptonote
     }
 
     crypto::hash top_hash;
-    if (!m_core.get_blockchain_top(res.height, top_hash))
-    {
-      res.status = "Failed";
-      return false;
-    }
+    m_core.get_blockchain_top(res.height, top_hash);
     ++res.height; // turn top block height into blockchain height
     res.top_block_hash = string_tools::pod_to_hex(top_hash);
     res.target_height = m_core.get_target_blockchain_height();
@@ -1716,11 +1702,7 @@ namespace cryptonote
     }
 
     crypto::hash top_hash;
-    if (!m_core.get_blockchain_top(res.height, top_hash))
-    {
-      res.status = "Failed";
-      return false;
-    }
+    m_core.get_blockchain_top(res.height, top_hash);
     ++res.height; // turn top block height into blockchain height
     res.target_height = m_core.get_target_blockchain_height();
 

src/simplewallet/simplewallet.cpp

@@ -1708,9 +1708,18 @@ bool simple_wallet::open_wallet(const boost::program_options::variables_map& vm)
   catch (const std::exception& e)
   {
     fail_msg_writer() << tr("failed to load wallet: ") << e.what();
+    if (m_wallet)
+    {
       // only suggest removing cache if the password was actually correct
-    if (m_wallet && m_wallet->verify_password(password))
+      bool password_is_correct = false;
+      try
+      {
+        password_is_correct = m_wallet->verify_password(password);
+      }
+      catch (...) { } // guard against I/O errors
+      if (password_is_correct)
         fail_msg_writer() << boost::format(tr("You may want to remove the file \"%s\" and try again")) % m_wallet_file;
+    }
     return false;
   }
   success_msg_writer() <<

src/wallet/api/address_book.cpp

@@ -42,7 +42,7 @@ namespace Monero {
 AddressBook::~AddressBook() {}
   
 AddressBookImpl::AddressBookImpl(WalletImpl *wallet)
-    : m_wallet(wallet) {}
+    : m_wallet(wallet), m_errorCode(Status_Ok) {}
 
 bool AddressBookImpl::addRow(const std::string &dst_addr , const std::string &payment_id_str, const std::string &description)
 {

src/wallet/api/wallet.h

@@ -153,7 +153,6 @@ private:
     std::string m_password;
     TransactionHistoryImpl * m_history;
     bool        m_trustedDaemon;
-    WalletListener * m_walletListener;
     Wallet2CallbackImpl * m_wallet2Callback;
     AddressBookImpl *  m_addressBook;
 

src/wallet/api/wallet_manager.cpp

@@ -105,6 +105,8 @@ Wallet *WalletManagerImpl::createWalletFromKeys(const std::string &path,
 bool WalletManagerImpl::closeWallet(Wallet *wallet, bool store)
 {
     WalletImpl * wallet_ = dynamic_cast<WalletImpl*>(wallet);
+    if (!wallet_)
+        return false;
     bool result = wallet_->close(store);
     if (!result) {
         m_errorString = wallet_->errorString();

src/wallet/wallet2.cpp

@@ -383,7 +383,11 @@ std::unique_ptr<tools::wallet2> generate_from_json(const std::string& json_file,
             cryptonote::account_public_address address2;
             bool has_payment_id;
             crypto::hash8 new_payment_id;
-            get_account_integrated_address_from_str(address2, has_payment_id, new_payment_id, testnet, field_address);
+            if (!get_account_integrated_address_from_str(address2, has_payment_id, new_payment_id, testnet, field_address))
+            {
+              tools::fail_msg_writer() << tools::wallet2::tr("failed to parse address: ") << field_address;
+              return false;
+            }
             address.m_spend_public_key = address2.m_spend_public_key;
           }
           wallet->generate(field_filename, field_password, address, viewkey);
@@ -2653,10 +2657,11 @@ void wallet2::store_to(const std::string &path, const std::string &password)
   // if we here, main wallet file is saved and we only need to save keys and address files
   if (!same_file) {
     prepare_file_names(path);
-    store_keys(m_keys_file, password, false);
+    bool r = store_keys(m_keys_file, password, false);
+    THROW_WALLET_EXCEPTION_IF(!r, error::file_save_error, m_keys_file);
     // save address to the new file
     const std::string address_file = m_wallet_file + ".address.txt";
-    bool r = file_io_utils::save_string_to_file(address_file, m_account.get_public_address_str(m_testnet));
+    r = file_io_utils::save_string_to_file(address_file, m_account.get_public_address_str(m_testnet));
     THROW_WALLET_EXCEPTION_IF(!r, error::file_save_error, m_wallet_file);
     // remove old wallet file
     r = boost::filesystem::remove(old_file);

src/wallet/wallet_args.cpp

@@ -99,7 +99,7 @@ namespace wallet_args
     command_line::add_arg(desc_general, command_line::arg_help);
     command_line::add_arg(desc_general, command_line::arg_version);
 
-    command_line::add_arg(desc_params, arg_log_file, "");
+    command_line::add_arg(desc_params, arg_log_file);
     command_line::add_arg(desc_params, arg_log_level);
     command_line::add_arg(desc_params, arg_max_concurrency);
     command_line::add_arg(desc_params, arg_config_file);

src/wallet/wallet_rpc_server.cpp

@@ -27,6 +27,7 @@
 // THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 // 
 // Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers
+#include <boost/format.hpp>
 #include <boost/asio/ip/address.hpp>
 #include <boost/filesystem/operations.hpp>
 #include <cstdint>
@@ -69,7 +70,7 @@ namespace tools
   }
 
   //------------------------------------------------------------------------------------------------------------------------------
-  wallet_rpc_server::wallet_rpc_server():m_wallet(NULL), rpc_login_file(), m_stop(false), m_trusted_daemon(false)
+  wallet_rpc_server::wallet_rpc_server():m_wallet(NULL), rpc_login_file(), m_stop(false), m_trusted_daemon(false), m_vm(NULL)
   {
   }
   //------------------------------------------------------------------------------------------------------------------------------
@@ -153,7 +154,15 @@ namespace tools
 #else
 #define MKDIR(path, mode)    mkdir(path, mode)
 #endif
-      MKDIR(m_wallet_dir.c_str(), 0700);
+      if (MKDIR(m_wallet_dir.c_str(), 0700) < 0)
+      {
+#ifdef _WIN32
+        LOG_ERROR(tr("Failed to create directory ") + m_wallet_dir);
+#else
+        LOG_ERROR((boost::format(tr("Failed to create directory %s: %s")) % m_wallet_dir % strerror(errno)).str());
+#endif
+        return false;
+      }
     }
 
     if (disable_auth)
@@ -1887,7 +1896,15 @@ just_dir:
     wrpc.send_stop_signal();
   });
   LOG_PRINT_L0(tools::wallet_rpc_server::tr("Starting wallet rpc server"));
+  try
+  {
     wrpc.run();
+  }
+  catch (const std::exception &e)
+  {
+    LOG_ERROR(tools::wallet_rpc_server::tr("Failed to run wallet: ") << e.what());
+    return 1;
+  }
   LOG_PRINT_L0(tools::wallet_rpc_server::tr("Stopped wallet rpc server"));
   try
   {

tests/core_proxy/core_proxy.cpp

@@ -229,10 +229,9 @@ bool tests::proxy_core::get_short_chain_history(std::list<crypto::hash>& ids) {
     return true;
 }
 
-bool tests::proxy_core::get_blockchain_top(uint64_t& height, crypto::hash& top_id) {
+void tests::proxy_core::get_blockchain_top(uint64_t& height, crypto::hash& top_id) {
     height = 0;
     top_id = get_block_hash(m_genesis);
-    return true;
 }
 
 bool tests::proxy_core::init(const boost::program_options::variables_map& /*vm*/) {

tests/core_proxy/core_proxy.h

@@ -74,7 +74,7 @@ namespace tests
     bool get_short_chain_history(std::list<crypto::hash>& ids);
     bool get_stat_info(cryptonote::core_stat_info& st_inf){return true;}
     bool have_block(const crypto::hash& id);
-    bool get_blockchain_top(uint64_t& height, crypto::hash& top_id);
+    void get_blockchain_top(uint64_t& height, crypto::hash& top_id);
     bool handle_incoming_tx(const cryptonote::blobdata& tx_blob, cryptonote::tx_verification_context& tvc, bool keeped_by_block, bool relayed, bool do_not_relay);
     bool handle_incoming_txs(const std::list<cryptonote::blobdata>& tx_blobs, std::vector<cryptonote::tx_verification_context>& tvc, bool keeped_by_block, bool relayed, bool do_not_relay);
     bool handle_incoming_block(const cryptonote::blobdata& block_blob, cryptonote::block_verification_context& bvc, bool update_miner_blocktemplate = true);

tests/core_tests/rct.cpp

@@ -484,8 +484,9 @@ bool gen_rct_tx_pre_rct_altered_extra::generate(std::vector<test_event_entry>& e
   const int mixin = 2;
   const int out_idx[] = {0, -1};
   const uint64_t amount_paid = 10000;
+  bool failed = false;
   return generate_with(events, out_idx, mixin, amount_paid, false,
-    NULL, [](transaction &tx) {std::string extra_nonce; crypto::hash pid = cryptonote::null_hash; set_payment_id_to_tx_extra_nonce(extra_nonce, pid); add_extra_nonce_to_tx_extra(tx.extra, extra_nonce);});
+    NULL, [&failed](transaction &tx) {std::string extra_nonce; crypto::hash pid = cryptonote::null_hash; set_payment_id_to_tx_extra_nonce(extra_nonce, pid); if (!add_extra_nonce_to_tx_extra(tx.extra, extra_nonce)) failed = true; }) && !failed;
 }
 
 bool gen_rct_tx_rct_altered_extra::generate(std::vector<test_event_entry>& events) const
@@ -493,7 +494,8 @@ bool gen_rct_tx_rct_altered_extra::generate(std::vector<test_event_entry>& event
   const int mixin = 2;
   const int out_idx[] = {1, -1};
   const uint64_t amount_paid = 10000;
+  bool failed = false;
   return generate_with(events, out_idx, mixin, amount_paid, false,
-    NULL, [](transaction &tx) {std::string extra_nonce; crypto::hash pid = cryptonote::null_hash; set_payment_id_to_tx_extra_nonce(extra_nonce, pid); add_extra_nonce_to_tx_extra(tx.extra, extra_nonce);});
+    NULL, [&failed](transaction &tx) {std::string extra_nonce; crypto::hash pid = cryptonote::null_hash; set_payment_id_to_tx_extra_nonce(extra_nonce, pid); if (!add_extra_nonce_to_tx_extra(tx.extra, extra_nonce)) failed = true; }) && !failed;
 }
 

tests/libwallet_api_tests/main.cpp

@@ -811,7 +811,7 @@ struct MyWalletListener : public Monero::WalletListener
 
     void reset()
     {
-        send_triggered = receive_triggered = update_triggered = refresh_triggered = false;
+        send_triggered = receive_triggered = newblock_triggered = update_triggered = refresh_triggered = false;
     }
 
     virtual void moneySpent(const string &txId, uint64_t amount)

tests/unit_tests/ban.cpp

@@ -51,7 +51,7 @@ public:
   bool get_short_chain_history(std::list<crypto::hash>& ids) const { return true; }
   bool get_stat_info(cryptonote::core_stat_info& st_inf) const {return true;}
   bool have_block(const crypto::hash& id) const {return true;}
-  bool get_blockchain_top(uint64_t& height, crypto::hash& top_id)const{height=0;top_id=cryptonote::null_hash;return true;}
+  void get_blockchain_top(uint64_t& height, crypto::hash& top_id)const{height=0;top_id=cryptonote::null_hash;}
   bool handle_incoming_tx(const cryptonote::blobdata& tx_blob, cryptonote::tx_verification_context& tvc, bool keeped_by_block, bool relayed, bool do_not_relay) { return true; }
   bool handle_incoming_txs(const std::list<cryptonote::blobdata>& tx_blob, std::vector<cryptonote::tx_verification_context>& tvc, bool keeped_by_block, bool relayed, bool do_not_relay) { return true; }
   bool handle_incoming_block(const cryptonote::blobdata& block_blob, cryptonote::block_verification_context& bvc, bool update_miner_blocktemplate = true) { return true; }