Modify

Opened 4 years ago

Closed 3 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 3 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 4 years ago by viboes

Is this a duplicate of #9618?

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

comment:2 Changed 4 years ago by viboes

Owner: changed from Anthony Williams to viboes
Status: newassigned

comment:3 Changed 4 years ago by 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.

comment:4 Changed 3 years ago by viboes

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 Changed 3 years ago by viboes

Have you tried it?

comment:6 in reply to:  5 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by 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.

comment:8 Changed 3 years ago by 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)
                       {

comment:9 Changed 3 years ago by 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

Changed 3 years ago by Niall Douglas

Attachment: test_9787.cpp added

comment:10 Changed 3 years ago by Niall Douglas

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

comment:11 in reply to:  10 Changed 3 years ago by 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?

comment:12 Changed 3 years ago by Niall Douglas

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

comment:13 Changed 3 years ago by viboes

Briand please, could you try the patch?

comment:14 in reply to:  12 Changed 3 years ago by viboes

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 Changed 3 years ago by Bryan Laird <bryan_laird@…>

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

comment:16 Changed 3 years ago by viboes

Milestone: To Be DeterminedBoost 1.57.0

comment:17 Changed 3 years ago by 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.

comment:18 Changed 3 years ago by 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.

comment:19 Changed 3 years ago by viboes

Resolution: fixed
Status: assignedclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain viboes.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.