Closed
Bug 1202677
Opened 9 years ago
Closed 9 years ago
Hit MOZ_CRASH(css::ImageValue not thread-safe) at c:/Users/mozilla/debug-builds/ mozilla-central/layout/style/nsCSSValue.cpp:2475
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla43
People
(Reporter: cbook, Assigned: rillian)
References
()
Details
(4 keywords, Whiteboard: [post-critsmash-triage][adv-main42+])
Crash Data
Attachments
(3 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
mozbugz
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
video/mp4
|
Details |
Found by Bughunter
Hit MOZ_CRASH(css::ImageValue not thread-safe) at c:/Users/mozilla/debug-builds/
mozilla-central/layout/style/nsCSSValue.cpp:2475
### ERROR: CreateThread: Access is denied.
Steps to reproduce:
--> Load http://www.musepen.com/blog/adobe-muse-hover-rolling-push-widget/
--> Hit MOZ_CRASH(css::ImageValue not thread-safe) at c:/Users/mozilla/debug-builds/
mozilla-central/layout/style/nsCSSValue.cpp:2475
### ERROR: CreateThread: Access is denied.
Comment 1•9 years ago
|
||
That stack looks pretty bizarre. I'm a little more inclined to believe the part of the stack suggesting the problem lies in the media code, rather than the CSS code. ni? to Ralph.
Component: General → Audio/Video
Flags: needinfo?(giles)
Updated•9 years ago
|
Group: core-security → media-core-security
Keywords: crash,
testcase-wanted
Comment 2•9 years ago
|
||
When I load this page in Nightly, it crashes immediately in libstagefright:
https://crash-stats.mozilla.com/report/index/992c4366-61a1-47e7-9e2b-6e0e32150908
Flags: needinfo?(jyavenard)
Comment 3•9 years ago
|
||
Could you take a look at this jya? Thanks.
Updated•9 years ago
|
Crash Signature: [@ __android_log_assert | stagefright::MPEG4Source::MPEG4Source(stagefright::sp<T> const&, stagefright::sp<T> const&, int, stagefright::sp<T> const&, stagefright::Vector<T>&, stagefright::MPEG4Extractor::TrackExtends&) ]
Assignee | ||
Comment 5•9 years ago
|
||
Reproducible with 42 on Mac as well.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 6•9 years ago
|
||
Looks like the avcC block is zero-length. The chunk parser faithfully registers a zero-length header, which is (properly) caught by the assert, so we're just back the problem of stagefright's tendency to handle errors with runtime asserts.
We can reject the stream instead, which should turn the crash into a playback failure.
Assignee | ||
Comment 7•9 years ago
|
||
Patch to reject the bad streams before we assert.
Safari can't play the file, but VLC can, so we could do something more permissive, but this addresses the immediate issue.
Assignee: nobody → giles
Attachment #8658367 -
Flags: review?(gsquelart)
Assignee | ||
Comment 8•9 years ago
|
||
The file causing the crash from comment #2 is fairly small, so attaching here for reference.
Updated•9 years ago
|
Flags: needinfo?(jyavenard)
Assignee | ||
Updated•9 years ago
|
Keywords: testcase-wanted → testcase
Comment 9•9 years ago
|
||
I don't think you need to even return an error. It could simply be a badly marked AVC3 stream (where the extra data is inline rather than stored in the avcC atom)
If you simply skip the atom, the H264Converter will automatically start scanning the inner data to find the SPS/PPS
Attachment #8658367 -
Flags: review?(gsquelart) → review+
Comment 10•9 years ago
|
||
IMHO that should be r- , there's no reason we can't play that file especially as other player can play it. Safari only handles avc1 so hardly a reference.
(In reply to Jean-Yves Avenard [:jya] from comment #10)
> IMHO that should be r- , there's no reason we can't play that file
> especially as other player can play it. Safari only handles avc1 so hardly a
> reference.
Didn't see your comment 9 before reviewing.
I think r+ is appropriate as the patch does what is intended, with known limitations (see comment 7), and because fixing a crash quickly is preferable.
Of course, if we could make it even better that'd be good.
Ralph, would you like to submit another patch, or open another bug?
Flags: needinfo?(giles)
Assignee | ||
Comment 12•9 years ago
|
||
We've got plenty of security issues to fix without worrying about making stagefright handle broken files, so I'm happy with the current patch. We can open a follow up bug if you want once this one lands.
Flags: needinfo?(giles)
Assignee | ||
Comment 13•9 years ago
|
||
The stack unwinding into css code in the initial report is worrying, but I only see this asserting in the stagefright code, which I believe is sec-low. Daniel, does that seem correct to you?
Flags: needinfo?(dveditz)
Keywords: csectype-dos,
sec-low
Comment 14•9 years ago
|
||
I agree completely with comment 13: the initial stack is confusing and scary, but everyone else has seen a simple intentional assertion-induced crash (which happens in Release builds, not just debug). Doesn't even rate sec-low, really, unless we can figure out how Carsten got the stack he saw. A nice simple stability win by rejecting these broken files is fine.
Flags: needinfo?(dveditz)
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6d311c5911c057a9333ce82f2869ebe484b540
Bug 1202677 - Reject mp4 streams with short avc chunks. r=gerald
Assignee | ||
Comment 16•9 years ago
|
||
Thanks, Daniel. Setting a reminder to request aurora uplift after this lands in m-c.
Flags: needinfo?(giles)
Comment 17•9 years ago
|
||
Ni myself to remember to handle those files properly
Flags: needinfo?(jyavenard)
Comment 18•9 years ago
|
||
This is clearly incorrectly marked as avc1 when it is avc3) I can get it to play by ignoring the avcC atom and assuming a NAL size of 4 bytes. The apple decoder choke on the frame 21s in.
This is a stream marked as avc1 when it's clearly avc3. The IEF 14496-15 standard clearly shows that an avcC atom is at least 7 bytes long.
so won't bother changing to play it
Flags: needinfo?(jyavenard)
Reporter | ||
Comment 19•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•9 years ago
|
Group: media-core-security → core-security-release
Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8658367 [details] [diff] [review]
Reject mp4 streams with short avc chunks
Approval Request Comment
[Feature/regressing bug #]: HTML5 video playback
[User impact if declined]: Malicious or invalid files can crash the browser tab.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Risk is very low. We reject invalid files we previously tried to play. No affect on valid files.
[String/UUID change made/needed]: None.
Flags: needinfo?(giles)
Attachment #8658367 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8658367 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter | ||
Comment 21•9 years ago
|
||
Comment 22•9 years ago
|
||
This caused regression with AVC3 (all BBC stream), they now don't play
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+]
Blocks: 1181233
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•