1329126 - Handle OOM in rust parser

Status: RESOLVED FIXED

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

Description

[Alfredo Yang (:alfredo)]

Some samples could cause OOM [1] in rust. I'm not sure if there is way to handle this in rust [2].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1206211
[2] https://github.com/rust-lang/rust/issues/27700

Comment 1

[Ralph Giles (:rillian)]

Indeed. Video playback is something that should fail cleanly in low memory conditions, but I don't think there's a way to handle this right now. As you note, rust's support for custom allocation failure handling is still unstable. Eventually we might be able to use something like the std::panic::recover() wrapper to make allocations fallible.

This would be a heavy-weight change for gecko. We previously ran the mp4parse calls on a separate thread and used std::thead::JoinHandle to convert any panic into an Err, but we abandoned this approach. First, spawning a thread can also result in an OOM (for the stack space) so at best we need some threadpool machinery to make this approach reliable.

More importantly, unwinding a panic across the FFI boundary is undefined behaviour. To ensure this doesn't happen we build gecko with panic=abort so any panic is effectively a MOZ_CRASH(). Changing that means giving up the ability to call C++ -> Rust -> C++ which we thought would be a more serious limitation than the OOM issue.

In the meantime we've been relying on manually enforcing arbitrary limitations on variable-sized buffers to try and keep the OOMs small.

Comment 2

[Alfredo Yang (:alfredo)]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #1)
> Indeed. Video playback is something that should fail cleanly in low memory
> conditions, but I don't think there's a way to handle this right now. As you
> note, rust's support for custom allocation failure handling is still
> unstable. Eventually we might be able to use something like the
> std::panic::recover() wrapper to make allocations fallible.
> 
> This would be a heavy-weight change for gecko. We previously ran the
> mp4parse calls on a separate thread and used std::thead::JoinHandle to
> convert any panic into an Err, but we abandoned this approach. First,
> spawning a thread can also result in an OOM (for the stack space) so at best
> we need some threadpool machinery to make this approach reliable.
> 
> More importantly, unwinding a panic across the FFI boundary is undefined
> behaviour. To ensure this doesn't happen we build gecko with panic=abort so
> any panic is effectively a MOZ_CRASH(). Changing that means giving up the
> ability to call C++ -> Rust -> C++ which we thought would be a more serious
> limitation than the OOM issue.
> 
> In the meantime we've been relying on manually enforcing arbitrary
> limitations on variable-sized buffers to try and keep the OOMs small.

The most vulnerable I found now is read_buf [1], should we add size limitation inside this function or check size before calling it like [2]?

[1] https://github.com/mozilla/mp4parse-rust/blob/master/mp4parse/src/lib.rs#L1995
[2] https://github.com/mozilla/mp4parse-rust/blob/master/mp4parse/src/lib.rs#L1513

Comment 3

[Ralph Giles (:rillian)]

I think checking `size` against `BUF_SIZE_LIMIT` inside `read_buf` is a good idea. This will catch some future oom. We can change it later if we need to handle buffers larger than 1M.

I would also check against the box size before calling `read_buf` where possible, like we do in `read_flac_metadata`, since it will usually be a smaller bound. However, `read_pssh` and `read_vpcc` seem to be the only places we currently have where this can help. It doesn't make sense to limit things where the size is a small fixed number, or where it comes from a `read_u8()` call.

Comment 4

[Alfredo Yang (:alfredo)]

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

Comment 5

[Alfredo Yang (:alfredo)]

Attachment: Bug 1329126 - update rust mp4 parser for preventing buffer overflow.

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

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 8853306 [details]
Bug 1329126 - update rust mp4 parser for preventing buffer overflow.

https://reviewboard.mozilla.org/r/125386/#review127964

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f1efaf04b46
update rust mp4 parser for preventing buffer overflow. r=kinetik

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f1efaf04b46