Closed
Bug 1202556
Opened 9 years ago
Closed 9 years ago
TimeStamp arithmetic operators do not detect underflow
Categories
(Core :: mozglue, defect)
Core
mozglue
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
2.40 KB,
patch
|
froydnj
:
feedback+
|
Details | Diff | Splinter Review |
The following operators do not detect the case where the passed in TimeDuration produces a time less than or equal to the zero-time of the underlying clock. TimeStamp TimeStamp::operator+(const TimeDuration& aOther) const TimeStamp TimeStamp::operator-(const TimeDuration& aOther) const TimeStamp& TimeStamp::operator+=(const TimeDuration& aOther) TimeStamp& TimeStamp::operator-=(const TimeDuration& aOther) I'm hitting this particularly on Linux (on try). For Linux we use clock_gettime with CLOCK_MONOTONIC which represents, "represents monotonic time since some unspecified starting point."[1] [1] http://linux.die.net/man/3/clock_gettime For example, for the operator+ method, it's quite easy to specify a negative time duration that would create a TimeStamp before the "unspecified starting point." In terms of handling this we could: 1. Detect it and return a null timestamp in such a case 2. Add an assertion when this happens and make it the call-site's responsibility to detect this case appropriately 3. Add a warning Or perhaps some combination of the above. I'm currently leaning towards 2 but if I discover this throws assertions all over the place I might have to reconsider. I don't think doing nothing is an option since I was getting odd test failures due to this that took nearly 2 days to debug (since they only happened on try).
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
After trying 2 from comment 0, I realized that client code can't really detect this case easily so it's better to do 1 and 3. Unfortunately, I'm not sure if we can add warnings in mozglue code. Mike, in bug 858927 comment 29 you said we don't *yet* have a way to do logging in mozglue. Has that changed? It would really help here.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Updated•9 years ago
|
Summary: TimeStamp addition/subtraction operators do not detect backwards wrapping → TimeStamp arithmetic operators do not detect underflow
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8658530 -
Flags: review?(nfroyd)
Comment 4•9 years ago
|
||
Comment on attachment 8658530 [details] [diff] [review] Detect underflow in TimeStamp addition/subtraction operators Review of attachment 8658530 [details] [diff] [review]: ----------------------------------------------------------------- Can you file two followup bugs: - One for unifying the operator+ and operator+= (resp. operator-) implementations - One for writing some tests for TimeStamp ? Thank you in advance! ::: mozglue/misc/TimeStamp.h @@ +493,5 @@ > { > MOZ_ASSERT(!IsNull(), "Cannot compute with a null value"); > + TimeStampValue value = mValue + aOther.mValue; > + // Check for underflow. > + if (aOther.mValue < 0 && value > mValue) { I realize underflow is easier to trigger here than overflow, but I'm wondering if we shouldn't just bite the bullet and check for overflow as well.
Attachment #8658530 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Add overflow detection
Attachment #8658994 -
Flags: review?(nfroyd)
Assignee | ||
Updated•9 years ago
|
Attachment #8658530 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
Actually put the changes in the right patch this time
Attachment #8658998 -
Flags: review?(nfroyd)
Assignee | ||
Updated•9 years ago
|
Attachment #8658994 -
Attachment is obsolete: true
Attachment #8658994 -
Flags: review?(nfroyd)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #4) > - One for unifying the operator+ and operator+= (resp. operator-) > implementations Filed bug 1203350 > - One for writing some tests for TimeStamp Filed bug 1203351
Comment 8•9 years ago
|
||
Comment on attachment 8658998 [details] [diff] [review] Detect underflow and overflow in TimeStamp addition/subtraction operators Review of attachment 8658998 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/misc/TimeStamp.h @@ +495,5 @@ > + TimeStampValue value = mValue + aOther.mValue; > + // Check for underflow/overflow. > + if ((aOther.mValue < 0 && value > mValue) || > + (aOther.mValue > 0 && value < mValue)) { > + value = 0; These checks look fine, but overflow ought to set it to LARGE_VALUE, rather than 0, no?
Attachment #8658998 -
Flags: review?(nfroyd) → feedback+
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #8) > These checks look fine, but overflow ought to set it to LARGE_VALUE, rather > than 0, no? I was thinking we should just use null as the general error state. Then calling code can test IsNull() to check for an error. If we set it to LARGE_VALUE there will be no way to detect that the returned timestamp is bogus. While LARGE_VALUE might happen to give you the desired result in some cases, I think once these LARGE_VALUE timestamps propagate throughout the system you'd start to get odd effects. For example, if you subtract from the LARGE_VALUE timestamp you'll get something completely useless.
Assignee | ||
Comment 10•9 years ago
|
||
Nathan, what do you think about using null for overflow as per comment 9?
Flags: needinfo?(nfroyd)
Comment 11•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #10) > Nathan, what do you think about using null for overflow as per comment 9? Overloading 0 as "base time" and "error value" seems, well, error-prone. If you want an error state, I think it'd be better to force people to check for errors (IsError()) and/or refuse to operate with error values. To your example, I'm not sure why subtracting from LARGE_VALUE would be useless...if you were computing something that would have overflowed, giving you LARGE_VALUE, and then you subtract from it, the resulting value seems "more right" to me than handing you zero instead. Then again, maybe 64-bit timestamp values are simply so huge that we don't have to ever worry about overflow? Given that it's (seemingly) not been a problem, and that there's not an obvious right solution, why don't we just go with checking for underflow, and leave a comment about why we don't check for overflow. Sound good?
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #11) > (In reply to Brian Birtles (:birtles) from comment #10) > > Nathan, what do you think about using null for overflow as per comment 9? > > Overloading 0 as "base time" and "error value" seems, well, error-prone. I'm not sure I understand that, or at least "base time". Zero just means "null" timestamp; it's not a useful base time, just a sentinel value. > If > you want an error state, I think it'd be better to force people to check for > errors (IsError()) and/or refuse to operate with error values. We already do that--we assert if a null timestamp is passed into most operations. > To your example, I'm not sure why subtracting from LARGE_VALUE would be > useless...if you were computing something that would have overflowed, giving > you LARGE_VALUE, and then you subtract from it, the resulting value seems > "more right" to me than handing you zero instead. I'm suggesting you would get "null" when it first overflowed. If you failed to check the result of that operation and then tried to subtract from the null result you'd get an assertion failure. > Then again, maybe 64-bit timestamp values are simply so huge that we don't > have to ever worry about overflow? Given that it's (seemingly) not been a > problem, and that there's not an obvious right solution, why don't we just > go with checking for underflow, and leave a comment about why we don't check > for overflow. Sound good? Yeah, that's fine with me.
Comment 13•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #12) > (In reply to Nathan Froyd [:froydnj] from comment #11) > > (In reply to Brian Birtles (:birtles) from comment #10) > > > Nathan, what do you think about using null for overflow as per comment 9? > > > > Overloading 0 as "base time" and "error value" seems, well, error-prone. > > I'm not sure I understand that, or at least "base time". Zero just means > "null" timestamp; it's not a useful base time, just a sentinel value. > > > If > > you want an error state, I think it'd be better to force people to check for > > errors (IsError()) and/or refuse to operate with error values. > > We already do that--we assert if a null timestamp is passed into most > operations. Ah, I forgot that we do that already. OK, maybe doing 0 for overflow is not so bad... > > To your example, I'm not sure why subtracting from LARGE_VALUE would be > > useless...if you were computing something that would have overflowed, giving > > you LARGE_VALUE, and then you subtract from it, the resulting value seems > > "more right" to me than handing you zero instead. > > I'm suggesting you would get "null" when it first overflowed. If you failed > to check the result of that operation and then tried to subtract from the > null result you'd get an assertion failure. Right. What about the cases where the asserts aren't active? LARGE_VALUE makes more sense there...maybe?
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/820f227f2a40
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•