Closed Bug 1457359 Opened 2 years ago Closed 2 years ago

The FallibleVec code is doing memory-unsafe things

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: glandium, Assigned: kinetik)

Details

Attachments

(1 file, 1 obsolete file)

Specifically, this code:
https://github.com/mozilla/mp4parse_fallible/blob/master/lib.rs#L67-L69

    let new_ptr = unsafe {
        if old_cap == 0 {
            malloc(new_size_bytes)
        } else {
            realloc(old_ptr as *mut u8, new_size_bytes)
        }
    };

What's wrong here is that on Windows, rust is *not* using malloc/realloc/free, but HeapAlloc/HeapReAlloc/HeapFree. This only works fine because in Firefox, we intercept the Heap* functions, so practically, they are malloc/realloc/free.

But the above is not true in many other situations:
- Firefox builds with --disable-jemalloc, such as ASAN builds.
- Outside Firefox, the default for rust is to use its copy of jemalloc, which may or may not expose its symbols as malloc/realloc. One can also build like we do in Firefox, with the system allocator, in which case rust would use Heap*.

So in those cases, you can end up with realloc called with memory that was initially allocated with another allocator, or when the Vec is dropped, it would try to HeapFree memory that was allocated with malloc/realloc, which would fail pretty badly.
Kinetik: FYI. Would be good if you could double check my priority assessment and see how hard this is to get fixed.
Rank: 25
Flags: needinfo?(kinetik)
Priority: -- → P3
mp4parse doesn't use FallibleVec outside of Gecko.  It's controlled by the mp4parse_fallible feature in Cargo, off by default and enabled in Gecko via https://searchfox.org/mozilla-central/source/media/mp4parse-rust/mp4parse_capi/Cargo.toml#29.  You could argue that it should include a warning that it depends on Gecko's allocator overrides to be safe.

It'd make sense to disable mp4parse_fallible for non-jemalloc Gecko builds to avoid causing issues there.  Is there an easy way to detect that from within a Cargo.toml?

Once Rust's fallible Vec methods are stable, we'll switch to them: https://github.com/mozilla/mp4parse-rust/issues/146
Flags: needinfo?(kinetik)
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attached patch bug1457359_wip.patch (obsolete) — Splinter Review
Something like this, although I need to update the mp4parse-rust Cargo.toml upstream and fix up mp4parse-cargo.patch.
Attachment #8974259 - Flags: feedback?(mh+mozilla)
Attachment #8974259 - Flags: feedback?(mh+mozilla) → feedback+
Attachment #8974259 - Attachment is obsolete: true
Comment on attachment 8974587 [details]
Bug 1457359 - Update mp4parse and disable FallibleVec when jemalloc is disabled.

https://reviewboard.mozilla.org/r/242932/#review248772
Attachment #8974587 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8974587 [details]
Bug 1457359 - Update mp4parse and disable FallibleVec when jemalloc is disabled.

https://reviewboard.mozilla.org/r/242932/#review248990

