1655846 - Hit MOZ_CRASH(assertion failed: `(left == right)` left: `1024`, right: `0`) at <::std::macros::panic macros>:5

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.avif

When running in a non-debug build this file also triggers high memory usage (triggering an OOM while fuzzing)

Hit MOZ_CRASH(assertion failed: (left == right) left: 1024, right: 0) at <::std::macros::panic macros>:5

#0 0x7f3202230f24 in AnnotateMozCrashReason /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:42:19
#1 0x7f3202230f24 in MOZ_Crash /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:331:3
#2 0x7f3202230f24 in RustMozCrash src/mozglue/static/rust/wrappers.cpp:17:3
#3 0x7f3202230ed4 in mozglue_static::panic_hook::h718309d1c883b225 src/mozglue/static/rust/lib.rs:89:8
#4 0x7f32022307cb in core::ops::function::Fn::call::hff608039b849de82 /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libcore/ops/function.rs:72:4
#5 0x7f32035fa8b4 in std::panicking::rust_panic_with_hook::hb976084785e50594 /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/panicking.rs:474:16
#6 0x7f32035fa3ca in rust_begin_unwind /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/panicking.rs:378:4
#7 0x7f32035fa33a in std::panicking::begin_panic_fmt::h82e7fee729e9f5fb /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/panicking.rs:332:4
#8 0x7f32021c04ca in mp4parse::read_avif_meta::habd5cd318e2ce234 src/third_party/rust/mp4parse/src/lib.rs
#9 0x7f32021ecae0 in mp4parse::read_avif::hd1fa7ea5272c95c6 src/third_party/rust/mp4parse/src/lib.rs:1249:39
#10 0x7f32021ecae0 in _$LT$mp4parse_capi..Mp4parseAvifParser$u20$as$u20$mp4parse_capi..ContextParser$GT$::read::hef3221dcc9a82af9 src/third_party/rust/mp4parse_capi/src/lib.rs:377:8
#11 0x7f32021ecae0 in mp4parse_capi::mp4parse_new_common_safe::hd57d8a158ec0d232 src/third_party/rust/mp4parse_capi/src/lib.rs:482:4
#12 0x7f32021ecae0 in mp4parse_capi::mp4parse_new_common::h1c5dc3b269492dbc src/third_party/rust/mp4parse_capi/src/lib.rs:467:14
#13 0x7f32021ecae0 in mp4parse_avif_new src/third_party/rust/mp4parse_capi/src/lib.rs:451:4
#14 0x7f31fbf7f498 in mozilla::image::nsAVIFDecoder::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*) src/image/decoders/nsAVIFDecoder.cpp:379:29
#15 0x7f31fbebe7f8 in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) src/image/Decoder.cpp:172:19
#16 0x7f31fbed5ae7 in mozilla::image::MetadataDecodingTask::Run() src/image/IDecodingTask.cpp:150:34
#17 0x7f31fbee0046 in mozilla::image::DecodePoolWorker::Run() src/image/DecodePool.cpp:276:23
#18 0x7f31fa2758f9 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1234:14
#19 0x7f31fa27b41a in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:513:10
#20 0x7f31fab8875c in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:332:5
#21 0x7f31faaf8813 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:334:10
#22 0x7f31faaf872d in RunHandler src/ipc/chromium/src/base/message_loop.cc:327:3
#23 0x7f31faaf872d in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:309:3
#24 0x7f31fa271c8a in nsThread::ThreadFunc(void*) src/xpcom/threads/nsThread.cpp:447:10
#25 0x7f320e93653b in _pt_root src/nsprpub/pr/src/pthreads/ptthread.c:201:5
#26 0x7f320f6a8608 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x9608)
#27 0x7f320f271102 in clone /build/glibc-YYA7BZ/glibc-2.31/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Comment 1

[Jon Bauman [:jbauman:]]

I see the issue. Good catch, I'll have a fix for this soon.

Comment 2

[Timothy Nikkel (:tnikkel)]

Assuming this is only hit with the avif pref flipped this is probably an S4.

Comment 3

[Jon Bauman [:jbauman:]]

I've locally tested a fix for this and will be able to merge it in after https://github.com/mozilla/mp4parse-rust/pull/228 lands. That will just change the debug assert to an error so that this can be handled more cleanly (and the memory freed). Unfortunately it won't prevent an input like this from allocating an unnecessarily large buffer when the file contains invalid information with regard to the size of different sections. We may be able to mitigate some of those conditions when a box's header indicates a size larger than the box containing it, but we could just as easily have invalid size data in top-level boxes, so I think that would add complexity with fairly limited utility. If any of you think if would be worthwhile, let's discuss the cost/benefit further.

Comment 4

[Tyson Smith [:tsmith]]

Adding the ability to avoid OOMs while fuzzing would be very helpful. I don't know if that needs to be added to mp4parse-rust or if it could be added to the browser and wrapped in #ifdef FUZZING.

Comment 5

[Jon Bauman [:jbauman:]]

