Merge pull request #165 from pmconrad/node_crashes

Changes wrt recent crashes of nodes

include/fc/container/zeroed_array.hpp

@@ -0,0 +1,74 @@
+/*
+ * Copyright (c) 2019 BitShares Blockchain Foundation, and contributors
+ *
+ * The MIT License
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#pragma once
+#include <fc/io/raw_fwd.hpp>
+
+namespace fc {
+
+   template< typename T, size_t N >
+   class zero_initialized_array;
+
+   template< size_t N >
+   class zero_initialized_array< unsigned char, N > : public std::array< unsigned char, N > {
+      public:
+         zero_initialized_array() : std::array< unsigned char, N >() { }
+   };
+
+   template<typename T,size_t N>
+   struct get_typename< zero_initialized_array<T,N> >
+   {
+      static const char* name()
+      {
+         static std::string _name = std::string("zero_initialized_array<")
+                                    + std::string(fc::get_typename<T>::name())
+                                    + "," + fc::to_string(N) + ">";
+         return _name.c_str();
+      }
+   };
+
+   class variant;
+   template<size_t N>
+   void to_variant( const zero_initialized_array<unsigned char,N>& bi, variant& v, uint32_t max_depth = 1 )
+   {
+      to_variant( static_cast<const std::array<unsigned char,N>&>( bi ), v, max_depth );
+   }
+   template<size_t N>
+   void from_variant( const variant& v, zero_initialized_array<unsigned char,N>& bi, uint32_t max_depth = 1 )
+   {
+      from_variant( v, static_cast<std::array<unsigned char,N>&>( bi ), max_depth );
+   }
+
+   namespace raw {
+      template<typename Stream, size_t N>
+      inline void pack( Stream& s, const zero_initialized_array<unsigned char,N>& v, uint32_t _max_depth ) {
+         pack( s, static_cast<const std::array<unsigned char,N>&>( v ), _max_depth );
+      }
+      template<typename Stream, size_t N>
+      inline void unpack( Stream& s, zero_initialized_array<unsigned char,N>& v, uint32_t _max_depth ) {
+         try {
+            unpack( s, static_cast<std::array<unsigned char,N>&>( v ), _max_depth );
+         } FC_RETHROW_EXCEPTIONS( warn, "zero_initialized_array<unsigned char,${length}>", ("length",N) )
+      }
+   }
+}

include/fc/crypto/elliptic.hpp

@@ -1,4 +1,5 @@
 #pragma once
+#include <fc/container/zeroed_array.hpp>
 #include <fc/crypto/bigint.hpp>
 #include <fc/crypto/openssl.hpp>
 #include <fc/crypto/sha256.hpp>
@@ -17,15 +18,15 @@ namespace fc {
       class private_key_impl;
     }
 
-    typedef fc::sha256                   blind_factor_type;
+    typedef fc::sha256                               blind_factor_type;
-    typedef std::array<unsigned char,33> commitment_type;
+    typedef zero_initialized_array<unsigned char,33> commitment_type;
-    typedef std::array<unsigned char,33> public_key_data;
+    typedef zero_initialized_array<unsigned char,33> public_key_data;
-    typedef fc::sha256                   private_key_secret;
+    typedef fc::sha256                               private_key_secret;
-    typedef std::array<unsigned char,65> public_key_point_data; ///< the full non-compressed version of the ECC point
+    typedef zero_initialized_array<unsigned char,65> public_key_point_data; ///< the full non-compressed version of the ECC point
-    typedef std::array<unsigned char,72> signature;
+    typedef zero_initialized_array<unsigned char,72> signature;
-    typedef std::array<unsigned char,65> compact_signature;
+    typedef zero_initialized_array<unsigned char,65> compact_signature;
-    typedef std::vector<char>            range_proof_type;
+    typedef std::vector<char>                        range_proof_type;
-    typedef std::array<unsigned char,78> extended_key_data;
+    typedef zero_initialized_array<unsigned char,78> extended_key_data;
 
     /**
      *  @class public_key

include/fc/thread/future.hpp

@@ -3,9 +3,9 @@
 #include <fc/exception/exception.hpp>
 #include <fc/thread/spin_yield_lock.hpp>
 #include <fc/optional.hpp>
-#include <memory>
 
-#include <boost/atomic.hpp>
+#include <atomic>
+#include <memory>
 
 //#define FC_TASK_NAMES_ARE_MANDATORY 1
 #ifdef FC_TASK_NAMES_ARE_MANDATORY
@@ -77,23 +77,22 @@ namespace fc {
 
       void _wait( const microseconds& timeout_us );
       void _wait_until( const time_point& timeout_us );
-      void _enqueue_thread();
-      void _dequeue_thread();
       void _notify();
-      void _set_timeout();
       void _set_value(const void* v);
 
       void _on_complete( detail::completion_handler* c );
 
     private:
+      void _enqueue_thread();
+      void _dequeue_thread();
+
       friend class  thread;
       friend struct context;
       friend class  thread_d;
 
-      bool                        _ready;
+      std::atomic<bool>           _ready;
-      mutable spin_yield_lock     _spin_yield;
+      std::atomic<thread*>        _blocked_thread;
-      thread*                     _blocked_thread;
+      std::atomic<int32_t>        _blocked_fiber_count;
-      unsigned                    _blocked_fiber_count;
       time_point                  _timeout;
       fc::exception_ptr           _exceptp;
       bool                        _canceled;
@@ -103,7 +102,7 @@ namespace fc {
     private:
 #endif
       const char*                   _desc;
-      detail::completion_handler*   _compl;
+      std::atomic<detail::completion_handler*> _compl;
   };
 
   template<typename T = void> 

include/fc/thread/task.hpp

@@ -4,6 +4,8 @@
 #include <fc/fwd.hpp>
 #include <type_traits>
 
+#include <boost/atomic.hpp>
+
 namespace fc {
   struct context;
   class spin_lock;

src/crypto/elliptic_common.cpp

@@ -19,7 +19,7 @@
 namespace fc { namespace ecc {
 
     namespace detail {
-        typedef std::array<unsigned char,37> chr37;
+        typedef zero_initialized_array<unsigned char,37> chr37;
 
         fc::sha256 _left( const fc::sha512& v )
         {

src/crypto/elliptic_secp256k1.cpp

@@ -53,7 +53,7 @@ namespace fc { namespace ecc {
                 public_key_data _key;
         };
 
-        typedef std::array<unsigned char,37> chr37;
+        typedef zero_initialized_array<unsigned char,37> chr37;
         chr37 _derive_message( const public_key_data& key, int i );
         fc::sha256 _left( const fc::sha512& v );
         fc::sha256 _right( const fc::sha512& v );

src/thread/future.cpp

@@ -6,7 +6,6 @@
 
 #include <boost/assert.hpp>
 
-
 namespace fc {
 
   promise_base::promise_base( const char* desc )
@@ -22,6 +21,8 @@ namespace fc {
    _compl(nullptr)
   { }
 
+  promise_base::~promise_base() { }
+
   const char* promise_base::get_desc()const{
     return _desc; 
   }
@@ -34,16 +35,14 @@ namespace fc {
 #endif
     }
   bool promise_base::ready()const {
-    return _ready;
+    return _ready.load();
   }
   bool promise_base::error()const {
-    { synchronized(_spin_yield) 
+    return std::atomic_load( &_exceptp ) != nullptr;
-      return _exceptp != nullptr;
-    }
   }
 
   void promise_base::set_exception( const fc::exception_ptr& e ){
-    _exceptp = e;
+    std::atomic_store( &_exceptp, e );
     _set_value(nullptr);
   }
 
@@ -54,16 +53,21 @@ namespace fc {
        _wait_until( time_point::now() + timeout_us );
   }
   void promise_base::_wait_until( const time_point& timeout_us ){
-    { synchronized(_spin_yield) 
+    if( _ready.load() ) {
-      if( _ready ) {
+       fc::exception_ptr ex = std::atomic_load( &_exceptp );
-        if( _exceptp ) 
+       if( ex )
-          _exceptp->dynamic_rethrow_exception();
+          ex->dynamic_rethrow_exception();
-        return;
+       return;
-      }
+    }
-      _enqueue_thread();
+    _enqueue_thread();
+    // Need to check _ready again to avoid a race condition.
+    if( _ready.load() )
+    {
+       _dequeue_thread();
+       return _wait_until( timeout_us ); // this will simply return or throw _exceptp
     }
-    std::exception_ptr e;
 
+    std::exception_ptr e;
     //
     // Create shared_ptr to take ownership of this; i.e. this will
     // be deleted when p_this goes out of scope.  Consequently,
@@ -71,9 +75,7 @@ namespace fc {
     // before we're done reading/writing instance variables!
     // See https://github.com/cryptonomex/graphene/issues/597
     //
-
     ptr p_this = shared_from_this();
-
     try
     {
        //
@@ -94,61 +96,45 @@ namespace fc {
 
     if( e ) std::rethrow_exception(e);
 
-    if( _ready )
+    if( _ready.load() ) return _wait_until( timeout_us ); // this will simply return or throw _exceptp
-    {
+
-       if( _exceptp )
-         _exceptp->dynamic_rethrow_exception();
-       return;
-    }
     FC_THROW_EXCEPTION( timeout_exception, "" );
   }
   void promise_base::_enqueue_thread(){
-     ++_blocked_fiber_count;
+     _blocked_fiber_count.fetch_add( 1 );
+     thread* blocked_thread = _blocked_thread.load();
      // only one thread can wait on a promise at any given time
-     assert(!_blocked_thread ||
+     do
-            _blocked_thread == &thread::current());
+        assert( !blocked_thread || blocked_thread == &thread::current() );
-     _blocked_thread = &thread::current();
+     while( !_blocked_thread.compare_exchange_weak( blocked_thread, &thread::current() ) );
   }
   void promise_base::_dequeue_thread(){ 
-    synchronized(_spin_yield)
+    if( _blocked_fiber_count.fetch_add( -1 ) == 1 )
-    if (!--_blocked_fiber_count)
+       _blocked_thread.store( nullptr );
-      _blocked_thread = nullptr;
   }
   void promise_base::_notify(){
     // copy _blocked_thread into a local so that if the thread unblocks (e.g., 
     // because of a timeout) before we get a chance to notify it, we won't be
     // calling notify on a null pointer
-    thread* blocked_thread;
+    thread* blocked_thread = _blocked_thread.load();
-    { synchronized(_spin_yield)
-      blocked_thread = _blocked_thread;
-    }
     if( blocked_thread ) 
       blocked_thread->notify( shared_from_this() );
   }
-  promise_base::~promise_base() { }
+
-  void promise_base::_set_timeout(){
-    if( _ready ) 
-      return;
-    set_exception( std::make_shared<fc::timeout_exception>() );
-  }
   void promise_base::_set_value(const void* s){
- //   slog( "%p == %d", &_ready, int(_ready));
+     bool ready = false;
-//    BOOST_ASSERT( !_ready );
+     if( !_ready.compare_exchange_strong( ready, true ) ) //don't allow promise to be set more than once
-    { synchronized(_spin_yield) 
-      if (_ready) //don't allow promise to be set more than once
         return;
-      _ready = true;
+     _notify();
-    }
+     auto* hdl = _compl.load();
-    _notify();
+     if( nullptr != hdl )
-    if( nullptr != _compl ) {
+        hdl->on_complete( s, std::atomic_load( &_exceptp ) );
-      _compl->on_complete(s,_exceptp);
-    }
   }
+
   void promise_base::_on_complete( detail::completion_handler* c ) {
-    { synchronized(_spin_yield) 
+     auto* hdl = _compl.load();
-        delete _compl; 
+     while( !_compl.compare_exchange_weak( hdl, c ) );
-        _compl = c;
+     delete hdl;
-    }
   }
 }
 

src/thread/thread_d.hpp

@@ -1,12 +1,14 @@
 #include <fc/thread/thread.hpp>
+#include <fc/stacktrace.hpp>
 #include <fc/time.hpp>
 #include <boost/thread.hpp>
 #include "context.hpp"
 #include <boost/thread/condition_variable.hpp>
 #include <boost/thread.hpp>
 #include <boost/atomic.hpp>
+
+#include <sstream>
 #include <vector>
-//#include <fc/logger.hpp>
 
 namespace fc {
     struct sleep_priority_less {
@@ -390,7 +392,14 @@ namespace fc {
               /* NB: At least on Win64, this only catches a yield while in the body of 
                * a catch block; it fails to catch a yield while unwinding the stack, which 
                * is probably just as likely to cause crashes */
