detail::win32::ReleaseSemaphore may be called with count_to_release equal to 0

[Jesper Storm Bache <jsbache@…>]

ReleaseSemaphore is documented by Microsoft as: "... lReleaseCount [in] The amount by which the semaphore object's current count is to be increased. The value must be greater than zero" ​http://msdn.microsoft.com/en-us/library/windows/desktop/ms685071(v=vs.85).aspx

When I run boundschecker on boost threads, I get a error because count_to_release is 0 in the following:

void release(unsigned count_to_release) {

notified=true; detail::win32::ReleaseSemaphore(semaphore,count_to_release,0);

}

(this is line 71 in boost_1_51_0/boost/thread/win32/condition_variable.hpp)

[viboes]

Hi,

I don't know how the tool you are using has deduced such an issue.

The count_to_release parameter seems to be always > 0.

Please let me know if I'm missing something.

Either it is called with 1

                    for(generation_list::iterator it=generations.begin(),
                            end=generations.end();
                        it!=end;++it)
                    {
                        (*it)->release(1);
                    }

or with the contents of waiters.

            void release_waiters()
            {
                release(detail::interlocked_read_acquire(&waiters));
            }

This variable is initialized as follows

                waiters(1),notified(false),references(0)

and updated by

            void add_waiter()
            {
                BOOST_INTERLOCKED_INCREMENT(&waiters);
            }

            void remove_waiter()
            {
                BOOST_INTERLOCKED_DECREMENT(&waiters);
            }

But remove_waiter is always called after add_waiter

            entry_ptr get_wait_entry()
            {
                boost::lock_guard<boost::mutex> internal_lock(internal_mutex);

                if(!wake_sem)
                {
                    wake_sem=detail::win32::create_anonymous_semaphore(0,LONG_MAX);
                    BOOST_ASSERT(wake_sem);
                }

                detail::interlocked_write_release(&total_count,total_count+1);
                if(generations.empty() || generations.back()->is_notified())
                {
                    entry_ptr new_entry(new list_entry(wake_sem));
                    generations.push_back(new_entry);
                    return new_entry;
                }
                else
                {
                    generations.back()->add_waiter();
                    return generations.back();
                }
            }

            struct entry_manager
            {
                entry_ptr const entry;

                BOOST_THREAD_NO_COPYABLE(entry_manager)
                entry_manager(entry_ptr const& entry_):
                    entry(entry_)
                {}

                ~entry_manager()
                {
                    entry->remove_waiter();
                }

                list_entry* operator->()
                {
                    return entry.get();
                }
            };


        protected:
            template<typename lock_type>
            bool do_wait(lock_type& lock,timeout abs_time)
            {
                relocker<lock_type> locker(lock);

                entry_manager entry(get_wait_entry());

                locker.unlock();

                bool woken=false;
                while(!woken)
                {
                    if(!entry->wait(abs_time))
                    {
                        return false;
                    }

                    woken=entry->woken();
                }
                return woken;
            }

Any deep explanation of on which circumstances this issue appear is welcome.

[jsbache@…]

I apologize for the terseness. Here are a few detais. When I notice a zero count, the stack is:

c4test.exe!boost::detail::basic_cv_list_entry::release(unsigned int count_to_release=0) Line 68 c4test.exe!boost::detail::basic_cv_list_entry::release_waiters() Line 73

c4test.exe!boost::detail::basic_condition_variable::notify_all() Line 285 + 0x1a bytes