I think the issue with the OOMs while fuzzing were related to the fact that the assertion failure prevented the memory that was allocated on the rust side from being freed. In the test case, the iloc box indicates its size is FFFB 001E and so tries to allocate ~4GB. This allocation is designed to be fallible, so if that got an OOM, I don't believe it should interfere with fuzzing, since the error is handled gracefully. If we find that with the fix it's still causing issues with fuzzing, we can look at ways to mitigate it.

Comment 6

[Timothy Nikkel (:tnikkel)]

Imagelib has limits on the size of images and other allocations, so if further mitigations are needed an option is to introduce some limits to the allocations in this code since it will need to produce a result that imagelib won't reject anyways. Hopefully that wouldn't be too hard to do.

Comment 7

[Jon Bauman [:jbauman:]]

The rust-side fix has merged on the GitHub side and bug 1656616 will update to use that new code shortly. I'll keep this open to add a test for this case in the gtest code, but as soon as the rust code is updated, I believe the fuzzing should be unblocked. In my local testing with the rust fix I see no asserts in either debug or release. Additionally (at least on macOS) the large allocation succeeds, but is very quickly freed, so a spike in the memory usage of the content process is observable, but just barely.

Comment 8

[Jon Bauman [:jbauman:]]

(In reply to Timothy Nikkel (:tnikkel) from comment #6)

Imagelib has limits on the size of images and other allocations, so if further mitigations are needed an option is to introduce some limits to the allocations in this code since it will need to produce a result that imagelib won't reject anyways. Hopefully that wouldn't be too hard to do.

In this case the allocation is purely on the rust side of things, so I don't think it should be affected by the ImageLib limits. Since this file is invalid, we're not going to get to the decoding stage anyhow. If there were a valid file this large I'd expect we'd run into failures when trying to allocate buffers in ImageLib, but that seems like an orthogonal issue to me. Let me know if you disagree.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Jon Bauman [:jbauman:] from comment #8)

(In reply to Timothy Nikkel (:tnikkel) from comment #6)

Imagelib has limits on the size of images and other allocations, so if further mitigations are needed an option is to introduce some limits to the allocations in this code since it will need to produce a result that imagelib won't reject anyways. Hopefully that wouldn't be too hard to do.

In this case the allocation is purely on the rust side of things, so I don't think it should be affected by the ImageLib limits. Since this file is invalid, we're not going to get to the decoding stage anyhow. If there were a valid file this large I'd expect we'd run into failures when trying to allocate buffers in ImageLib, but that seems like an orthogonal issue to me. Let me know if you disagree.

What I meant was that implementing a limit on the rust side wouldn't change whether we accept or reject an image because there are limits on the imagelib side already, so it wouldn't be a behaviour change from Firefox's perspective.

Comment 10

[Jon Bauman [:jbauman:]]

Ah! Thank you for clarifying. That makes a lot of sense.

Comment 11

[Jon Bauman [:jbauman:]]

Attachment: Bug 1655846 - Hit MOZ_CRASH(assertion failed: `(left == right)` left: `1024`, right: `0`). r=tsmith

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3449736e0af5
Hit MOZ_CRASH(assertion failed: `(left == right)` left: `1024`, right: `0`). r=tsmith

Comment 13

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 3449736e0af5 (bug 1655846) for GTest failures on Android

Backout: https://hg.mozilla.org/integration/autoland/rev/521f35a76e7c10dbc805ff25a86d40d7bba8d1aa

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3449736e0af51820ac182ceede48c80ec82c15f8

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312111293&repo=autoland&lineNumber=22601

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/ec2d72877b7b
Backed out changeset 3449736e0af5 for GTest failures on Android

Comment 15

[Cristina Coroiu [:ccoroiu]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/ec2d72877b7b

Comment 16

[Jon Bauman [:jbauman:]]

It looks like something android-specific is going on here. My guess is that it has do do with the fallible allocation code. I've managed to reproduce it with an x86_64 emulator running on macOS, but am having difficulty figuring out how to get logging output from running gtest in the emulator to track down where things are going awry.

Comment 17

[Jon Bauman [:jbauman:]]

After investigating this what I've determined is that (at least in our emulated Android 7, x86_64 environment) even though fallible allocation hanling is enabled, this test successfully allocates 4,294,639,634 bytes, and the graceful error-handling of allocation failures doesn't get activated. However, because the system is so low on memory, things fall over essentially immediately afterward and the test doesn't even get a chance to complete.

Two options occur to me:

1. Skip this test on android
2. Add logic to limit the maximum allocation mp4parse-rust will even attempt based on SurfaceCache::MaximumCapacity()

Option 2 would be a good approach if this failure mode turns out to be a problem in practice, but it would be somewhat awkward to integrate into mp4parse-rust as it currently exists, so for now I'm going to pursue skipping this test only on android.

Comment 18

[Jon Bauman [:jbauman:]]

Attachment: Bug 1655846 - Hit MOZ_CRASH(assertion failed: `(left == right)` left: `1024`, right: `0`). r=tnikkel

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b0795cc0cfb
Hit MOZ_CRASH(assertion failed: `(left == right)` left: `1024`, right: `0`). r=tnikkel

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/4b0795cc0cfb