1835044 - nsTArray bounds check crash in mozilla::media::IntervalSet

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jan de Mooij [:jandem]]

I was looking at crash URLs for bug 1834386 and, trying one of them (see URL field, you may have to check "I agree" and then "Next"), I get this crash:

https://crash-stats.mozilla.org/report/index/1d50e38d-139b-45d7-95e5-99a400230525

Marking as security-sensitive in case there are issues not caught by the nsTArray release assertions.

Comment 1

[Jan de Mooij [:jandem]]

Note that you need to be on the latest Nightly (from a few hours ago) because else it crashes due to bug 1834984.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

This bug has the keyword crash, so its type should be defect.

Comment 3

[Andrew McCreight [:mccr8]]

This crash is a bit odd because the crash annotation seems to be missing.

Comment 4

[Paul Adenot (:padenot)]

That's a fun one.

This site doesn't play here in France, but before that it can crash because a couple buffers are appended. It's using MSE, and the start time in their mp4 container is about 53 years or so. So we do all our calculations in the fallback code that translates the rational numbers into doubles, and there was a bug in the operator>= implementation in the fallback path: https://searchfox.org/mozilla-central/rev/daedd554ae8a2c7f420ad77311134c8c298ba318/dom/media/TimeUnits.cpp#204-207. But an earlier interval computation was comparing the numbers in a different way, resulting in a correct comparison code being used.

Those are meant to be divisions (or we can also swap the multiplicators, so we multiply the number in the same base, but it's easier convert to seconds: the numbers are smaller, the precision is better).

And also the unit test for this was wrong of course.

Comment 5

[Paul Adenot (:padenot)]

Attachment: Bug 1835044 - Properly compare extremely large TimeUnits. r?alwu

Comment 6

[Paul Adenot (:padenot)]

I plan to land this directly because it's nightly-only and we know precisely when this landed (last week).

Comment 7

[Andrew McCreight [:mccr8]]

I'll mark this sec-other. It sounds like there's no known security issue and it is Nightly only with a patch up for review, but it does seem like there could be some issues somewhere so we might want to leave it hidden until a fix lands.

Comment 8

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

Properly compare extremely large TimeUnits. r=alwu
https://hg.mozilla.org/integration/autoland/rev/2f0238fa0a6b6167a7fbf5cbeb1803b579fdbe85
https://hg.mozilla.org/mozilla-central/rev/2f0238fa0a6b