Fix condvar.wait(distant future) return immediately on OSX #42604

stepancheg · 2017-06-12T16:01:37Z

Fixes issue #37440: pthread_cond_timedwait on macOS Sierra seems
to overflow ts_sec parameter and returns immediately. To work
around this problem patch rounds timeout down to year 3000.

Patch also fixes overflow when converting u64 to time_t.

nagisa · 2017-06-12T19:25:00Z

Is year 3000 special in that sense that this is a maximum OS X will support?

stepancheg · 2017-06-12T20:01:01Z

@nagisa OSX pthread_cond_timedwait breaks near year 3170843 on my notebook. But I'm not sure about behavior on other versions of OSX or iPhone, so I decided it is safer to round down to year 3000. I hope in a couple of centuries Apple will fix this issue, and this rounding down could be removed from rust std source.

stepancheg · 2017-06-12T20:51:17Z

r? @alexcrichton

alexcrichton

Do you have an evidence of where the overflow is? The apparent source code for pthread_cond_timedwait shells out to _pthread_cond_wait which doesn't look like it has any overflows?

Using dtruss indicates that psynch_cvwait is failing with error 316. Headers don't seem to indicate what that is and related StackOverflow questions seem to also be similarly confused.

It seems that we don't know why this is failing and we're just blindly applying a workaround?

alexcrichton · 2017-06-13T03:57:14Z

src/libstd/sync/condvar.rs

If we can't have this assert, why have the Arc?

So next person going to patch this part of the library could execute test with this line uncommented. I believe that prevous comment "spurious wakeups mean this isn't necessarily true" several lines above kept in the source for the same reason.

Can this be removed in that case? It's surely easier to test this outside the standard library than inside, so this seems like it's just an extra diff for not a lot of value.

stepancheg · 2017-06-13T19:40:50Z

Do you have an evidence of where the overflow is? The apparent source code for pthread_cond_timedwait shells out to _pthread_cond_wait which doesn't look like it has any overflows?

I believe, overflow is somewhere inside kernel. I couldn't found sources of latest kernel, but there's a source of 10.7.5.

It has code like this:

nanoseconds_to_absolutetime((uint64_t)ts.tv_sec * NSEC_PER_SEC + ts.tv_nsec,  &abstime );

which does not explain this particular issue, but shows that authors of xnu don't care much about integer overflows.

Using dtruss indicates that psynch_cvwait is failing with error 316.

Or maybe it is not overflow, but an indication of parameter error. Whatever.

It seems that we don't know why this is failing and we're just blindly applying a workaround?

That's fine with me.

BTW, now I think that it would probably be slightly better to wait with explicitly infinite timeout if duration is more than 1000 years, instead of rounding down to year 3000, but this version is OK too.

stepancheg · 2017-06-13T19:58:17Z

BTW, now I think that it would probably be slightly better to wait with explicitly infinite timeout if duration is more than 1000 years, instead of rounding down to year 3000, but this version is OK too.

No, rounding down is legal, because cond.wait is allowed to wake up spuriously.

stepancheg · 2017-06-13T20:03:11Z

Going to update patch now to round duration down to 1000 years instead of rounding down deadline to year 3000.

stepancheg · 2017-06-13T20:17:11Z

Done.

alexcrichton · 2017-06-13T21:37:30Z

src/libstd/sys/unix/condvar.rs

This should no longer be necessary due to the clamp above, right?

Yes, if time_t is more than 32 bit (as in all modern OSes), this cast is not needed.

However, I'd keep this cast, because

it is not performance critical