-              assert(std::current_exception() == std::exception_ptr());
+              if( std::current_exception() != std::exception_ptr() )
+              {
+                 std::stringstream stacktrace;
+                 print_stacktrace( stacktrace );
+                 elog( "Thread ${name} yielded in exception handler!\n${trace}",
+                       ("name",thread::current().name())("trace",stacktrace.str()) );
+                 assert( std::current_exception() == std::exception_ptr() );
+              }
 
               check_for_timeouts();
               if( !current ) 

tests/CMakeLists.txt

@@ -26,6 +26,7 @@ target_link_libraries( ecc_test fc )
 add_executable( all_tests all_tests.cpp
                           compress/compress.cpp
                           crypto/aes_test.cpp
+                          crypto/array_initialization_test.cpp
                           crypto/base_n_tests.cpp
                           crypto/bigint_test.cpp
                           crypto/blind.cpp

tests/crypto/array_initialization_test.cpp

@@ -0,0 +1,67 @@
+/*
+ * Copyright (c) 2019 BitShares Blockchain Foundation, and contributors.
+ *
+ * The MIT License
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include <boost/test/unit_test.hpp>
+
+#include <fc/crypto/elliptic.hpp>
+#include <fc/crypto/rand.hpp>
+
+#include <string.h>
+
+static void check_null_key()
+{
+   fc::ecc::public_key_data key1;
+   fc::ecc::public_key_data key2;
+   unsigned char zeroes[33];
+   static_assert( key1.size() == sizeof(zeroes), "Wrong array size!" );
+   memset( zeroes, 0, sizeof(zeroes) );
+   BOOST_CHECK( !memcmp( key1.data(), zeroes, sizeof(zeroes) ) );
+   BOOST_CHECK( !memcmp( key2.data(), zeroes, sizeof(zeroes) ) );
+
+   // now "pollute" the keys for the next invocation
+   key1 = fc::ecc::private_key::generate().get_public_key();
+   for( unsigned char c = 0; c < key2.size(); c++ )
+   {
+      key2[c] = c ^ 17;
+      zeroes[c] = c ^ 47;
+   }
+
+   // ...and use them to prevent the compiler from optimizing the pollution away.
+   wlog( "Key1: ${k}", ("k",fc::ecc::public_key::to_base58(key1)) );
+   wlog( "Key2: ${k}", ("k",fc::ecc::public_key::to_base58(key2)) );
+}
+
+BOOST_AUTO_TEST_SUITE(fc_crypto)
+
+BOOST_AUTO_TEST_CASE(array_init_test)
+{
+   check_null_key();
+   check_null_key();
+   {
+      char junk[128];
+      fc::rand_bytes( junk, sizeof(junk) );
+   }
+   check_null_key();
+}
+
+BOOST_AUTO_TEST_SUITE_END()