1388618 - MP4 file triggers SIGILL and OOM

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: 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.

Comment 1

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

"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.

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Alfredo Yang (:alfredo)]

Invalid pssh data.

Comment 4

[Alfredo Yang (:alfredo)]

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

[Alfredo Yang (:alfredo)]

https://github.com/mozilla/mp4parse-rust/pull/112

Comment 6

[Alfredo Yang (:alfredo)]

Attachment: Bug 1388618 - update mp4 rust parser to fix invalid PSSH box.

Review commit: https://reviewboard.mozilla.org/r/173494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173494/

Comment 7

[Matthew Gregan [:kinetik]]

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

https://reviewboard.mozilla.org/r/173494/#review178844

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a33825413ef
update mp4 rust parser to fix invalid PSSH box. r=kinetik

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0a33825413ef

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 11

[Alfredo Yang (:alfredo)]

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