1305876 - Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::operator[] | mozilla::TrackBuffersManager::GetSample

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-8366c767-19dc-401b-a360-ae5e62160927.
=============================================================

I see a handful of these. It looks like MediaSourceTrackDemuxer::DoGetSamples(int) is calling into TrackBuffersManager::GetSample, and maybe when calling GetTracksData(aTrack).mBuffers.LastElement() it turns out that mBuffers is empty.

Comment 1

[Daniel Veditz [:dveditz]]

I assume we're just reading what we think is media data, so worst case we might display stuff based on private memory?

Comment 2

[Jean-Yves Avenard [:jya]]

That crash report doesn't make any sense...
The code starts with:
https://hg.mozilla.org/releases/mozilla-beta/annotate/91faf7ec36cd/dom/media/mediasource/TrackBuffersManager.cpp#l2067


  if (aIndex >= track.Length()) {
  2068
    // reached the end.
  2069
    return nullptr;
  2070
  }
  2071

The first thing it does is checking that the index is less than the size of the array...

So how can we get to a point where we attempt to read past the size of the array?

The only explanation here is that the size of the array reported is nonsensical and greater than 2^32. NsTArray::Length returns a size_t. Which by design of a nsTArray is impossible (maximum number of item is 2^31)

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

jya: ElementAt(aIndex = 4294967295, aLength = 0)  4294967295 = 0xFFFFFFFF

It appears to be crashing *before* "if (aIndex >= track.Length())" - on GetTrackBuffer(aTrack), which as comment 0 notes, eventally calls GetTracksData(aTrack).mBuffers.LastElement().  mBuffers must be empty.

LastElement() looks like this:
 return ElementAt(Length() - 1);

Comment 4

[Jean-Yves Avenard [:jya]]

Interesting. 

Though that would make even less sense as we can only be called to retrieve a particular sample of a track if that track has been created. 

I can't see a way for GetTracksData(aTrack).mBuffers to be an empty array. 

But thanks for the pointer, it gives me a new thing to look at.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: 0001-Bug-1305876-P1.-Add-diagnostic-assert-to-more-easily.patch

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: 0002-Bug-1305876-P2.-Do-not-modify-internal-data-when-cal.patch

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: 0003-Bug-1305876-P3.-Make-function-members-const.-r-geral.patch

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8800174 [details] [diff] [review]
0002-Bug-1305876-P2.-Do-not-modify-internal-data-when-cal.patch

