Closed
Bug 1236639
Opened 5 years ago
Closed 5 years ago
SIGFPE in [@mozilla::mp3::MP3TrackDemuxer::OffsetFromFrameIndex]
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: tsmith, Assigned: esawin)
References
(Blocks 1 open bug)
Details
(Keywords: crash, regression, testcase)
Crash Data
Attachments
(2 files, 2 obsolete files)
1.05 KB,
audio/mpeg
|
Details | |
3.36 KB,
patch
|
gerald
:
review+
|
Details | Diff | Splinter Review |
[50190] ###!!! ABORT: Divide by zero: file /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsSigHandlers.cpp, line 156 ==50190==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x00000048bc1e sp 0x7fcc0ff11320 bp 0x7fcc0ff11330 T84) #0 0x48bc1d in mozalloc_abort(char const*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/memory/mozalloc/mozalloc_abort.cpp:33 #1 0x7fcc4ef5dca5 in Abort(char const*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/base/nsDebugImpl.cpp:452 #2 0x7fcc4ef5d96e in NS_DebugBreak /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/base/nsDebugImpl.cpp:404 #3 0x7fcc56b36519 in fpehandler(int, siginfo*, void*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsSigHandlers.cpp:156 #4 0x7fcc68d6633f (/lib/x86_64-linux-gnu/libpthread.so.0+0x1033f) #5 0x7fcc53a8efbb in mozilla::mp3::MP3TrackDemuxer::OffsetFromFrameIndex(long) const /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/MP3Demuxer.cpp:537 #6 0x7fcc53a89578 in mozilla::mp3::MP3TrackDemuxer::FastSeek(mozilla::media::TimeUnit const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/MP3Demuxer.cpp:216 #7 0x7fcc53a88314 in mozilla::mp3::MP3TrackDemuxer::Init() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/MP3Demuxer.cpp:132 #8 0x7fcc53a88a1a in mozilla::mp3::MP3Demuxer::Init() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/MP3Demuxer.cpp:52 #9 0x7fcc53b05838 in mozilla::MediaFormatReader::AsyncReadMetadata() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/MediaFormatReader.cpp:258 #10 0x7fcc53b80049 in get /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/MozPromise.h:873 #11 0x7fcc53b80049 in operator-> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/MozPromise.h:898 #12 0x7fcc53b80049 in mozilla::detail::ProxyRunnable<mozilla::MozPromise<RefPtr<mozilla::MetadataHolder>, mozilla::ReadMetadataFailureReason, true>, mozilla::MediaDecoderReader>::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/MozPromise.h:916 #13 0x7fcc4f09e81a in mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/TaskDispatcher.h:180 #14 0x7fcc4f07e8ac in mozilla::TaskQueue::Runner::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/TaskQueue.cpp:170 #15 0x7fcc4f0939ec in nsThreadPool::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThreadPool.cpp:221 #16 0x7fcc4f093f8c in non-virtual thunk to nsThreadPool::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/xpcom/threads/Unified_cpp_xpcom_threads0.cpp:235 #17 0x7fcc4f08d264 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:989 #18 0x7fcc4f106e5a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297 #19 0x7fcc4fa3562f in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessagePump.cpp:326 #20 0x7fcc4f9a1bac in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234 #21 0x7fcc4f9a1bac in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227 #22 0x7fcc4f9a1bac in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201 #23 0x7fcc4f088db0 in nsThread::ThreadFunc(void*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:401 #24 0x7fcc658364b5 in _pt_root /builds/slave/m-cen-l64-asan-000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212 #25 0x7fcc68d5e181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181) #26 0x7fcc67e5f47c (/lib/x86_64-linux-gnu/libc.so.6+0xfa47c)
Comment 1•5 years ago
|
||
Quick analysis: The Xing header in the first frame indicates that there are 0 frames! This number is then used in MP3TrackDemuxer::OffsetFromFrameIndex, which causes a div-by-zero. I'm guessing that either this file should be rejected, or this 0 field could be ignored. Eugen, I'm told you're the MP3 expert! So you should know best what to do with it, thanks. If you don't have the time, feel free to reassign it to me (in which case please let me know which option you think we should take.)
Assignee: nobody → esawin
Assignee | ||
Comment 2•5 years ago
|
||
We must have introduced this error when switching to Maybe<> optionals. The old code was checking for zero values, while the new code only checks for isSome(), expecting non-zero value. There are two ways to fix this, reset the optionals on zero values during parsing or add explicit zero checks. Changing optional semantics seems wrong to me (the XING header does contain the field after all) so I would prefer adding the original zero checks back in. There is no need to reject the file, we can simply not rely on its meta data, which the code should handle in any case.
Attachment #8705335 -
Flags: review?(jyavenard)
Assignee | ||
Comment 3•5 years ago
|
||
Attachment #8705335 -
Attachment is obsolete: true
Attachment #8705335 -
Flags: review?(jyavenard)
Attachment #8705336 -
Flags: review?(jyavenard)
Assignee | ||
Updated•5 years ago
|
Attachment #8705336 -
Flags: review?(jyavenard) → review?(gsquelart)
Comment 4•5 years ago
|
||
Comment on attachment 8705336 [details] [diff] [review] 0001-Bug-1236639-1.1-Avoid-division-by-zero-in-MP3Demuxer.patch Review of attachment 8705336 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for working on this so quickly. r+, assuming you consider the following concerns: Please update the patch comment's 'r=jya' to 'r=gerald' (to assign the correct blame of course!). ::: dom/media/MP3Demuxer.cpp @@ +534,4 @@ > int64_t offset = 0; > const auto& vbr = mParser.VBRInfo(); > > + if (vbr.NumBytes() && vbr.NumAudioFrames() && vbr.NumAudioFrames().value()) { You were saying that "the old code was checking for zero values, while the new code only checks for isSome(), expecting non-zero value." So shouldn't you test NumBytes for 0 as well? It's not as crashy as NumAudioFrames of course, but it would be good to deal with it now if it makes sense. @@ +550,4 @@ > int64_t frameIndex = 0; > const auto& vbr = mParser.VBRInfo(); > > + if (vbr.NumBytes() && vbr.NumAudioFrames() && vbr.NumBytes().value()) { Same as above, checking NumAudioFrames!=0 if it makes sense. Also I've found these other potential div-by-0: - In Duration(): numFrames = (streamLen - mFirstFrameOffset) / AverageFrameLength(); - In AverageFrameLength(): return static_cast<double>(vbr.NumBytes().value()) / (vbr.NumAudioFrames().value() + 1); (If NumAudioFrames is uint32_t(-1))
Attachment #8705336 -
Flags: review?(gsquelart) → review+
Assignee | ||
Comment 5•5 years ago
|
||
(In reply to Gerald Squelart [:gerald] from comment #4) > Please update the patch comment's 'r=jya' to 'r=gerald' (to assign the > correct blame of course!). I've already done it locally, it was more of a reflex of mine to assign jya. > You were saying that "the old code was checking for zero values, while the > new code only checks for isSome(), expecting non-zero value." > So shouldn't you test NumBytes for 0 as well? > It's not as crashy as NumAudioFrames of course, but it would be good to deal > with it now if it makes sense. That makes me think we should have an overall "sanity check" for XING headers - similar to the other headers' IsValid() - to simplify this checks and reduce redundancy. > Also I've found these other potential div-by-0: > - In Duration(): > numFrames = (streamLen - mFirstFrameOffset) / AverageFrameLength(); > > - In AverageFrameLength(): > return static_cast<double>(vbr.NumBytes().value()) / > (vbr.NumAudioFrames().value() + 1); > (If NumAudioFrames is uint32_t(-1)) Good catch, thanks.
Assignee | ||
Comment 6•5 years ago
|
||
Addressed the comments and added VBRHeader::IsValid, which ignores (commented out for documentation) the scale field's range because we don't use it in the demuxer.
Attachment #8705336 -
Attachment is obsolete: true
Attachment #8705376 -
Flags: review?(gsquelart)
Updated•5 years ago
|
Attachment #8705376 -
Flags: review?(gsquelart) → review+
Updated•5 years ago
|
Priority: -- → P1
Comment 8•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae30a0fe2953
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•5 years ago
|
Comment 12•5 years ago
|
||
Let's track for ESR 45 too, because some MP3 files crash Firefox (this bug) and have broken playback (bug 1263334), and both regressions have been introduced in 45.
status-firefox45:
--- → affected
status-firefox-esr45:
--- → affected
tracking-firefox-esr45:
--- → ?
Comment 13•5 years ago
|
||
Do we have an idea of the
Crash Signature: [@ mozilla::mp3::MP3TrackDemuxer::OffsetFromFrameIndex]
[@ mozilla::mp3::MP3TrackDemuxer::OffsetFromFrameIndex const]
Comment 14•5 years ago
|
||
Sorry, the volume is not big enough to deserve an uplift. Please ni me if you disagree.
Comment 15•5 years ago
|
||
The signature _alldiv (bug 1260779) increased by ~300% on the last 3 days. It represents 1436 crashes in 45.0.2 vs 47 crashes in 45.0.1 (#52 in topcrash)
Comment 16•5 years ago
|
||
Can we land the testcase here as a crashtest?
Flags: needinfo?(esawin)
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•