Bug 157937 – WTF::Condition::waitFor() will time out immediately for relativeTimeout values with very large tick counts

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Web Template Framework

[Andy Estes]

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.

[Radar WebKit Bug Importer]

<rdar://problem/26382498>

[Filip Pizlo]

(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.

[Geoffrey Garen]

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. :(

[Filip Pizlo]

(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.

[Filip Pizlo]

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