Bug 157937 - WTF::Condition::waitFor() will time out immediately for relativeTimeout values with very large tick counts
Summary: WTF::Condition::waitFor() will time out immediately for relativeTimeout value...
Status: RESOLVED DUPLICATE of bug 152045
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-19 17:53 PDT by Andy Estes
Modified: 2016-11-03 12:32 PDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2016-05-19 17:53:56 PDT
As discussed in <https://bugs.webkit.org/show_bug.cgi?id=157924>, WTF::Condition::waitFor() times out immediately if given a relativeTimeout of std::chrono::milliseconds::max(), due to two signed integer overflow bugs conspiring against us in Condition::absoluteFromRelative().

The first happens in this comparison:

        if (relativeTimeout > Clock::duration::max()) {

std::chrono::duration converts the operands of its inequality operators to the type common to both durations (using std::common_type) before performing the comparison. In this case that's nanoseconds, and converting milliseconds::max() to nanoseconds will overflow since they both use the same underlying data type.

The second happens on this line, for the same reason, except this time the conversion is explicit:

        Clock::duration myRelativeTimeout =
            std::chrono::duration_cast<Clock::duration>(relativeTimeout);

Since the check that was supposed to protect us from overflowing itself overflowed, we now have a negative relative timeout.
Comment 1 Radar WebKit Bug Importer 2016-05-19 17:55:06 PDT
<rdar://problem/26382498>
Comment 2 Filip Pizlo 2016-05-21 19:41:00 PDT
(In reply to comment #0)
> As discussed in <https://bugs.webkit.org/show_bug.cgi?id=157924>,
> WTF::Condition::waitFor() times out immediately if given a relativeTimeout
> of std::chrono::milliseconds::max(), due to two signed integer overflow bugs
> conspiring against us in Condition::absoluteFromRelative().
> 
> The first happens in this comparison:
> 
>         if (relativeTimeout > Clock::duration::max()) {
> 
> std::chrono::duration converts the operands of its inequality operators to
> the type common to both durations (using std::common_type) before performing
> the comparison. In this case that's nanoseconds, and converting
> milliseconds::max() to nanoseconds will overflow since they both use the
> same underlying data type.

Wow!  I did not know about this behavior.  That's so awkward!  I don't think I would have been so enthusiastic about using std::chrono if I had known how overflow-prone it was.

> 
> The second happens on this line, for the same reason, except this time the
> conversion is explicit:
> 
>         Clock::duration myRelativeTimeout =
>             std::chrono::duration_cast<Clock::duration>(relativeTimeout);
> 
> Since the check that was supposed to protect us from overflowing itself
> overflowed, we now have a negative relative timeout.

Dang, that's too funny.
Comment 3 Geoffrey Garen 2016-05-22 18:20:07 PDT
Kind of sad that std::chrono devotes so much boilerplate to giving each time unit its own type, only to explode anyway due to overflow. :(
Comment 4 Filip Pizlo 2016-05-22 18:40:26 PDT
(In reply to comment #3)
> Kind of sad that std::chrono devotes so much boilerplate to giving each time
> unit its own type, only to explode anyway due to overflow. :(

Yes.  I think we need to stop using std::chrono.  I just sent a proposal to that effect to webkit-dev.
Comment 5 Filip Pizlo 2016-11-03 12:32:32 PDT

*** This bug has been marked as a duplicate of bug 152045 ***