Review of attachment 8800174 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a suggestion:

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +325,4 @@
>  
>    // 4. For each track buffer managed by this SourceBuffer, run the following steps:
>    //   1. Let track ranges equal the track buffer ranges for the current track buffer.
> +  for (uint32_t i = 0; i < tracks.Length();  i++) {

I don't think you needed you avoid a range-for.
To make sure it's doing what you want, you could specify the loop-variable type, e.g.:
  for (const TimeIntervals* tR : tracks) {
    TimeIntervals trackRanges = *tR;

Comment 9

[Jean-Yves Avenard [:jya]]

(In reply to Gerald Squelart [:gerald] from comment #8)
> Comment on attachment 8800174 [details] [diff] [review]
> 0002-Bug-1305876-P2.-Do-not-modify-internal-data-when-cal.patch
> 
> Review of attachment 8800174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with a suggestion:
> 
> ::: dom/media/mediasource/TrackBuffersManager.cpp
> @@ +325,4 @@
> >  
> >    // 4. For each track buffer managed by this SourceBuffer, run the following steps:
> >    //   1. Let track ranges equal the track buffer ranges for the current track buffer.
> > +  for (uint32_t i = 0; i < tracks.Length();  i++) {
> 
> I don't think you needed you avoid a range-for.
> To make sure it's doing what you want, you could specify the loop-variable
> type, e.g.:
>   for (const TimeIntervals* tR : tracks) {
>     TimeIntervals trackRanges = *tR;

Yes, but I wanted to avoid the unnecessary copy that would happen most of the time (under most cases the buffered range is queried when the mediasource isn't in ended mode).

Hence why I did it like that...

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8800173 [details] [diff] [review]
0001-Bug-1305876-P1.-Add-diagnostic-assert-to-more-easily.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? very hard, I can't see how in fact

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? it doesn't. Plus we aren't fixing the problem, just adding a release assert

Which older supported branches are affected by this flaw? 42 (and as such ESR45)

If not all supported branches, which bug introduced the flaw? 1171330. I don't think there's any flaw yet though...

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? easy for P1

How likely is this patch to cause regressions; how much testing does it need? It will cause a proper crash if that happens. And I think this is related to other GetSamples crashes we've been getting for a long time.

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> (In reply to Gerald Squelart [:gerald] from comment #8)
> > I don't think you needed you avoid a range-for.
> > To make sure it's doing what you want, you could specify the loop-variable
> > type, e.g.:
> >   for (const TimeIntervals* tR : tracks) {
> >     TimeIntervals trackRanges = *tR;
> 
> Yes, but I wanted to avoid the unnecessary copy that would happen most of
> the time (under most cases the buffered range is queried when the
> mediasource isn't in ended mode).
> 
> Hence why I did it like that...

I'm dubious that there is more copying in the range-for option than with the numbered-for case.
E.g., in the non-ended case, 'intersection.Intersection(*tracks[i]);' still accesses the TimeIntervals pointer (through an index-accessor instead of through the loop iterator) and dereferences it.

I was suggesting the range-for more for clarity than performance.

If performance is that important here, then I would suggest doing some profiling to pick the best solution!

Comment 12

[Jean-Yves Avenard [:jya]]

(In reply to Gerald Squelart [:gerald] from comment #11)
> I'm dubious that there is more copying in the range-for option than with the
> numbered-for case.
> E.g., in the non-ended case, 'intersection.Intersection(*tracks[i]);' still
> accesses the TimeIntervals pointer (through an index-accessor instead of
> through the loop iterator) and dereferences it.

none of which will make a copy. All called code uses const references.

> 
> I was suggesting the range-for more for clarity than performance.
> 
> If performance is that important here, then I would suggest doing some
> profiling to pick the best solution!

it's cheap enough to not do it IMHO, plus i find the code perfectly clear as it is :)

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8800173 [details] [diff] [review]
0001-Bug-1305876-P1.-Add-diagnostic-assert-to-more-easily.patch

As a sec-moderate rated issue, this doesn't need sec-approval. You can just check it into trunk. If we want to backport this to more than Aurora, we would need to discuss the ramifications of doing so.

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8800174 [details] [diff] [review]
0002-Bug-1305876-P2.-Do-not-modify-internal-data-when-cal.patch

Approval Request Comment
[Feature/regressing bug #]: 1171330
[User impact if declined]: Returning incorrect buffered range for MSE under some circumstances.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Very low. Spec compliant issue..
[String/UUID change made/needed]: None

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8800173 [details] [diff] [review]
0001-Bug-1305876-P1.-Add-diagnostic-assert-to-more-easily.patch

Approval Request Comment
[Feature/regressing bug #]: 1305876
[User impact if declined]: Can't find why and where it's crashing. While this bug is low, I have the gutfeel that it's the same as bug 1247189.
[Describe test coverage new/current, TreeHerder]: local test. This change is functionally identical to what was there before, it just crashes if something that can't happen happened 
[Risks and why]: It will crash prior doing a possibly out of bound memory access.
[String/UUID change made/needed]: none

Comment 16

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4bcd1f78d5cbc89e4dca6ea33ec188c1dcf02273
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b46ebdc8289865010ba7a2d75be657b77baab5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ae7d9cd010790f18f1dfb9903ef9b5a47f685600

Comment 17

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

Hi Al, Dan, from this point onwards, I'd prefer to only take sec-high and sec-crits in the Beta50 cycle. This one was nominated for uplift to beta. Also see Al's comment 13. Is this something we ought to take in 50?

Comment 18

[Jean-Yves Avenard [:jya]]

I only nominated patch 2 for beta uplift. It's something I noticed while I was looking at the code. It was a face palm moment really. 

It's very safe really, hence why I thought it could be nominated for beta uplift, despite the lateness. It has nothing to do with the bug at hand.

Comment 19

[Daniel Veditz [:dveditz]]

We probably need a separate bug to uplift only part of this, so we can explain the patch, give it a security rating (or explicitly call it out as not a security bug) so we can write an appropriate advisory, and so it can (maybe) be tested.

Comment 20

[Daniel Veditz [:dveditz]]

Then again why do that work if it's going to be denied? Maybe just explain the above here, and then create the sub-bug if Ritu will let it into beta.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4bcd1f78d5cb
https://hg.mozilla.org/mozilla-central/rev/f0b46ebdc828
https://hg.mozilla.org/mozilla-central/rev/ae7d9cd01079

Comment 22

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

Comment on attachment 8800174 [details] [diff] [review]
0002-Bug-1305876-P2.-Do-not-modify-internal-data-when-cal.patch

Jean-Yves emphatically believes this is low risk and I trust him, Beta50+

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Should I uplift these in this bug or are you splitting this off to a sub-bug?

Comment 24

[Daniel Veditz [:dveditz]]

There's no "these", only patch 2 gets uplifted. We're not creating a sub-bug.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/11ba9af07367

Comment 26

[Gerry Chang [:gchang]]

Comment on attachment 8800173 [details] [diff] [review]
0001-Bug-1305876-P1.-Add-diagnostic-assert-to-more-easily.patch

Fix a crash. Take it in 51 aurora.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/87144d917608

Comment 28

[Jean-Yves Avenard [:jya]]

This actual crash wasn't uplifted to the various version, only a side bug.

As such, it is still happening in release and beta.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

I don't understand. What is affected in 51. What bug is there that was then fixed in 52?

Comment 30

[Jean-Yves Avenard [:jya]]

(In reply to Al Billings [:abillings] from comment #29)
> I don't understand. What is affected in 51. What bug is there that was then
> fixed in 52?

what was actually fixed was a side problem, with no link to the bug title. data were self-modifying when calling a JS instruction.