::: media/mp4parse-rust/mp4parse/src/lib.rs:604
(Diff revision 1)
>          1 => 4 + 4 + 8,
>          _ => 4 + 4,
>      };
> +    let uuid = if name == BoxType::UuidBox {
> +        if size < offset + 16 {
> +            return Err(Error::InvalidData("malformed size for uuid"));

shouldn't we juse ignore the box ?
after all, it's an optional box.

like with the recent edit list change, errors were made warnings ,and invalid box simply ignored
Attachment #8974587 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #6)
> shouldn't we juse ignore the box ?
> after all, it's an optional box.
> 
> like with the recent edit list change, errors were made warnings ,and
> invalid box simply ignored

Sure, can you please review https://github.com/mozilla/mp4parse-rust/pull/151, then I'll merge it and update this patch.
Pushed by mgregan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5fac38dc791
Update mp4parse and disable FallibleVec when jemalloc is disabled.  r=glandium,jya
Backed out changeset b5fac38dc791 (bug 1457359) for build bustage on a CLOSED TREE

Backout: https://hg.mozilla.org/integration/autoland/rev/a7d70f62c8057a74ddcf7d80ed869c8e5b3f1075

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

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=177981493&repo=autoland&lineNumber=7678

[task 2018-05-11T01:12:17.460Z] 01:12:17     INFO -  force-cargo-library-build
[task 2018-05-11T01:12:17.461Z] 01:12:17     INFO -  env   RUSTC_BOOTSTRAP=1 RUSTFLAGS='-C opt-level=2 -C debuginfo=2 ' RUSTC_WRAPPER='/builds/worker/workspace/build/src/sccache2/sccache' CARGO_TARGET_DIR=/builds/worker/workspace/build/src/obj-firefox/toolkit/library RUSTC=/builds/worker/workspace/build/src/rustc/bin/rustc RUSTDOC=/builds/worker/workspace/build/src/rustc/bin/rustdoc MOZ_SRC=/builds/worker/workspace/build/src MOZ_DIST=/builds/worker/workspace/build/src/obj-firefox/dist LIBCLANG_PATH="/builds/worker/workspace/build/src/clang/lib" CLANG_PATH="/builds/worker/workspace/build/src/clang/bin/clang" PKG_CONFIG_ALLOW_CROSS=1 RUST_BACKTRACE=full MOZ_TOPOBJDIR=/builds/worker/workspace/build/src/obj-firefox CARGO_INCREMENTAL=0 MOZ_CARGO_WRAP_LDFLAGS="-lpthread -Wl,-z,noexecstack -Wl,-z,text -Wl,-z,relro -Wl,--build-id -Wl,-rpath-link,/builds/worker/workspace/build/src/obj-firefox/dist/bin -Wl,-rpath-link,/usr/local/lib" MOZ_CARGO_WRAP_LD=" /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/gcc/bin/gcc -m32 -march=pentium-m -std=gnu99" CARGO_TARGET_I686_UNKNOWN_LINUX_GNU_LINKER=/builds/worker/workspace/build/src/build/cargo-linker /builds/worker/workspace/build/src/rustc/bin/cargo rustc  --frozen --manifest-path /builds/worker/workspace/build/src/toolkit/library/gtest/rust/Cargo.toml --verbose --lib --target=i686-unknown-linux-gnu --features "servo bindgen gecko_debug quantum_render cubeb_pulse_rust simd-accel cubeb-remoting moz_memory no-static-ideograph-encoder-tables oom_with_global_alloc" --
[task 2018-05-11T01:12:17.461Z] 01:12:17     INFO -  error: Package `gkrust-gtest v0.1.0 (file:///builds/worker/workspace/build/src/toolkit/library/gtest/rust)` does not have these features: `moz_memory`
[task 2018-05-11T01:12:17.461Z] 01:12:17     INFO -  /builds/worker/workspace/build/src/config/rules.mk:949: recipe for target 'force-cargo-library-build' failed
[task 2018-05-11T01:12:17.461Z] 01:12:17     INFO -  make[4]: *** [force-cargo-library-build] Error 101
[task 2018-05-11T01:12:17.461Z] 01:12:17     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/gtest/rust'
[task 2018-05-11T01:12:17.461Z] 01:12:17     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:73: recipe for target 'toolkit/library/gtest/rust/target' failed
[task 2018-05-11T01:12:17.461Z] 01:12:17     INFO -  make[3]: *** [toolkit/library/gtest/rust/target] Error 2
[task 2018-05-11T01:12:17.462Z] 01:12:17     INFO -  make[3]: *** Waiting for unfinished jobs....
[task 2018-05-11T01:12:17.462Z] 01:12:17     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/i18n'
[task 2018-05-11T01:12:17.462Z] 01:12:17     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/i18n'
[task 2018-05-11T01:12:17.462Z] 01:12:17     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/i18n'
[task 2018-05-11T01:12:17.462Z] 01:12:17     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/i18n'
[task 2018-05-11T01:12:17.660Z] 01:12:17     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/common'
Flags: needinfo?(kinetik)
Ah, forgot to add `moz_memory = ["gkrust-shared/moz_memory"]` to toolkit/library/gtest/rust/Cargo.toml.
Flags: needinfo?(kinetik)
Pushed by mgregan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1591f55d3e32
Update mp4parse and disable FallibleVec when jemalloc is disabled.  r=glandium,jya
https://hg.mozilla.org/mozilla-central/rev/1591f55d3e32
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.