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)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: cbook, Assigned: rillian)
References
()
Details
(Keywords: crash)
Attachments
(5 files)
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
Reporter | ||
Comment 1•8 years ago
|
||
[Tracking Requested - why for this release]: bughunter find
tracking-firefox51:
--- → ?
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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?
Assignee | ||
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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.
Comment 8•8 years ago
|
||
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.
Comment 9•8 years ago
|
||
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.
Comment 10•8 years ago
|
||
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.
Updated•8 years ago
|
Assignee: giles → kinetik
Status: NEW → ASSIGNED
Blocks: 1301293
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.
Assignee | ||
Comment 13•8 years ago
|
||
Thanks for the analysis, Matthew and Gerald.
Reporter | ||
Updated•8 years ago
|
Group: core-security
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8789475 -
Flags: review?(kinetik)
Updated•8 years ago
|
Assignee: kinetik → giles
Comment 15•8 years ago
|
||
Comment on attachment 8789475 [details] [review] Proposed fix r- on the basis of the comments in the PR
Attachment #8789475 -
Flags: review?(kinetik) → review-
Comment 16•8 years ago
|
||
(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.
Updated•8 years ago
|
See Also: → https://github.com/mozilla/mp4parse-rust/pull/24
Assignee | ||
Comment 18•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8789475 -
Flags: review?(kinetik) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•8 years ago
|
||
Forgot to update Cargo.lock...
Comment hidden (mozreview-request) |
Comment 23•8 years ago
|
||
mozreview-review |
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 24•8 years ago
|
||
mozreview-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.
Comment 26•8 years ago
|
||
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
Reporter | ||
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/18f34131ce30 https://hg.mozilla.org/mozilla-central/rev/3a61bfe089b2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•