Closed Bug 1329126 Opened 5 years ago Closed 5 years ago

Handle OOM in rust parser

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

Attachments

(1 file)

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
Blocks: 1302027
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.
(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
Flags: needinfo?(giles)
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.
Flags: needinfo?(giles)
Assignee: nobody → ayang
Comment on attachment 8853306 [details]
Bug 1329126 - update rust mp4 parser for preventing buffer overflow.

https://reviewboard.mozilla.org/r/125386/#review127964
Attachment #8853306 - Flags: review?(kinetik) → review+
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f1efaf04b46
update rust mp4 parser for preventing buffer overflow. r=kinetik
https://hg.mozilla.org/mozilla-central/rev/5f1efaf04b46
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.