makes code easier to read (reader does not need to check that there's no overflow)

makes OSX/Linux versions more like to each other

alexcrichton · 2017-06-13T21:37:58Z

src/libstd/sys/unix/condvar.rs

This is a pretty long comment to work with, could the program be extracted to something like a gist? Could the example program be C and not Rust?

Rewritten in C and moved to gist.

alexcrichton · 2017-06-13T21:39:23Z

src/libstd/sys/unix/condvar.rs

Are we 100% sure that this is an overflow somewhere? The fact that the kernel returns an error seems to indicate that this comment is not correct. I think we can basically just say that OSX is buggy with super long durations, so we clamp which is also allowable per the API of wait_timeout because of spurious wakeups.

Updated comment to use diffent words.

alexcrichton · 2017-06-13T21:39:56Z

src/libstd/sync/condvar.rs

Can this be removed in that case? It's surely easier to test this outside the standard library than inside, so this seems like it's just an extra diff for not a lot of value.

Fixes issue rust-lang#37440: `pthread_cond_timedwait` on macOS Sierra seems to overflow `ts_sec` parameter and returns immediately. To work around this problem patch rounds timeout down to approximately 1000 years. Patch also fixes overflow when converting `u64` to `time_t`.

stepancheg · 2017-06-15T21:53:52Z

Can this be removed in that case? It's surely easier to test this outside the standard library than inside, so this seems like it's just an extra diff for not a lot of value.

I've rewritten tests with loop until wake up is no spurious.

alexcrichton · 2017-06-16T14:42:17Z

@bors: r+

bors · 2017-06-16T14:42:18Z

📌 Commit 0c26b59 has been approved by alexcrichton

bors · 2017-06-17T00:05:56Z

⌛ Testing commit 0c26b59 with merge 1169a1f...

Fix condvar.wait(distant future) return immediately on OSX Fixes issue #37440: `pthread_cond_timedwait` on macOS Sierra seems to overflow `ts_sec` parameter and returns immediately. To work around this problem patch rounds timeout down to year 3000. Patch also fixes overflow when converting `u64` to `time_t`.

bors · 2017-06-17T02:21:33Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 1169a1f to master...

…raheemdev std: improve handling of timed condition variable waits on macOS Fixes rust-lang#37440 (for good). This fixes two issues with `Condvar::wait_timeout` on macOS: Apple's implementation of `pthread_cond_timedwait` internally converts the absolute timeout to a relative one, measured in nanoseconds, but fails to consider overflow when doing so. This results in `wait_timeout` returning much earlier than anticipated when passed a duration that is slightly longer than `u64::MAX` nanoseconds (around 584 years). The existing clamping introduced by rust-lang#42604 to address rust-lang#37440 unfortunately used a maximum duration of 1000 years and thus still runs into the bug when run on older macOS versions (or with `PTHREAD_MUTEX_USE_ULOCK` set to a value other than "1"). See rust-lang#37440 (comment) for context. Reducing the maximum duration alone however would not be enough to make the implementation completely correct. As macOS does not support `pthread_condattr_setclock`, the deadline passed to `pthread_cond_timedwait` is measured against the wall-time clock. `std` currently calculates the deadline by retrieving the current time and adding the duration to that, only for macOS to convert the deadline back to a relative duration by [retrieving the current time itself](https://github.com/apple-oss-distributions/libpthread/blob/1ebf56b3a702df53213c2996e5e128a535d2577e/src/pthread_cond.c#L802-L819) (this conversion is performed before the aforementioned problematic one). Thus, if the wall-time clock is adjusted between the `std` lookup and the system lookup, the relative duration could have changed, possibly even to a value larger than $2^{64}\ \textrm{ns}$. Luckily however, macOS supports the non-standard, tongue-twisting `pthread_cond_timedwait_relative_np` function which avoids the wall-clock-time roundtrip by taking a relative timeout. Even apart from that, this function is perfectly suited for `std`'s purposes: it is public (albeit badly-documented) API, [available since macOS 10.4](https://github.com/apple-oss-distributions/libpthread/blob/1ebf56b3a702df53213c2996e5e128a535d2577e/include/pthread/pthread.h#L555-L559) (that's way below our minimum of 10.12) and completely resilient against wall-time changes as all timeouts are [measured against the monotonic clock](https://github.com/apple-oss-distributions/xnu/blob/e3723e1f17661b24996789d8afc084c0c3303b26/bsd/kern/sys_ulock.c#L741) inside the kernel. Thus, this PR switches `Condvar::wait_timeout` to `pthread_cond_timedwait_relative_np`, making sure to clamp the duration to a maximum of $2^{64} - 1 \ \textrm{ns}$. I've added a miri shim as well, so the only thing missing is a definition of `pthread_cond_timedwait_relative_np` inside `libc`.

Rollup merge of #146503 - joboet:macos-condvar-timeout, r=ibraheemdev std: improve handling of timed condition variable waits on macOS Fixes #37440 (for good). This fixes two issues with `Condvar::wait_timeout` on macOS: Apple's implementation of `pthread_cond_timedwait` internally converts the absolute timeout to a relative one, measured in nanoseconds, but fails to consider overflow when doing so. This results in `wait_timeout` returning much earlier than anticipated when passed a duration that is slightly longer than `u64::MAX` nanoseconds (around 584 years). The existing clamping introduced by #42604 to address #37440 unfortunately used a maximum duration of 1000 years and thus still runs into the bug when run on older macOS versions (or with `PTHREAD_MUTEX_USE_ULOCK` set to a value other than "1"). See #37440 (comment) for context. Reducing the maximum duration alone however would not be enough to make the implementation completely correct. As macOS does not support `pthread_condattr_setclock`, the deadline passed to `pthread_cond_timedwait` is measured against the wall-time clock. `std` currently calculates the deadline by retrieving the current time and adding the duration to that, only for macOS to convert the deadline back to a relative duration by [retrieving the current time itself](https://github.com/apple-oss-distributions/libpthread/blob/1ebf56b3a702df53213c2996e5e128a535d2577e/src/pthread_cond.c#L802-L819) (this conversion is performed before the aforementioned problematic one). Thus, if the wall-time clock is adjusted between the `std` lookup and the system lookup, the relative duration could have changed, possibly even to a value larger than $2^{64}\ \textrm{ns}$. Luckily however, macOS supports the non-standard, tongue-twisting `pthread_cond_timedwait_relative_np` function which avoids the wall-clock-time roundtrip by taking a relative timeout. Even apart from that, this function is perfectly suited for `std`'s purposes: it is public (albeit badly-documented) API, [available since macOS 10.4](https://github.com/apple-oss-distributions/libpthread/blob/1ebf56b3a702df53213c2996e5e128a535d2577e/include/pthread/pthread.h#L555-L559) (that's way below our minimum of 10.12) and completely resilient against wall-time changes as all timeouts are [measured against the monotonic clock](https://github.com/apple-oss-distributions/xnu/blob/e3723e1f17661b24996789d8afc084c0c3303b26/bsd/kern/sys_ulock.c#L741) inside the kernel. Thus, this PR switches `Condvar::wait_timeout` to `pthread_cond_timedwait_relative_np`, making sure to clamp the duration to a maximum of $2^{64} - 1 \ \textrm{ns}$. I've added a miri shim as well, so the only thing missing is a definition of `pthread_cond_timedwait_relative_np` inside `libc`.

std: improve handling of timed condition variable waits on macOS Fixes rust-lang/rust#37440 (for good). This fixes two issues with `Condvar::wait_timeout` on macOS: Apple's implementation of `pthread_cond_timedwait` internally converts the absolute timeout to a relative one, measured in nanoseconds, but fails to consider overflow when doing so. This results in `wait_timeout` returning much earlier than anticipated when passed a duration that is slightly longer than `u64::MAX` nanoseconds (around 584 years). The existing clamping introduced by rust-lang/rust#42604 to address rust-lang/rust#37440 unfortunately used a maximum duration of 1000 years and thus still runs into the bug when run on older macOS versions (or with `PTHREAD_MUTEX_USE_ULOCK` set to a value other than "1"). See rust-lang/rust#37440 (comment) for context. Reducing the maximum duration alone however would not be enough to make the implementation completely correct. As macOS does not support `pthread_condattr_setclock`, the deadline passed to `pthread_cond_timedwait` is measured against the wall-time clock. `std` currently calculates the deadline by retrieving the current time and adding the duration to that, only for macOS to convert the deadline back to a relative duration by [retrieving the current time itself](https://github.com/apple-oss-distributions/libpthread/blob/1ebf56b3a702df53213c2996e5e128a535d2577e/src/pthread_cond.c#L802-L819) (this conversion is performed before the aforementioned problematic one). Thus, if the wall-time clock is adjusted between the `std` lookup and the system lookup, the relative duration could have changed, possibly even to a value larger than $2^{64}\ \textrm{ns}$. Luckily however, macOS supports the non-standard, tongue-twisting `pthread_cond_timedwait_relative_np` function which avoids the wall-clock-time roundtrip by taking a relative timeout. Even apart from that, this function is perfectly suited for `std`'s purposes: it is public (albeit badly-documented) API, [available since macOS 10.4](https://github.com/apple-oss-distributions/libpthread/blob/1ebf56b3a702df53213c2996e5e128a535d2577e/include/pthread/pthread.h#L555-L559) (that's way below our minimum of 10.12) and completely resilient against wall-time changes as all timeouts are [measured against the monotonic clock](https://github.com/apple-oss-distributions/xnu/blob/e3723e1f17661b24996789d8afc084c0c3303b26/bsd/kern/sys_ulock.c#L741) inside the kernel. Thus, this PR switches `Condvar::wait_timeout` to `pthread_cond_timedwait_relative_np`, making sure to clamp the duration to a maximum of $2^{64} - 1 \ \textrm{ns}$. I've added a miri shim as well, so the only thing missing is a definition of `pthread_cond_timedwait_relative_np` inside `libc`.

stepancheg force-pushed the timedwait branch from dbf0708 to 709cfe4 Compare June 12, 2017 16:06

rust-highfive assigned alexcrichton Jun 12, 2017

alexcrichton reviewed Jun 13, 2017

View reviewed changes

stepancheg force-pushed the timedwait branch from 709cfe4 to 24f915b Compare June 13, 2017 20:17

arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2017

alexcrichton reviewed Jun 13, 2017

View reviewed changes

stepancheg force-pushed the timedwait branch from 24f915b to 0c26b59 Compare June 15, 2017 21:53

bors merged commit 0c26b59 into rust-lang:master Jun 17, 2017

Mark-Simulacrum mentioned this pull request Jun 17, 2017

Unexpected behavior using std::thread::park_timeout on OSX #37440

Closed

joboet mentioned this pull request Sep 13, 2025

std: improve handling of timed condition variable waits on macOS #146503

Merged

Fix condvar.wait(distant future) return immediately on OSX #42604

Fix condvar.wait(distant future) return immediately on OSX #42604

Uh oh!

Conversation

stepancheg commented Jun 12, 2017

Uh oh!

nagisa commented Jun 12, 2017

Uh oh!

stepancheg commented Jun 12, 2017

Uh oh!

stepancheg commented Jun 12, 2017

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stepancheg commented Jun 13, 2017

Uh oh!

stepancheg commented Jun 13, 2017

Uh oh!

stepancheg commented Jun 13, 2017

Uh oh!

stepancheg commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stepancheg commented Jun 15, 2017

Uh oh!

alexcrichton commented Jun 16, 2017

Uh oh!

bors commented Jun 16, 2017

Uh oh!

bors commented Jun 17, 2017

Uh oh!

bors commented Jun 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants