From: khizmax Date: Wed, 16 Mar 2016 21:08:58 +0000 (+0300) Subject: * Fixed serious bug in MichaelSet::emplace() function X-Git-Tag: v2.2.0~345 X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;h=70cba4469ee675d00bfa91c10879753296e819d5;p=libcds.git * Fixed serious bug in MichaelSet::emplace() function New node was created twice from the arguments by move semantics. However, move semantics may change internal state of the argument. This can lead to an incorrect element in the set and even to an incorrect key that breaks the set logic. --- diff --git a/cds/container/michael_list_nogc.h b/cds/container/michael_list_nogc.h index c6bd1fd1..b7fbbf6e 100644 --- a/cds/container/michael_list_nogc.h +++ b/cds/container/michael_list_nogc.h @@ -426,6 +426,16 @@ namespace cds { namespace container { protected: //@cond + static value_type& node_to_value( node_type& n ) + { + return n.m_Value; + } + + iterator insert_node( node_type * pNode ) + { + return node_to_iterator( insert_node_at( head(), pNode )); + } + node_type * insert_node_at( head_type& refHead, node_type * pNode ) { assert( pNode != nullptr ); diff --git a/cds/container/michael_list_rcu.h b/cds/container/michael_list_rcu.h index d4510a4e..3a9ba738 100644 --- a/cds/container/michael_list_rcu.h +++ b/cds/container/michael_list_rcu.h @@ -191,7 +191,7 @@ namespace cds { namespace container { /// Result of \p get(), \p get_with() functions - pointer to the node found typedef cds::urcu::raw_ptr_adaptor< value_type, typename base_class::raw_ptr, raw_ptr_converter > raw_ptr; - private: + protected: //@cond static value_type& node_to_value( node_type& n ) { @@ -201,10 +201,7 @@ namespace cds { namespace container { { return n.m_Value; } - //@endcond - protected: - //@cond template static node_type * alloc_node( Q const& v ) { @@ -788,6 +785,11 @@ namespace cds { namespace container { protected: //@cond + bool insert_node( node_type * pNode ) + { + return insert_node_at( head(), pNode ); + } + bool insert_node_at( head_type& refHead, node_type * pNode ) { assert( pNode ); diff --git a/cds/container/michael_set_nogc.h b/cds/container/michael_set_nogc.h index c4ae09b8..b3e7db4b 100644 --- a/cds/container/michael_set_nogc.h +++ b/cds/container/michael_set_nogc.h @@ -70,19 +70,31 @@ namespace cds { namespace container { typedef typename cds::opt::v::hash_selector< typename traits::hash >::type hash; typedef typename traits::item_counter item_counter; ///< Item counter type - /// Bucket table allocator - typedef cds::details::Allocator< bucket_type, typename traits::allocator > bucket_table_allocator; - protected: //@cond + class internal_bucket_type: public bucket_type + { + typedef bucket_type base_class; + public: + using base_class::node_type; + using base_class::alloc_node; + using base_class::insert_node; + using base_class::node_to_value; + }; + + /// Bucket table allocator + typedef cds::details::Allocator< internal_bucket_type, typename traits::allocator > bucket_table_allocator; + typedef typename bucket_type::iterator bucket_iterator; typedef typename bucket_type::const_iterator bucket_const_iterator; //@endcond protected: + //@cond item_counter m_ItemCounter; ///< Item counter hash m_HashFunctor; ///< Hash functor - bucket_type * m_Buckets; ///< bucket table + internal_bucket_type * m_Buckets; ///< bucket table + //@endcond private: //@cond @@ -100,7 +112,7 @@ namespace cds { namespace container { /// Returns the bucket (ordered list) for \p key template - bucket_type& bucket( const Q& key ) + internal_bucket_type& bucket( const Q& key ) { return m_Buckets[ hash_value( key ) ]; } @@ -200,11 +212,11 @@ namespace cds { namespace container { //@cond const_iterator get_const_begin() const { - return const_iterator( const_cast(m_Buckets[0]).begin(), m_Buckets, m_Buckets + bucket_count() ); + return const_iterator( const_cast(m_Buckets[0]).begin(), m_Buckets, m_Buckets + bucket_count() ); } const_iterator get_const_end() const { - return const_iterator( const_cast(m_Buckets[bucket_count() - 1]).end(), m_Buckets + bucket_count() - 1, m_Buckets + bucket_count() ); + return const_iterator( const_cast(m_Buckets[bucket_count() - 1]).end(), m_Buckets + bucket_count() - 1, m_Buckets + bucket_count() ); } //@endcond @@ -250,7 +262,7 @@ namespace cds { namespace container { template iterator insert( const Q& val ) { - bucket_type& refBucket = bucket( val ); + internal_bucket_type& refBucket = bucket( val ); bucket_iterator it = refBucket.insert( val ); if ( it != refBucket.end() ) { @@ -268,9 +280,9 @@ namespace cds { namespace container { template iterator emplace( Args&&... args ) { - bucket_type& refBucket = bucket( value_type(std::forward(args)...)); - bucket_iterator it = refBucket.emplace( std::forward(args)... ); - + typename internal_bucket_type::node_type * pNode = internal_bucket_type::alloc_node( std::forward( args )... ); + internal_bucket_type& refBucket = bucket( internal_bucket_type::node_to_value( *pNode )); + bucket_iterator it = refBucket.insert_node( pNode ); if ( it != refBucket.end() ) { ++m_ItemCounter; return iterator( it, &refBucket, m_Buckets + bucket_count() ); @@ -297,7 +309,7 @@ namespace cds { namespace container { template std::pair update( Q const& val, bool bAllowInsert = true ) { - bucket_type& refBucket = bucket( val ); + internal_bucket_type& refBucket = bucket( val ); std::pair ret = refBucket.update( val, bAllowInsert ); if ( ret.first != refBucket.end() ) { @@ -328,7 +340,7 @@ namespace cds { namespace container { template iterator contains( Q const& key ) { - bucket_type& refBucket = bucket( key ); + internal_bucket_type& refBucket = bucket( key ); bucket_iterator it = refBucket.contains( key ); if ( it != refBucket.end() ) return iterator( it, &refBucket, m_Buckets + bucket_count() ); @@ -353,7 +365,7 @@ namespace cds { namespace container { template iterator contains( Q const& key, Less pred ) { - bucket_type& refBucket = bucket( key ); + internal_bucket_type& refBucket = bucket( key ); bucket_iterator it = refBucket.contains( key, pred ); if ( it != refBucket.end() ) return iterator( it, &refBucket, m_Buckets + bucket_count() ); diff --git a/cds/container/michael_set_rcu.h b/cds/container/michael_set_rcu.h index 0ea938b5..5afd9a57 100644 --- a/cds/container/michael_set_rcu.h +++ b/cds/container/michael_set_rcu.h @@ -146,9 +146,6 @@ namespace cds { namespace container { typedef typename cds::opt::v::hash_selector< typename traits::hash >::type hash; typedef typename traits::item_counter item_counter; ///< Item counter type - /// Bucket table allocator - typedef cds::details::Allocator< bucket_type, typename traits::allocator > bucket_table_allocator; - typedef typename bucket_type::rcu_lock rcu_lock; ///< RCU scoped lock typedef typename bucket_type::exempt_ptr exempt_ptr; ///< pointer to extracted node typedef typename bucket_type::raw_ptr raw_ptr; ///< Return type of \p get() member function and its derivatives @@ -156,9 +153,26 @@ namespace cds { namespace container { static CDS_CONSTEXPR const bool c_bExtractLockExternal = bucket_type::c_bExtractLockExternal; protected: - item_counter m_ItemCounter; ///< Item counter - hash m_HashFunctor; ///< Hash functor - bucket_type * m_Buckets; ///< bucket table + //@cond + class internal_bucket_type: public bucket_type + { + typedef bucket_type base_class; + public: + using base_class::node_type; + using base_class::alloc_node; + using base_class::insert_node; + using base_class::node_to_value; + }; + + /// Bucket table allocator + typedef cds::details::Allocator< internal_bucket_type, typename traits::allocator > bucket_table_allocator; + + //@endcond + + protected: + item_counter m_ItemCounter; ///< Item counter + hash m_HashFunctor; ///< Hash functor + internal_bucket_type * m_Buckets; ///< bucket table private: //@cond @@ -176,12 +190,12 @@ namespace cds { namespace container { /// Returns the bucket (ordered list) for \p key template - bucket_type& bucket( Q const& key ) + internal_bucket_type& bucket( Q const& key ) { return m_Buckets[ hash_value( key ) ]; } template - bucket_type const& bucket( Q const& key ) const + internal_bucket_type const& bucket( Q const& key ) const { return m_Buckets[ hash_value( key ) ]; } @@ -280,11 +294,11 @@ namespace cds { namespace container { //@cond const_iterator get_const_begin() const { - return const_iterator( const_cast(m_Buckets[0]).begin(), m_Buckets, m_Buckets + bucket_count() ); + return const_iterator( const_cast(m_Buckets[0]).begin(), m_Buckets, m_Buckets + bucket_count() ); } const_iterator get_const_end() const { - return const_iterator( const_cast(m_Buckets[bucket_count() - 1]).end(), m_Buckets + bucket_count() - 1, m_Buckets + bucket_count() ); + return const_iterator( const_cast(m_Buckets[bucket_count() - 1]).end(), m_Buckets + bucket_count() - 1, m_Buckets + bucket_count() ); } //@endcond @@ -306,7 +320,6 @@ namespace cds { namespace container { // GC and OrderedList::gc must be the same static_assert( std::is_same::value, "GC and OrderedList::gc must be the same"); - // atomicity::empty_item_counter is not allowed as a item counter static_assert( !std::is_same::value, "atomicity::empty_item_counter is not allowed as a item counter"); @@ -455,7 +468,8 @@ namespace cds { namespace container { template bool emplace( Args&&... args ) { - bool bRet = bucket( value_type(std::forward(args)...) ).emplace( std::forward(args)... ); + typename internal_bucket_type::node_type * pNode = internal_bucket_type::alloc_node( std::forward( args )... ); + bool bRet = bucket( internal_bucket_type::node_to_value( *pNode ) ).insert_node( pNode ); if ( bRet ) ++m_ItemCounter; return bRet;