1533777 - Change assert in CheckedInt::value to a MOZ_RELEASE_ASSERT

Status: RESOLVED FIXED

(Core :: MFBT, defect, P2)

Description

[Alex Gaynor [:Alex_Gaynor]]

Right now it's a MOZ_ASSERT. This means that if you forget to call isValid(), in a release build you'll get an overflowed value, leading to sadness.

The good news is that since all correct callers should already be checking isValid(), the compiler should be able to easily remove the MOZ_RELEASE_ASSERT -- no performance regression for incorrect code, extra safety for incorrect code! Huzzah!

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1533777 - use a release assert in CheckedInt::value; r?froydnj

Comment 2

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/2f2a20c16289

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2f2a20c16289

Comment 4

[Kelsey Gilbert [:jgilbert]]

Thanks!

Comment 5

[Jean-Yves Avenard [:jya]]

I believe that pushing this in the freeze zone is dangerous.

the assumption that all caller would be doing a check first is obviously incorrect.

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #0)

The good news is that since all correct callers should already be checking isValid(), the compiler should be able to easily remove the MOZ_RELEASE_ASSERT -- no performance regression for incorrect code, extra safety for incorrect code! Huzzah!

I seriously doubt that this statement is correct even if the caller was doing the right thing.

Comment 7

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 2f2a20c16289 (bug 1533777) for crashing youtube on request by jya

Backout: https://hg.mozilla.org/mozilla-central/rev/9067457d7dcf72804288f61fc18326f31b2fc140

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Jean-Yves Avenard [:jya] from comment #5)

the assumption that all caller would be doing a check first is obviously incorrect.

Doing the check before calling value() is the documented way of using this class. If callers aren't following the contract, they should be fixed. What would value() contain if isValid() is false? (I see that we have made it contain 0 in many cases, which has probably encouraged people to rely on incorrect API usage.)

Comment 9

[Jean-Yves Avenard [:jya]]

