Skip to content

Commit

Permalink
Merge pull request #2179 from nfrmtkr/#2163-Race-condition-in-ACE_Fut…
Browse files Browse the repository at this point in the history 
…ure-double-checked-locking-pattern-code

Fixed a race condition caused by the double checked locking pattern
  • Loading branch information
@jwillemsen
jwillemsen authored Jan 15, 2024
2 parents 8c49e7b + 56d5db9 commit d571969
Show file tree
Hide file tree
Showing 7 changed files with 197 additions and 72 deletions.
14 changes: 6 additions & 8 deletions ACE/ace/Barrier.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,10 @@ ACE_Barrier::wait ()
ACE_TRACE ("ACE_Barrier::wait");
ACE_GUARD_RETURN (ACE_Thread_Mutex, ace_mon, this->lock_, -1);

ACE_Sub_Barrier *sbp =
this->sub_barrier_[this->current_generation_];
ACE_Sub_Barrier *sbp = this->sub_barrier_[this->current_generation_];

// Check for shutdown...
if (sbp == 0)
if (sbp == nullptr)
{
errno = ESHUTDOWN;
return -1;
Expand Down Expand Up @@ -127,19 +126,18 @@ ACE_Barrier::shutdown ()
ACE_TRACE ("ACE_Barrier::shutdown");
ACE_GUARD_RETURN (ACE_Thread_Mutex, ace_mon, this->lock_, -1);

ACE_Sub_Barrier *sbp =
this->sub_barrier_[this->current_generation_];
ACE_Sub_Barrier *sbp = this->sub_barrier_[this->current_generation_];

// Check for shutdown...
if (sbp == 0)
if (sbp == nullptr)
{
errno = ESHUTDOWN;
return -1;
}

// Flag the shutdown
this->sub_barrier_[0] = 0;
this->sub_barrier_[1] = 0;
this->sub_barrier_[0] = nullptr;
this->sub_barrier_[1] = nullptr;
// Tell all the threads waiting on the barrier to continue on their way.
sbp->running_threads_ = this->count_;
sbp->barrier_finished_.broadcast ();
Expand Down
64 changes: 19 additions & 45 deletions ACE/ace/Future.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,32 +19,12 @@ ACE_ALLOC_HOOK_DEFINE_Tc(ACE_Future_Observer)
ACE_ALLOC_HOOK_DEFINE_Tc(ACE_Future_Rep)
ACE_ALLOC_HOOK_DEFINE_Tc(ACE_Future)

template <class T>
ACE_Future_Holder<T>::ACE_Future_Holder ()
{
}

template <class T>
ACE_Future_Holder<T>::ACE_Future_Holder (const ACE_Future<T> &item)
: item_ (item)
{
}

template <class T>
ACE_Future_Holder<T>::~ACE_Future_Holder ()
{
}

template <class T>
ACE_Future_Observer<T>::ACE_Future_Observer ()
{
}

template <class T>
ACE_Future_Observer<T>::~ACE_Future_Observer ()
{
}

// Dump the state of an object.

template <class T> void
Expand Down Expand Up @@ -74,10 +54,10 @@ ACE_Future_Rep<T>::dump () const
template <class T> ACE_Future_Rep<T> *
ACE_Future_Rep<T>::internal_create ()
{
ACE_Future_Rep<T> *temp = 0;
ACE_Future_Rep<T> *temp {};
ACE_NEW_RETURN (temp,
ACE_Future_Rep<T> (),
0);
nullptr);
return temp;
}

Expand All @@ -95,17 +75,17 @@ ACE_Future_Rep<T>::create ()
template <class T> ACE_Future_Rep<T> *
ACE_Future_Rep<T>::attach (ACE_Future_Rep<T>*& rep)
{
ACE_ASSERT (rep != 0);
ACE_ASSERT (rep != nullptr);
// Use value_ready_mutex_ for both condition and ref count management
ACE_GUARD_RETURN (ACE_SYNCH_RECURSIVE_MUTEX, r_mon, rep->value_ready_mutex_, 0);
ACE_GUARD_RETURN (ACE_SYNCH_RECURSIVE_MUTEX, r_mon, rep->value_ready_mutex_, nullptr);
++rep->ref_count_;
return rep;
}

template <class T> void
ACE_Future_Rep<T>::detach (ACE_Future_Rep<T>*& rep)
{
ACE_ASSERT (rep != 0);
ACE_ASSERT (rep != nullptr);
// Use value_ready_mutex_ for both condition and ref count management
ACE_GUARD (ACE_SYNCH_RECURSIVE_MUTEX, r_mon, rep->value_ready_mutex_);

Expand All @@ -122,8 +102,8 @@ ACE_Future_Rep<T>::detach (ACE_Future_Rep<T>*& rep)
template <class T> void
ACE_Future_Rep<T>::assign (ACE_Future_Rep<T>*& rep, ACE_Future_Rep<T>* new_rep)
{
ACE_ASSERT (rep != 0);
ACE_ASSERT (new_rep != 0);
ACE_ASSERT (rep != nullptr);
ACE_ASSERT (new_rep != nullptr);
// Use value_ready_mutex_ for both condition and ref count management
ACE_GUARD (ACE_SYNCH_RECURSIVE_MUTEX, r_mon, rep->value_ready_mutex_);

Expand All @@ -143,9 +123,7 @@ ACE_Future_Rep<T>::assign (ACE_Future_Rep<T>*& rep, ACE_Future_Rep<T>* new_rep)

template <class T>
ACE_Future_Rep<T>::ACE_Future_Rep ()
: value_ (0),
ref_count_ (0),
value_ready_ (value_ready_mutex_)
: value_ready_ (value_ready_mutex_)
{
}

Expand All @@ -158,15 +136,15 @@ ACE_Future_Rep<T>::~ACE_Future_Rep ()
template <class T> int
ACE_Future_Rep<T>::ready () const
{
return this->value_ != 0;
return this->value_ != nullptr;
}

template <class T> int
ACE_Future_Rep<T>::set (const T &r,
ACE_Future<T> &caller)
{
// If the value is already produced, ignore it...
if (this->value_ == 0)
if (this->value_ == nullptr)
{
ACE_GUARD_RETURN (ACE_SYNCH_RECURSIVE_MUTEX,
ace_mon,
Expand All @@ -175,18 +153,15 @@ ACE_Future_Rep<T>::set (const T &r,
// Otherwise, create a new result value. Note the use of the
// Double-checked locking pattern to avoid multiple allocations.

if (this->value_ == 0) // Still no value, so proceed
if (this->value_ == nullptr) // Still no value, so proceed
{
ACE_NEW_RETURN (this->value_,
T (r),
-1);

// Remove and notify all subscribed observers.
typename OBSERVER_COLLECTION::iterator iterator =
this->observer_collection_.begin ();

typename OBSERVER_COLLECTION::iterator end =
this->observer_collection_.end ();
typename OBSERVER_COLLECTION::iterator iterator = this->observer_collection_.begin ();
typename OBSERVER_COLLECTION::iterator end = this->observer_collection_.end ();

while (iterator != end)
{
Expand All @@ -210,15 +185,15 @@ ACE_Future_Rep<T>::get (T &value,
ACE_Time_Value *tv) const
{
// If the value is already produced, return it.
if (this->value_ == 0)
if (this->value_ == nullptr)
{
ACE_GUARD_RETURN (ACE_SYNCH_RECURSIVE_MUTEX, ace_mon,
this->value_ready_mutex_,
-1);
// If the value is not yet defined we must block until the
// producer writes to it.

while (this->value_ == 0)
while (this->value_ == nullptr)
// Perform a timed wait.
if (this->value_ready_.wait (tv) == -1)
return -1;
Expand All @@ -242,7 +217,7 @@ ACE_Future_Rep<T>::attach (ACE_Future_Observer<T> *observer,
int result = 1;

// If the value is already produced, then notify observer
if (this->value_ == 0)
if (this->value_ == nullptr)
result = this->observer_collection_.insert (observer);
else
observer->update (caller);
Expand All @@ -264,7 +239,7 @@ template <class T>
ACE_Future_Rep<T>::operator T ()
{
// If the value is already produced, return it.
if (this->value_ == 0)
if (this->value_ == nullptr)
{
// Constructor of ace_mon acquires the mutex.
ACE_GUARD_RETURN (ACE_SYNCH_RECURSIVE_MUTEX, ace_mon, this->value_ready_mutex_, 0);
Expand All @@ -274,7 +249,7 @@ ACE_Future_Rep<T>::operator T ()

// Wait ``forever.''

while (this->value_ == 0)
while (this->value_ == nullptr)
if (this->value_ready_.wait () == -1)
// What to do in this case since we've got to indicate
// failure somehow? Exceptions would be nice, but they're
Expand Down Expand Up @@ -356,8 +331,7 @@ ACE_Future<T>::ready () const
}

template <class T> int
ACE_Future<T>::get (T &value,
ACE_Time_Value *tv) const
ACE_Future<T>::get (T &value, ACE_Time_Value *tv) const
{
// We return the ACE_Future_rep.
return this->future_rep_->get (value, tv);
Expand Down
15 changes: 7 additions & 8 deletions ACE/ace/Future.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include /**/ "ace/pre.h"

#include <atomic>
#include "ace/Unbounded_Set.h"
#include "ace/Strategies_T.h"

Expand Down Expand Up @@ -47,15 +48,13 @@ class ACE_Future_Holder
{
public:
ACE_Future_Holder (const ACE_Future<T> &future);
~ACE_Future_Holder ();
~ACE_Future_Holder () = default;
ACE_Future_Holder () = delete;

/// Declare the dynamic allocation hooks.
ACE_ALLOC_HOOK_DECLARE;

ACE_Future<T> item_;

protected:
ACE_Future_Holder ();
};

/**
Expand All @@ -73,7 +72,7 @@ class ACE_Future_Observer
{
public:
/// Destructor
virtual ~ACE_Future_Observer ();
virtual ~ACE_Future_Observer () = default;

/// Called by the ACE_Future in which we are subscribed to when
/// its value is written to.
Expand All @@ -84,7 +83,7 @@ class ACE_Future_Observer

protected:
/// Constructor
ACE_Future_Observer ();
ACE_Future_Observer () = default;
};

/**
Expand Down Expand Up @@ -197,10 +196,10 @@ class ACE_Future_Rep
int ready () const;

/// Pointer to the result.
T *value_;
std::atomic<T*> value_ {};

/// Reference count.
int ref_count_;
int ref_count_ {};

typedef ACE_Future_Observer<T> OBSERVER;

Expand Down
19 changes: 8 additions & 11 deletions ACE/ace/Future_Set.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,13 @@ ACE_Future_Set<T>::is_empty () const
template <class T> int
ACE_Future_Set<T>::insert (ACE_Future<T> &future)
{
FUTURE_HOLDER *future_holder;
FUTURE_HOLDER *future_holder {};
ACE_NEW_RETURN (future_holder,
FUTURE_HOLDER (future),
-1);

FUTURE_REP *future_rep = future.get_rep ();
int result = this->future_map_.bind (future_rep,
future_holder);
int const result = this->future_map_.bind (future_rep, future_holder);

// If a new map entry was created, then attach to the future,
// otherwise we were already attached to the future or some error
Expand All @@ -83,7 +82,7 @@ ACE_Future_Set<T>::insert (ACE_Future<T> &future)
template <class T> void
ACE_Future_Set<T>::update (const ACE_Future<T> &future)
{
ACE_Message_Block *mb = 0;
ACE_Message_Block *mb = nullptr;
FUTURE &local_future = const_cast<ACE_Future<T> &> (future);

ACE_NEW (mb,
Expand All @@ -100,12 +99,11 @@ ACE_Future_Set<T>::next_readable (ACE_Future<T> &future,
if (this->is_empty ())
return 0;

ACE_Message_Block *mb = 0;
FUTURE_REP *future_rep = 0;
ACE_Message_Block *mb = nullptr;
FUTURE_REP *future_rep = nullptr;

// Wait for a "readable future" signal from the message queue.
if (this->future_notification_queue_->dequeue_head (mb,
tv) != -1)
if (this->future_notification_queue_->dequeue_head (mb, tv) != -1)
{
// Extract future rep from the message block.
future_rep = reinterpret_cast<FUTURE_REP *> (mb->base ());
Expand All @@ -117,9 +115,8 @@ ACE_Future_Set<T>::next_readable (ACE_Future<T> &future,
return 0;

// Remove the hash map entry with the specified future rep from our map.
FUTURE_HOLDER *future_holder = 0;
if (this->future_map_.find (future_rep,
future_holder) != -1)
FUTURE_HOLDER *future_holder = nullptr;
if (this->future_map_.find (future_rep, future_holder) != -1)
{
future = future_holder->item_;
this->future_map_.unbind (future_rep);
Expand Down
Loading

0 comments on commit d571969

Please sign in to comment.