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

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: 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)

Comment 1

[Gerald Squelart [:gerald] (he/him)]

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.)

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1236639-1.1-Avoid-division-by-zero-in-MP3Demuxer.patch

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.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1236639-1.1-Avoid-division-by-zero-in-MP3Demuxer.patch

Comment 4

[Gerald Squelart [:gerald] (he/him)]

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))

Comment 5

[Eugen Sawin [:esawin]]

(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.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1236639-1.2-Avoid-division-by-zero-in-MP3Demuxer.patch

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.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae30a0fe2953

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ae30a0fe2953

Comment 12

[Loic]

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.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Do we have an idea of the

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Sorry, the volume is not big enough to deserve an uplift. Please ni me if you disagree.

Comment 15

[Calixte Denizet (:calixte)]

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

[Ryan VanderMeulen [:RyanVM]]

Can we land the testcase here as a crashtest?

Comment 17

[Eugen Sawin [:esawin]]

Good idea, moving it into bug 1290178.