My conditional breakpoint (condition: "count_to_release == 0) is on the following line (before we modify notified)

   void release(unsigned count_to_release)
   {
   --break--       notified=true;

When I hit this, code I have the following state of the local variables. Note that notified = true at this point in time,

it	{px=0x0000000001dde7b0 }
	[ptr]	0x0000000001dd75b8 {px=0x0000000001dde7b0 }
	px	0x0000000001dde7b0
	semaphore	{handle_to_manage=0x0000000000000100 }
	wake_sem	{handle_to_manage=0x0000000000000184 }
	waiters		0	
	notified	true
	references	1

this	0x00000000003dee28
	internal_mutex	{...}	boost::mutex
	total_count	0	long
	active_generation_count	0	unsigned int
	generations	[2]({px=0x0000000001dde630 },{px=0x0000000001dde7b0 })
	wake_sem	{handle_to_manage=0x0000000000000198 }

[viboes]

Thanks for the details. Please, could you attach the example? Could you see when waiters become 0?

[viboes]

Hi again,

I think I have an hint. Could you check if defining ~entry_manager as follows avoid the problem?

                ~entry_manager()
                {
                  if(! entry->is_notified())
                  {
                    entry->remove_waiter();
                  }
                }

[jsbache@…]

The proposed solution does solve the issue, meaning that with this change I no longer observe a call to basic_cv_list_entry::release with count_to_release equal to 0.

[viboes]

Replying to jsbache@…:

The proposed solution does solve the issue, meaning that with this change I no longer observe a call to basic_cv_list_entry::release with count_to_release equal to 0.

Thanks for checking. I will apply the modification and run the test on windows as soon as i have access to a windows machine.

[viboes]

Committed revision [81024].

[viboes]

Hi,

it seems that the patch introduce a severe restriction, see #7657. I will rollback the change and take the time to analyze your issue.

[viboes]

Hi,

I don't reach to understand how your issue can arrive.

Could you add a small example reproduce the issue?

[viboes]

I suspect that the following sequence is possible.

1. thread A call wait and it stands in [1] below. The value for waiters is 1.
2. thread B notify_all and after wake the waiters in [2] the scheduler choose the ready thread A.
3. thread A ends the call to do_wait and the entry_manager destructor calls. The value for waiters is 0. in [3] the scheduler choose the ready thread B.
4. thread B calls release_waiters() in [4] which calls to release(0).

Current Code

            template<typename lock_type>
            bool do_wait(lock_type& lock,timeout abs_time)
            {
                relocker<lock_type> locker(lock);

                entry_manager entry(get_wait_entry());

                locker.unlock();

                bool woken=false;
                while(!woken)
                {
                    if(!entry->wait(abs_time)) // [1]
                    {
                        return false;
                    }

                    woken=entry->woken();
                }
                return woken;
            } // [3]

            void notify_all() BOOST_NOEXCEPT
            {
                if(detail::interlocked_read_acquire(&total_count))
                {
                    boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
                    if(!total_count)
                    {
                        return;
                    }
                    wake_waiters(total_count); // [2]
                    for(generation_list::iterator it=generations.begin(),
                            end=generations.end();
                        it!=end;++it)
                    {
                        (*it)->release_waiters(); [4]
                    }
                    generations.clear();
                    wake_sem=detail::win32::handle(0);
                }
            }

I suspect that the call to entry->remove_waiter() inside entry_manager() must be protected

Could some one check if the following patch for boost/thread/win32/condition_variable.hpp works

===================================================================
--- condition_variable.hpp	(revision 85663)
+++ condition_variable.hpp	(working copy)
@@ -191,18 +191,17 @@
             struct entry_manager
             {
                 entry_ptr const entry;
+                boost::mutex& internal_mutex;
 
                 BOOST_THREAD_NO_COPYABLE(entry_manager)
-                entry_manager(entry_ptr const& entry_):
-                    entry(entry_)
+                entry_manager(entry_ptr const& entry_, boost::mutex& mutex_):
+                    entry(entry_), internal_mutex(mutex_)
                 {}
 
                 ~entry_manager()
                 {
-                  //if(! entry->is_notified()) // several regression #7657
-                  {
+                    boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
                     entry->remove_waiter();
-                  }
                 }
 
                 list_entry* operator->()
@@ -218,7 +217,7 @@
             {
                 relocker<lock_type> locker(lock);
 
-                entry_manager entry(get_wait_entry());
+                entry_manager entry(get_wait_entry(), internal_mutex);
 
                 locker.unlock();
 

[viboes]

Committed revision [85733].

[viboes]

Committed revision [85815].