Closed Bug 1301065 Opened 8 years ago Closed 8 years ago

Crash at _rust_start_panic + 0x0 imalloc rust_begin_unwind + 0x3dc ZN3std9panicking15begin_panic_fmt17haf08a9a70a097ee1E + 0x90 rust_begin_unwind + 0x1d

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 + fixed

People

(Reporter: cbook, Assigned: rillian)

References

()

Details

(Keywords: crash)

Attachments

(5 files)

Attached file stack
Found by bughunter and reproduced on a latest win debug tinderbox build on win7

Steps to reproduce:
-> Load https://www.periscope.tv/couchmode?mode=couch
--> Crash

signature is _rust_start_panic + 0x0 ZN3std5panic13resume_unwind17h7ff3a727fcedda61E + 0x1c ZN3std9panicking20rust_panic_with_hook17h5dd7da6bb3d06020E + 0x1e9 rust_begin_unwind + 0x3dc ZN3std9panicking15begin_panic_fmt17haf08a9a70a097ee1E + 0x90

and marked as sec issue on bughunter - trunk only problem/regression it seems
[Tracking Requested - why for this release]:
bughunter find
 7  xul.dll!ZN4core9panicking5panic17h9d5bd65bbb401959E + 0x61
    rbp = 0x000000000754efe0   rsp = 0x000000000754f240
    rip = 0x000007fed7da81e1
    Found by: call frame info
 8  xul.dll!track_time_to_ms [lib.rs:24763f58772d : 1363 + 0xc]
    rbp = 0x000000000754efe0   rsp = 0x000000000754f2d0
    rip = 0x000007fed7d906f4
    Found by: call frame info
 9  xul.dll!mp4parse_get_track_info [capi.rs:24763f58772d : 327 + 0x53]
    rbp = 0x000000000754efe0   rsp = 0x000000000754f300
    rip = 0x000007fed7da2ae6
    Found by: call frame info

Probably one of the assertions there.
Flags: needinfo?(giles)
Hmm. Those assertions aren't new; they landed 9 months ago in bug 1229615, and bug 1229612. What has changed recently is the source file location in bug 1300219. Or it could be unrelated changes or data we haven't seen before. In any case, an assert should not be security critical.

How reliable is the reproduction? I cannot reproduce with today's nightly 51.0a1 (2016-09-07) on Linux, Mac or Win7. The url given seems to go to random videos which are streamed as mpeg-ts segments and are remuxed and stitched together in javascript to feed the MediaSource Extensions api. As such it's tricky to get test data for deterministic reproduction.
Assignee: nobody → giles
Flags: needinfo?(giles) → needinfo?(cbook)
I was able to reproduce in a Win7 vm with the latest m-c build https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win32-debug/1473266776/firefox-51.0a1.en-US.win32.installer.exe periscope redirected to https://www.periscope.tv/w/1vOxwReyOZWJB?mode=couch

Since it seems to be a new regression the most likely candidate is the toolchain bump to rust 1.11.0 in bug 1296403. Does someone more experienced with assembly want to look at the object code and see if there's something weird going on?
Reproducible on mac with a debug build from the same m-c revision.

* thread #68: tid = 0x61485, 0x0000000100008f41 libmozglue.dylib`mozalloc_abort(char const*) + 81, name = 'MediaPl~back #1', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100008f41 libmozglue.dylib`mozalloc_abort(char const*) + 81
    frame #1: 0x0000000100008f70 libmozglue.dylib`abort + 16
    frame #2: 0x0000000107c64b99 XUL`__rust_start_panic + 9
    frame #3: 0x0000000107c40040 XUL`___lldb_unnamed_function243772$$XUL + 16
    frame #4: 0x0000000107c2912d XUL`std::panicking::rust_panic_with_hook::hd7b83626099d3416 + 413
    frame #5: 0x0000000107c641f7 XUL`___lldb_unnamed_function243875$$XUL + 103
    frame #6: 0x0000000107c2af39 XUL`std::panicking::begin_panic_fmt::h30280d4dd3f149f5 + 169
    frame #7: 0x0000000107c63e50 XUL`rust_begin_unwind + 32
    frame #8: 0x0000000107c70201 XUL`core::panicking::panic_fmt::h2d3cc8234dde51b4 + 129
    frame #9: 0x0000000107c70dfd XUL`core::panicking::panic::heeca72c448510af4 + 109
    frame #10: 0x0000000107c1b1c6 XUL`___lldb_unnamed_function243216$$XUL + 102
    frame #11: 0x0000000107c1b3dc XUL`mp4parse_get_track_info + 524
    frame #12: 0x0000000102d18ebe XUL`___lldb_unnamed_function152$$XUL + 238
    [...]
    frame #26: 0x0000000102c4dcc0 libnss3.dylib`___lldb_unnamed_function3621$$libnss3.dylib + 288
Also reproducible on Linux, so I guess it's reasonably so. :)

In this backtrace track_time_to_ms() is called with track_duration and track_scale, and dies on the line after the asserts when the return value is calculated, if the line numbers are to be believed.
 
[...panic=abort...]
#10 0x00007fffe7d0fff9 in  () at /home/giles/mozilla/obj-eclipse/dist/bin/libxul.so
#11 0x00007fffe7cb9c46 in mp4parse_capi::track_time_to_ms (time=..., scale=<error reading variable: access outside bounds of object referenced via synthetic pointer>)
    at /home/giles/mozilla/firefox/media/libstagefright/binding/mp4parse_capi/src/lib.rs:289
#12 0x00007fffe7cb9e23 in mp4parse_capi::mp4parse_get_track_info (parser=<optimized out>, track_index=<optimized out>, info=0x7fffb076b690)
    at /home/giles/mozilla/firefox/media/libstagefright/binding/mp4parse_capi/src/lib.rs:342
#13 0x00007fffe15a9d2d in mp4_demuxer::MP4MetadataRust::GetNumberTracks(mozilla::TrackInfo::TrackType) const (this=0x7fffa056c480, aType=mozilla::TrackInfo::kAudioTrack)
    at /home/giles/mozilla/firefox/media/libstagefright/binding/MP4Metadata.cpp:605
[...]
#18 0x00007fffe485648d in mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::TrackBuffersManager, void (mozilla::TrackBuffersManager::*)()>(mozilla::TrackBuffersManager*, void (mozilla::TrackBuffersManager::*)(), mozilla::Tuple<>&, mozilla::IndexSequence<>) (o=0x7fff9d058000, m=(void (mozilla::TrackBuffersManager::*)(mozilla::TrackBuffersManager * const)) 0x7fffe482cacc <mozilla::TrackBuffersManager::SegmentParserLoop()>, args=...) at /home/giles/mozilla/obj-eclipse/dist/include/nsThreadUtils.h:729
Flags: needinfo?(cbook)
I'm out of time today and am traveling tomorrow. I would like to try:

1) Push a backout of the rust 1.11 update to try and see if that corrects the regression. If so, that's the fastest way to unbreak nightly.

2) Instrument a build to dump the mp4 buffer we're given which crashes and see if it reproduces on in a stand-alone test of just the mp4parse code, and otherwise try to isolate the fault to code, our build, or the compiler output.

Alfredo or Matthew, feel free to do any of these today that you can, otherwise I'll start on it tomorrow morning and report when I have network access.
Attached video bug1301065.mp4
With the attached buffer fragment, track_time_to_ms is called with a time of 4413527634807900 and a scale of 44100.  The multiplication of 4413527634807900 by 1000000 overflows, which is detected by Rust and triggers a panic.
track_time_to_ms (and media_time_to_ms) are misnamed, implying they return a time in milliseconds when they return a time in microseconds (should be suffixed _to_us), which is what the TrackInfo object we eventually fill out in MP4Metadata expects.

ffprobe reports the duration as 359928:51:51.00
MP4Box reports the duration as 3167 Years 2 Days, 05:41:59.000 (~6.5 years off a simple calculation, but maybe accounts calendar changes, etc.?)
Chrome reports the duration as 100079991719s (although the UI displays it as a large negative number)
I calculate it as 100079991719000000us -> 100079991719s


Verifying we're parsing the raw data correctly, xxd dump:

00000130: 0000 0000 0000 0144 6d64 6961 0000 002c  .......Dmdia...,
                                        ^^^^^^^^^
                                        44 bytes
00000140: 6d64 6864 0100 0000 0000 0000 0000 0000  mdhd............
                    ^^^^^^^^^
                    version 1 
00000150: 0000 0000 0000 0000 0000 ac44 000f ae14  ...........D....
                              ^^^^^^^^^ ^^^^^^^^^
                              scale=44100
00000160: 7ae1 0c5c 55c4 0000 0000 002d 6864 6c72  z..\U......-hdlr
          ^^^^^^^^^
          duration=4413527634807900

We could fix this particular case by changing the order of the conversion to |time.0 / scale.0 * 1000000|, but there are other values where we'd still overflow and panic.  The correct way to fix this is to check for overflow and treat it as an error rather than panicking.
CCing Gerald, since this also affects libstagefright.  The track's duration in MP4MetadataStagefright is reported as 107841296855264, which is the result of an overflow.  The calculation at https://dxr.mozilla.org/mozilla-central/rev/ab70808cd4b6c6ad9a57a9f71cfa495fcea0aecd/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#1095 is the same as the Rust version, minus the implicit overflow checks.
Assignee: giles → kinetik
Status: NEW → ASSIGNED
Thank you Matthew, I've opened bug 1301293 to fix the stagefright side. -- Though feel free to fix it here if you like!
There is another similar potential overflow in the duration calculation when parsing 'mehd': https://dxr.mozilla.org/mozilla-central/rev/ab70808cd4b6c6ad9a57a9f71cfa495fcea0aecd/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#1803
You should check if you have an equivalent issue in the rust parser.

Also I'm not sure it's a security issue, as it "just" sets a wrong duration, I don't think that value is used to go read random memory.
Thanks for the analysis, Matthew and Gerald.
Group: core-security
Attached file Proposed fix
Attachment #8789475 - Flags: review?(kinetik)
Assignee: kinetik → giles
Comment on attachment 8789475 [details] [review]
Proposed fix

r- on the basis of the comments in the PR
Attachment #8789475 - Flags: review?(kinetik) → review-
(In reply to Gerald Squelart [:gerald] from comment #12)
> There is another similar potential overflow in the duration calculation when
> parsing 'mehd':
> https://dxr.mozilla.org/mozilla-central/rev/
> ab70808cd4b6c6ad9a57a9f71cfa495fcea0aecd/media/libstagefright/frameworks/av/
> media/libstagefright/MPEG4Extractor.cpp#1803
> You should check if you have an equivalent issue in the rust parser.

Thanks for raising that.  The Rust parser case will handled by the same ultimate fixes to {media,track}_time_to_us that we'll make for the mdhd case, since all conversions should pass through those functions.
Tracking 51+ for this bughunter crash.
Comment on attachment 8789475 [details] [review]
Proposed fix

Updated PR implementing the div-remainder split and returning error on remaining overflow cases as suggested.
Attachment #8789475 - Flags: review- → review?(kinetik)
Attachment #8789475 - Flags: review?(kinetik) → review+
Forgot to update Cargo.lock...
Comment on attachment 8790797 [details]
Bug 1301065 - Bump requested rust mp4parse to v0.5.1.

https://reviewboard.mozilla.org/r/78458/#review77158
Attachment #8790797 - Flags: review?(kinetik) → review+
Comment on attachment 8790798 [details]
Bug 1301065 - Update rust mp4parse to v0.5.1.

https://reviewboard.mozilla.org/r/78460/#review77160
Attachment #8790798 - Flags: review?(kinetik) → review+
Good news everyone: The upcoming gtest in bug 1301293 passes after the patch in attachment 8790798 [details] is applied.
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18f34131ce30
Bump requested rust mp4parse to v0.5.1. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/3a61bfe089b2
Update rust mp4parse to v0.5.1. r=kinetik
https://hg.mozilla.org/mozilla-central/rev/18f34131ce30
https://hg.mozilla.org/mozilla-central/rev/3a61bfe089b2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1308685
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: