Closed Bug 1236639 Opened 4 years ago Closed 4 years ago

SIGFPE in [@mozilla::mp3::MP3TrackDemuxer::OffsetFromFrameIndex]

Categories

(Core :: Audio/Video: Playback, defect, P1, critical)

45 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- wontfix
firefox46 --- fixed
firefox-esr45 - wontfix

People

(Reporter: tsmith, Assigned: esawin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(2 files, 2 obsolete files)

Attached audio test_case.mp3
[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)
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
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)
Attachment #8705335 - Attachment is obsolete: true
Attachment #8705335 - Flags: review?(jyavenard)
Attachment #8705336 - Flags: review?(jyavenard)
Attachment #8705336 - Flags: review?(jyavenard) → review?(gsquelart)
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+
(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.
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)
Attachment #8705376 - Flags: review?(gsquelart) → review+
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/ae30a0fe2953
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Duplicate of this bug: 1258802
Duplicate of this bug: 1260779
Duplicate of this bug: 1263241
Blocks: 1219178
Keywords: regression
Version: Trunk → 45 Branch
See Also: → 1263334
Blocks: 1263334
See Also: 1263334
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.
Do we have an idea of the
Crash Signature: [@ mozilla::mp3::MP3TrackDemuxer::OffsetFromFrameIndex] [@ mozilla::mp3::MP3TrackDemuxer::OffsetFromFrameIndex const]
Sorry, the volume is not big enough to deserve an uplift. Please ni me if you disagree.
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)
Blocks: grizzly
Can we land the testcase here as a crashtest?
Flags: needinfo?(esawin)
Flags: in-testsuite?
Good idea, moving it into bug 1290178.
Flags: needinfo?(esawin)
You need to log in before you can comment on or make changes to this bug.