Opened 9 years ago

Closed 8 years ago

#9787 closed Bugs (fixed)

[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

Reported by: Bryan Laird <bryan_laird@…> Owned by: viboes
Milestone: Boost 1.57.0 Component: thread
Version: Boost 1.53.0 Severity: Problem
Keywords: WaitForSingleObject timed_mutex try_lock_until wait_until Cc:

Description

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.

Attachments (1)

test_9787.cpp (989 bytes ) - added by Niall Douglas 8 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 by viboes, 9 years ago

Is this a duplicate of #9618?

Last edited 8 years ago by viboes (previous) (diff)

comment:2 by viboes, 9 years ago

Owner: changed from Anthony Williams to viboes
Status: newassigned

comment:3 by Bryan Laird <bryan_laird@…>, 9 years ago

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.

comment:4 by viboes, 9 years ago

Summary: 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[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

comment:5 by viboes, 9 years ago

Have you tried it?

in reply to:  5 comment:6 by Bryan Laird <bryan_laird@…>, 8 years ago

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.

comment:7 by Bryan Laird <bryan_laird@…>, 8 years ago

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.

comment:8 by viboes, 8 years ago

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)
                       {

comment:9 by Niall Douglas, 8 years ago

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

by Niall Douglas, 8 years ago

Attachment: test_9787.cpp added

comment:10 by Niall Douglas, 8 years ago

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

in reply to:  10 comment:11 by viboes, 8 years ago

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?

comment:12 by Niall Douglas, 8 years ago

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

comment:13 by viboes, 8 years ago

Briand please, could you try the patch?

in reply to:  12 comment:14 by viboes, 8 years ago

Replying to ned14:

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

Thanks Niall.

comment:15 by Bryan Laird <bryan_laird@…>, 8 years ago

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

comment:16 by viboes, 8 years ago

Milestone: To Be DeterminedBoost 1.57.0

comment:17 by Bryan Laird <bryan_laird@…>, 8 years ago

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.

comment:18 by viboes, 8 years ago

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.

comment:19 by viboes, 8 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.