1202677 - Hit MOZ_CRASH(css::ImageValue not thread-safe) at c:/Users/mozilla/debug-builds/ mozilla-central/layout/style/nsCSSValue.cpp:2475

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: windgb information from this url

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

[Nathan Froyd [:froydnj]]

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.

Comment 2

[Andrew McCreight [:mccr8]]

When I load this page in Nightly, it crashes immediately in libstagefright:
https://crash-stats.mozilla.com/report/index/992c4366-61a1-47e7-9e2b-6e0e32150908

Comment 3

[Andrew McCreight [:mccr8]]

Could you take a look at this jya? Thanks.

Comment 4

[Ralph Giles (:rillian)]

Hey, at least we asserted!

Comment 5

[Ralph Giles (:rillian)]

Reproducible with 42 on Mac as well.

Comment 6

[Ralph Giles (:rillian)]

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.

Comment 7

[Ralph Giles (:rillian)]

Attachment: Reject mp4 streams with short avc chunks

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.

Comment 8

[Ralph Giles (:rillian)]

Attachment: invalid file

The file causing the crash from comment #2 is fairly small, so attaching here for reference.

Comment 9

[Jean-Yves Avenard [:jya]]

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

Comment 10

[Jean-Yves Avenard [:jya]]

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.

Comment 11

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

(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?

Comment 12

[Ralph Giles (:rillian)]

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.

Comment 13

[Ralph Giles (:rillian)]

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?

Comment 14

[Daniel Veditz [: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.

Comment 15

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6d311c5911c057a9333ce82f2869ebe484b540
Bug 1202677 - Reject mp4 streams with short avc chunks. r=gerald

Comment 16

[Ralph Giles (:rillian)]

Thanks, Daniel. Setting a reminder to request aurora uplift after this lands in m-c.

Comment 17

[Jean-Yves Avenard [:jya]]

Ni myself to remember to handle those files properly

Comment 18

[Jean-Yves Avenard [:jya]]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c6d311c5911

Comment 20

[Ralph Giles (:rillian)]

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.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c5cd3b4b145c

Comment 22

[Jean-Yves Avenard [:jya]]

This caused regression with AVC3 (all BBC stream), they now don't play