(In reply to Nathan Froyd [:froydnj] from comment #8)

(In reply to Jean-Yves Avenard [:jya] from comment #5)

the assumption that all caller would be doing a check first is obviously incorrect.

Doing the check before calling value() is the documented way of using this class. If callers aren't following the contract, they should be fixed. What would value() contain if isValid() is false? (I see that we have made it contain 0 in many cases, which has probably encouraged people to rely on incorrect API usage.)

Absolutely. It should be fixed.

However, this should go after 67 has merged to beta, so we have time fixing the invalid use without rushing.

I also agree with your review comment that a DIAGNOSTIC_ASSERT would have been preferred.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Jean-Yves Avenard [:jya] from comment #9)

(In reply to Nathan Froyd [:froydnj] from comment #8)

(In reply to Jean-Yves Avenard [:jya] from comment #5)

the assumption that all caller would be doing a check first is obviously incorrect.

Doing the check before calling value() is the documented way of using this class. If callers aren't following the contract, they should be fixed. What would value() contain if isValid() is false? (I see that we have made it contain 0 in many cases, which has probably encouraged people to rely on incorrect API usage.)

Absolutely. It should be fixed.

However, this should go after 67 has merged to beta, so we have time fixing the invalid use without rushing.

We are in violent agreement.

I also agree with your review comment that a DIAGNOSTIC_ASSERT would have been preferred.

I think Alex's explanation of why the compiler should be able to optimize it away is a correct assessment of the situation. OTOH, I suppose it's possible that for something like media's TimeUnit, you could have a collection of TimeUnits where some caller has ensured that all members of the collection are isValid() and therefore you can call value() when manipulating the collection without penalty. (Or other similar scenarios.) I think that such a scenario is still analogous to why we unconditionally turn on array bounds checking. Maybe we could provide escape hatches, but I'd want to see some performance numbers first.

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Yup -- landing during the freeze period was a mistake, sorry about that.

Comment 12

[Calixte Denizet (:calixte)]

There are 2 crashes with signature 'mozilla::media::TimeUnit::operator>=' and the moz_crash_reason is:
MOZ_RELEASE_ASSERT(mIsValid) (Invalid checked integer (division by zero or integer overflow))

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Looks like :jya fixed at least one of them -- are these all the same root cause, or do they need their own fixes?

Comment 14

[Jean-Yves Avenard [:jya]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #13)

Looks like :jya fixed at least one of them -- are these all the same root cause, or do they need their own fixes?

so far they are different.
The one I fixed will likely have significant number by tomorrow, it makes YouTube crash as soon as you seek. And YT is over 65% of what people spend time on with a web browser!

When I made mEnd >= mStart a MOZ_DIAGNOSTIC_ASSERT in Intervals.h (bug 1530322) (most of them using TimeUnits), it took over 2 weeks to eradicate them all, and there's still 2-3 crashes daily for remaining issue.

The problem here, in all these case, we're dealing with external user data; and while the vast majority of all cases are covered there are still some.

Some sites in particular use EPOCH time measured in microseconds and then they play with media offsets. that's a lot of cases to handle.

Comment 15

[Alex Gaynor [:Alex_Gaynor]]

Eeep! Is it possible to write tests for these cases, so we (I) won't accidentally break them again?

Comment 16

[Jean-Yves Avenard [:jya]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #15)

Eeep! Is it possible to write tests for these cases, so we (I) won't accidentally break them again?

for those solved in those past few weeks, I have opened 1531991.

I will ping the fuzzing team to re-do a run targeting that code

Comment 17

[Alex Gaynor [:Alex_Gaynor]]

:jya, since bug 1534993 is still open I'm not planning on relanding this. Do you expect to be able to get to that bug sometime early in the cycle?

Comment 18

[Jean-Yves Avenard [:jya]]

I'd be okay to push that in now, we have enough time in the cycle to look into it.

Comment 19

[Alex Gaynor [:Alex_Gaynor]]

Thanks!

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/f9c865b93ecd4d184b50de9a5ca9da15928c27a8

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out because arc changed the author when it got pulled and pushed to be able to land it again with Lando: https://hg.mozilla.org/integration/autoland/rev/07595d5c98d2137f0a7dd4334b0c315988b8642e
Re-landed: https://hg.mozilla.org/integration/autoland/rev/16f19322ec762261a5abe9e70c935e6e6bf90582

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/16f19322ec76

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Alex, since 67 is marked as affected, should we consider uplifting this to Beta67? If yes, please nominate a patch for uplift.

Comment 24

[Jean-Yves Avenard [:jya]]

(In reply to Ritu Kothari (:ritu) from comment #23)

Hi Alex, since 67 is marked as affected, should we consider uplifting this to Beta67? If yes, please nominate a patch for uplift.

please don't.

Comment 25

[Alex Gaynor [:Alex_Gaynor]]

Yes, like :jya said, this should not be uplifted.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks, with that I'll mark this wontfix for 67.

Comment 27

[Calixte Denizet (:calixte)]

There 26 crashes with signature 'mozilla::CheckedInt<T>::value' in nightly 68 starting with buildid 20190329131714.
The moz_crash_reason is always MOZ_RELEASE_ASSERT(mIsValid) (Invalid checked integer (division by zero or integer overflow)).

Comment 28

[Alex Gaynor [:Alex_Gaynor]]

Finding those issues is the expected result of this -- we should file bugs blocking this for those (e.g. bug 1534893), I don't think reopening this bug is correct.

Comment 30

[Andrew McCreight [:mccr8]]

I've filed bugs for all of the assertions that have shown up on Nightly so far, splitting them based on the top most "interesting" non-generic library frame. Of note, they are all on Android, and all in media code. It is also possible there are other crashes with different signatures, but I haven't been able to get the search on raw crash reason to work.

Comment 31

[Andrew McCreight [:mccr8]]

Ok, I did get the raw crash reason to work. I think I had the product set to Firefox, which excluded Android. Every crash where the raw reason contains "mIsValid" does have the signature mozilla::CheckedInt<T>::value.