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

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

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 1

[Christian Holler (:decoder)]

Attachment: Bug 1447124 - Limit Ogg time denominator to UINT32_MAX.

Review commit: https://reviewboard.mozilla.org/r/229100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229100/

Comment 2

[Jean-Yves Avenard [:jya]]

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

Comment 3

[Jean-Yves Avenard [:jya]]

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)

Comment 4

[Jean-Yves Avenard [:jya]]

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.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8960347 [details]
Bug 1447124 - Limit Ogg time denominator to UINT32_MAX.

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

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1447124 - Use int64_t for SaferMultDiv.

This prevent potential division by zero should the cast on the argument cause an overflow.
We still limit the mul and div arguments to INT64_MAX.

Review commit: https://reviewboard.mozilla.org/r/231512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231512/

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 8

[Jean-Yves Avenard [:jya]]

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 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8962652 [details]
Bug 1447124 - Use int64_t for SaferMultDiv.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231512/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aff1318be50a
Use int64_t for SaferMultDiv. r=gerald

Comment 11

[Eliza Balazs [:ebalazs_]]

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