Closed
Bug 1305876
Opened 8 years ago
Closed 8 years ago
Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::operator[] | mozilla::TrackBuffersManager::GetSample
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: mccr8, Assigned: jya)
Details
(Keywords: crash, csectype-bounds, sec-other, Whiteboard: [post-critsmash-triage][adv-main50+][adv-main52-])
Crash Data
Attachments
(3 files)
5.56 KB,
patch
|
mozbugz
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.99 KB,
patch
|
mozbugz
:
review+
gchang
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
7.69 KB,
patch
|
mozbugz
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
I assume we're just reading what we think is media data, so worst case we might display stuff based on private memory?
Keywords: csectype-bounds,
sec-moderate
Assignee | ||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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);
Component: Audio/Video → Audio/Video: Playback
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
Flags: needinfo?(jyavenard)
Attachment #8800173 -
Flags: review?(gsquelart)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8800174 -
Flags: review?(gsquelart)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8800175 -
Flags: review?(gsquelart)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jyavenard
Attachment #8800173 -
Flags: review?(gsquelart) → review+
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;
Attachment #8800174 -
Flags: review?(gsquelart) → review+
Attachment #8800175 -
Flags: review?(gsquelart) → review+
Assignee | ||
Comment 9•8 years ago
|
||
(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...
Assignee | ||
Comment 10•8 years ago
|
||
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.
Attachment #8800173 -
Flags: sec-approval?
(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!
Assignee | ||
Comment 12•8 years ago
|
||
(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•8 years ago
|
||
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.
Attachment #8800173 -
Flags: sec-approval?
Assignee | ||
Comment 14•8 years ago
|
||
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
Attachment #8800174 -
Flags: approval-mozilla-beta?
Attachment #8800174 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 15•8 years ago
|
||
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
Attachment #8800173 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 16•8 years ago
|
||
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
status-firefox50:
--- → affected
status-firefox51:
--- → affected
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?
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Assignee | ||
Comment 18•8 years ago
|
||
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•8 years ago
|
||
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.
Flags: needinfo?(dveditz)
Comment 20•8 years ago
|
||
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•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4bcd1f78d5cb https://hg.mozilla.org/mozilla-central/rev/f0b46ebdc828 https://hg.mozilla.org/mozilla-central/rev/ae7d9cd01079
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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+
Attachment #8800174 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Should I uplift these in this bug or are you splitting this off to a sub-bug?
Flags: needinfo?(dveditz)
Comment 24•8 years ago
|
||
There's no "these", only patch 2 gets uplifted. We're not creating a sub-bug.
Flags: needinfo?(dveditz)
Updated•8 years ago
|
Flags: needinfo?(abillings)
Comment 26•8 years ago
|
||
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.
Attachment #8800173 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8800174 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Group: media-core-security → core-security-release
Updated•8 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+]
Assignee | ||
Comment 28•7 years ago
|
||
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•7 years ago
|
||
I don't understand. What is affected in 51. What bug is there that was then fixed in 52?
Flags: needinfo?(jyavenard)
Updated•7 years ago
|
Whiteboard: [post-critsmash-triage][adv-main50+] → [post-critsmash-triage][adv-main50+][adv-main52-]
Updated•7 years ago
|
Group: core-security-release
Keywords: sec-moderate → sec-other
Assignee | ||
Comment 30•7 years ago
|
||
(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.
Flags: needinfo?(jyavenard)
You need to log in
before you can comment on or make changes to this bug.
Description
•