Closed Bug 1202677 Opened 4 years ago Closed 4 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)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- wontfix
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: cbook, Assigned: rillian)

References

(Blocks 1 open bug, )

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main42+])

Crash Data

Attachments

(3 files)

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.
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)
Group: core-security → media-core-security
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)
Could you take a look at this jya? Thanks.
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&) ]
Hey, at least we asserted!
Flags: needinfo?(giles)
Reproducible with 42 on Mac as well.
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.
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)
Attached video invalid file
The file causing the crash from comment #2 is fairly small, so attaching here for reference.
Flags: needinfo?(jyavenard)
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+
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)
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)
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)
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)
Thanks, Daniel. Setting a reminder to request aurora uplift after this lands in m-c.
Flags: needinfo?(giles)
Ni myself to remember to handle those files properly
Flags: needinfo?(jyavenard)
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)
https://hg.mozilla.org/mozilla-central/rev/4c6d311c5911
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Group: media-core-security → core-security-release
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?
Attachment #8658367 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This caused regression with AVC3 (all BBC stream), they now don't play
Blocks: 1205179
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.