Closed Bug 1388618 Opened 3 years ago Closed 3 years ago

MP4 file triggers SIGILL and OOM

Categories

(Core :: Audio/Video: Playback, defect, P1)

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: tsmith, Assigned: ayang)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-oom, testcase)

Attachments

(2 files)

Attached video 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
Invalid pssh data.
Assignee: nobody → ayang
Flags: needinfo?(ayang)
Priority: -- → P1
(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 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+
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
Closed: 3 years ago
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
Flags: needinfo?(ayang)
Flags: in-testsuite?
Flags: in-testsuite+
Version: Trunk → 53 Branch
(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.