MP4 file triggers SIGILL and OOM

RESOLVED FIXED in Firefox 57

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: tsmith, Assigned: alfredo)

Tracking

(Blocks: 1 bug, {crash, csectype-oom, testcase})

53 Branch
mozilla57
crash, csectype-oom, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

9 months ago
Created attachment 8895212 [details]
test_case.mp4

I'm marking this as s-s for now since it is triggering a SIGILL.

Thread 61 "MediaPD~oder #1" received signal SIGILL, Illegal instruction.
[Switching to Thread 0x7fffa2daf700 (LWP 4502)]
alloc::oom::imp::oom () at /checkout/src/liballoc/oom.rs:41
41	/checkout/src/liballoc/oom.rs: No such file or directory.

+backtrace full
#0  alloc::oom::imp::oom () at /checkout/src/liballoc/oom.rs:41
No locals.
#1  alloc::oom::oom () at /checkout/src/liballoc/oom.rs:27
No locals.
Dwarf Error: Cannot find DIE at 0x49907e9 referenced from DIE at 0x4a78709 [in module /home/user/workspace/browsers/m-c-1502010358-asan-opt/libxul.so]
+disassemble
Dump of assembler code for function alloc::oom::oom:
   0x00007fffe5d88fc0 <+0>:	push   %rbp
   0x00007fffe5d88fc1 <+1>:	mov    %rsp,%rbp
   0x00007fffe5d88fc4 <+4>:	mov    0x29d6a8d(%rip),%rax        # 0x7fffe875fa58 <_ZN5alloc3oom3imp11OOM_HANDLER17h4618046b35e384fcE>
=> 0x00007fffe5d88fcb <+11>:	ud2    
End of assembler dump.
Flags: in-testsuite?
"liballoc/oom.rs", sounds like the place Rust code would go to die when too much memory has been requested!

Alfredo, I would suspect the Rust MP4 parser for that, would you mind having a look? Thanks.
Flags: needinfo?(ayang)
We probably want this to be a fallible allocation and handle it more gracefully (media/images can be large -- tell the user we can't process it rather than dying), but it's not an exploitable issue.
Group: media-core-security
(Assignee)

Comment 3

8 months ago
Invalid pssh data.
Assignee: nobody → ayang
Flags: needinfo?(ayang)
Priority: -- → P1
(Assignee)

Comment 4

8 months ago
(In reply to Daniel Veditz [:dveditz] from comment #2)
> We probably want this to be a fallible allocation and handle it more
> gracefully (media/images can be large -- tell the user we can't process it
> rather than dying), but it's not an exploitable issue.

Fallible allocation in Rust is under discussing.
https://internals.rust-lang.org/t/notes-interviews-on-fallible-collection-allocations/5619
Comment hidden (mozreview-request)

Comment 7

8 months ago
mozreview-review
Comment on attachment 8902080 [details]
Bug 1388618 - update mp4 rust parser to fix invalid PSSH box.

https://reviewboard.mozilla.org/r/173494/#review178844
Attachment #8902080 - Flags: review?(kinetik) → review+

Comment 8

8 months ago
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a33825413ef
update mp4 rust parser to fix invalid PSSH box. r=kinetik
https://hg.mozilla.org/mozilla-central/rev/0a33825413ef
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
I don't think there's enough user impact here to justify backporting to 56, but feel free to set the status back to affected and nominate it for approval if you feel otherwise. That said, should we land the attached testcase as a crashtest still or is the Rust-only test good enough?
Blocks: 1313556
status-firefox55: --- → wontfix
status-firefox56: --- → wontfix
status-firefox-esr52: --- → unaffected
Flags: needinfo?(ayang)
Flags: in-testsuite?
Flags: in-testsuite+
Version: Trunk → 53 Branch
(Assignee)

Comment 11

8 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> I don't think there's enough user impact here to justify backporting to 56,
> but feel free to set the status back to affected and nominate it for
> approval if you feel otherwise. That said, should we land the attached
> testcase as a crashtest still or is the Rust-only test good enough?

The rust test is part of stream from the attached testcase so it should be enough IMO.
Flags: needinfo?(ayang)
You need to log in before you can comment on or make changes to this bug.