Divide-by-zero through [@ mozilla::SkeletonState::DecodeIndex] in dom/media/ogg/OggCodecState.cpp

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: decoder, Assigned: jya)

Tracking

(Blocks 1 bug)

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

a year ago
The Ogg DecodeIndex function seems to suffer from divide-by-zero here:

https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/dom/media/ogg/OggCodecState.cpp#1566

Even though timeDenom is checked a few lines before the call to SaferMultDiv, it is downcasted for that call (from int64_t to uint32_t) which can cause the denominator to end up as 0:


[16321, MediaPlayback #1] ###!!! ABORT: Divide by zero: file toolkit/xre/nsSigHandlers.cpp, line 155
ASAN:DEADLYSIGNAL
=================================================================
==16321==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x00000051ada7 bp 0x7f7637ab3350 sp 0x7f7637ab3340 T12815)
==16321==The signal is caused by a WRITE memory access.
==16321==Hint: address points to the zero page.
    #0 0x51ada6 in mozalloc_abort(char const*) memory/mozalloc/mozalloc_abort.cpp:33:5
    #1 0x7f764c58a192 in NS_DebugBreak xpcom/base/nsDebugImpl.cpp
    #2 0x7f765a714e1a in fpehandler(int, siginfo_t*, void*) toolkit/xre/nsSigHandlers.cpp:155:5
    #3 0x7f766fa4038f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1138f)
    #4 0x7f765426495a in mozilla::SaferMultDiv(long, unsigned int, unsigned int) dom/media/VideoUtils.cpp:41:30
    #5 0x7f765489d2f5 in mozilla::SkeletonState::DecodeIndex(ogg_packet*) dom/media/ogg/OggCodecState.cpp:1566:20
    #6 0x7f76548ae97e in mozilla::OggDemuxer::ReadHeaders(mozilla::TrackInfo::TrackType, mozilla::OggCodecState*) dom/media/ogg/OggDemuxer.cpp:328:18
    #7 0x7f76548b0027 in mozilla::OggDemuxer::SetupTargetSkeleton() dom/media/ogg/OggDemuxer.cpp:380:16
    #8 0x7f76548abca3 in mozilla::OggDemuxer::ReadMetadata() dom/media/ogg/OggDemuxer.cpp:558:3
    #9 0x7f76548a9d34 in mozilla::OggDemuxer::Init() dom/media/ogg/OggDemuxer.cpp:215:7
    #10 0x7f76542745d9 in mozilla::BenchmarkPlayback::DemuxSamples() dom/media/Benchmark.cpp:176:13


The upcoming patch validates that timeDenom is within uint32_t range.
Comment hidden (mozreview-request)
Assignee

Comment 2

a year ago
mozreview-review
Comment on attachment 8960347 [details]
Bug 1447124 - Limit Ogg time denominator to UINT32_MAX.

https://reviewboard.mozilla.org/r/229100/#review234894

::: dom/media/ogg/OggCodecState.cpp:1564
(Diff revision 1)
>      LOG(LogLevel::Debug, ("Ogg Skeleton Index packet for stream %u has 0 "
>                         "timestamp denominator.", serialno));
>      return (mActive = false);
>    }
>  
> +  if (timeDenom < 0 || timeDenom > UINT32_MAX) {

it worroes me that this could regress in the case timestamps are negative, as I am unsure if those are allowed.
i need to investigate first...
Component: Audio/Video → Audio/Video: Playback
Assignee

Comment 3

a year ago
mozreview-review
Comment on attachment 8960347 [details]
Bug 1447124 - Limit Ogg time denominator to UINT32_MAX.

https://reviewboard.mozilla.org/r/229100/#review236988

::: dom/media/ogg/OggCodecState.cpp:1564
(Diff revision 1)
>      LOG(LogLevel::Debug, ("Ogg Skeleton Index packet for stream %u has 0 "
>                         "timestamp denominator.", serialno));
>      return (mActive = false);
>    }
>  
> +  if (timeDenom < 0 || timeDenom > UINT32_MAX) {

ok..

so a granule pos of -1 is valid and indicates that this page doesn't contain stuff.

so don't test if it's negative.

simply checking that timeDenom is > INT32_MAX will do (not UINT32_MAX)
Assignee

Comment 4

a year ago
The times are 64 bits here, so either we create a SaferMultDiv that takes 64 bits , or modify the current one to use 64 bits, that will do.
Assignee

Comment 5

a year ago
mozreview-review
Comment on attachment 8960347 [details]
Bug 1447124 - Limit Ogg time denominator to UINT32_MAX.

https://reviewboard.mozilla.org/r/229100/#review236990
Attachment #8960347 - Flags: review?(jyavenard) → review-
Assignee

Updated

a year ago
Assignee: nobody → jyavenard
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8962652 [details]
Bug 1447124 - Use int64_t for SaferMultDiv.

https://reviewboard.mozilla.org/r/231512/#review236994

::: dom/media/VideoUtils.cpp:7
(Diff revision 1)
> +#include <functional>
> +#include <stdint.h>

Why did you move these lines?

::: dom/media/VideoUtils.cpp:43
(Diff revision 1)
>    int64_t major = aValue / aDiv;
>    int64_t remainder = aValue % aDiv;
>    return CheckedInt64(remainder) * aMul / aDiv + CheckedInt64(major) * aMul;

So many little signed<->unsigned conversions happening in there! :-)

Could you please explicitly convert aMul and aDiv to signed numbers? Or take then as (positive) signed parameters.
Attachment #8962652 - Flags: review?(gsquelart) → review+
Assignee

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8962652 [details]
Bug 1447124 - Use int64_t for SaferMultDiv.

https://reviewboard.mozilla.org/r/231512/#review236994

> Why did you move these lines?

Because our coding style says so :)
Comment hidden (mozreview-request)

Comment 10

a year ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aff1318be50a
Use int64_t for SaferMultDiv. r=gerald

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aff1318be50a
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.