[windows] Small duration value passed down to basic_timed_mutex::try_lock_until and condition_variable::wait_until can cause infinite or near infinite wait for win32

[Bryan Laird <bryan_laird@…>]

Creating a unique_lock using a timed_mutex which is passed a small chrono::milliseconds duration value (10 milliseconds in our case) can cause a very large value to be passed to WaitForSingleObject for the dwMilliseconds parameter. The code path creates an initial chrono::time_point where the duration value is added to the current time. Once it gets to try_lock_until it calculates a new rel_time duration to feed to WaitForSingleObject. If the small duration has elapsed due to thread switching then the subtraction results in a very large positive value to be passed to WaitForSingleObject. Similarly, if we calculate our own boost::chrono::system_clock::time_point and pass it to boost::condition_variable::wait_until and the time_point has passed then the subtraction to compute a relative time value can result in a very large positive value. Another possibility is if the caller inadvertently passes an elapsed time_point to condition_variable::wait_until.

[viboes]

Is this a duplicate of #9618?

[Bryan Laird <bryan_laird@…>]

This ticket does not describe an issue with thread::try_join_for or thread::try_join_until so it is not a duplicate of #9618. However what's described in comment 9 of that ticket sounds similar in nature. The patch in comment 10 of that ticket would not fix this problem.

[viboes]

Have you tried it?

[Bryan Laird <bryan_laird@…>]

No because we are not calling try_join_for and basic_timed_mutex::try_lock_until and condition_variable::wait_until don't call try_join_for.

[Bryan Laird <bryan_laird@…>]

It appears that this problem is fixed for the condition_variable::wait_until method in version 1.55.

The problem still exists for basic_timed_mutex::try_lock_until in 1.55.

[viboes]

Could someone try this patch

git diff  include/boost/thread/win32/basic_timed_mutex.hpp
diff --git a/include/boost/thread/win32/basic_timed_mutex.hpp b/include/boost/thread/win32/basic_timed_mutex.hpp
index b55affd..d20c658 100644
--- a/include/boost/thread/win32/basic_timed_mutex.hpp
+++ b/include/boost/thread/win32/basic_timed_mutex.hpp
@@ -203,7 +203,12 @@ namespace boost
 
                   do
                   {
-                      chrono::milliseconds rel_time= chrono::ceil<chrono::milliseconds>(tp-chrono::system_clock::now());
+                      chrono::time_point<chrono::system_clock, chrono::system_clock::duration> now = chrono::system_clock::now();
+                      if (tp<=now) {
+                        BOOST_INTERLOCKED_DECREMENT(&active_count);
+                        return false;
+                      }
+                      chrono::milliseconds rel_time= chrono::ceil<chrono::milliseconds>(tp-now);
 
                       if(win32::WaitForSingleObjectEx(sem,static_cast<unsigned long>(rel_time.count()),0)!=0)
                       {

[Niall Douglas]

I have not been able to replicate this issue with Boost 1.56 release. See the attached test case - it tests both of the issues reported by the OP and soak tests the timed_mutex for thirty seconds across 128 threads. Runs flawlessly without any fixes.

OP - please supply a version of the attached test case test_9787 which demonstrates the problem you report. Else we shall close this issue as invalid.

Niall

[Niall Douglas]

FYI I tested on Win8.1 x86 using VS2013 Update 1.

[viboes]

Replying to ned14:

FYI I tested on Win8.1 x86 using VS2013 Update 1.

Thanks Niall.

Even if the issue is not reproducible in your configuration, could you try the patch to see if there is no regressions on the Thread tests?

[Niall Douglas]

With the patch applied, all unit tests pass including test_9787 on Win8.1 x64 VS2013 Update 1.

[viboes]

Briand please, could you try the patch?

[viboes]

Replying to ned14:

With the patch applied, all unit tests pass including test_9787 on Win8.1 x64 VS2013 Update 1.

Thanks Niall.

[Bryan Laird <bryan_laird@…>]

I'm not sure if "Briand" is me, but I will test this the week of September 22nd.

[Bryan Laird <bryan_laird@…>]

The fix looks good.

I would prefer to call WaitForSingleObjectEx() with a 0ms timeout in the case of tp<=now but that's up to you.

[viboes]

Thanks for trying it.

I will commit this patch. If you have any patch that can improve the current code, please post it here on another ticket. As I said, I need some help on the windows platform.