1384243 - Assertion failure: index == aOffsetInFile % mCacheBlockSize [@ mozilla::MediaResourceIndex::ReadAt]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Assertion failure: index == aOffsetInFile % mCacheBlockSize, at src/dom/media/MediaResource.h:828

#0 0x7fee7bc9b29f in mozilla::MediaResourceIndex::IndexInCache(long) const src/dom/media/MediaResource.h:828:5
#1 0x7fee7bc9b18f in mozilla::MediaResourceIndex::CacheOffsetContaining(long) const src/dom/media/MediaResource.h:836:5
#2 0x7fee7bc99a54 in mozilla::MediaResourceIndex::ReadAt(long, char*, unsigned int, unsigned int*) src/dom/media/MediaResource.cpp:1601:35
#3 0x7fee7bc99886 in mozilla::MediaResourceIndex::Read(char*, unsigned int, unsigned int*) src/dom/media/MediaResource.cpp:1568:17
#4 0x7fee7c0cb7ea in mozilla::webmdemux_read(void*, unsigned long, void*) src/dom/media/webm/WebMDemuxer.cpp:74:29
#5 0x7fee7e076f65 in ne_bare_read_vint src/media/libnestegg/src/nestegg.c:654:7
#6 0x7fee7e06e53c in ne_peek_element src/media/libnestegg/src/nestegg.c:965:7
#7 0x7fee7e073c4d in ne_read_element src/media/libnestegg/src/nestegg.c:988:7
#8 0x7fee7e06fa54 in ne_init_cue_points src/media/libnestegg/src/nestegg.c:1907:9
#9 0x7fee7e070133 in nestegg_track_seek src/media/libnestegg/src/nestegg.c:2304:9
#10 0x7fee7c0d3455 in mozilla::WebMDemuxer::SeekInternal(mozilla::TrackInfo::TrackType, mozilla::media::TimeUnit const&) src/dom/media/webm/WebMDemuxer.cpp:974:11
#11 0x7fee7c0d6853 in mozilla::WebMTrackDemuxer::Reset() src/dom/media/webm/WebMDemuxer.cpp:1222:14
#12 0x7fee7bc4c0ec in mozilla::detail::RunnableFunction<mozilla::MediaFormatReader::DemuxerProxy::Wrapper::Reset()::{lambda()#1}>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:527:5
#13 0x7fee7786a0d5 in mozilla::TaskQueue::Runner::Run() src/xpcom/threads/TaskQueue.cpp:246:12
#14 0x7fee778a8d6e in nsThreadPool::Run() src/xpcom/threads/nsThreadPool.cpp:225:14
#15 0x7fee778a91ec in non-virtual thunk to nsThreadPool::Run() src/xpcom/threads/nsThreadPool.cpp:154:15
#16 0x7fee778a05dc in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1579:14
#17 0x7fee778a6340 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:530:10
#18 0x7fee784127ec in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:369:5
#19 0x7fee7835d647 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:321:10
#20 0x7fee7835d4d9 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:294:3
#21 0x7fee7789768b in nsThread::ThreadFunc(void*) src/xpcom/threads/nsThread.cpp:507:11
#22 0x7fee9320b5ad in _pt_root src/nsprpub/pr/src/pthreads/ptthread.c:216:5
#23 0x7fee968146b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
#24 0x7fee9589d3dc in clone /build/glibc-bfm8X4/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.webm

Comment 2

[Matthew Gregan [:kinetik]]

I think the only libnestegg part of this is the same thing as bug 1379039, which is that it parses a invalid offset from the file and tries to seek to and read from that location.

MediaResourceIndex should probably return an error rather than asserting.

Comment 3

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

Agreed!

Comment 4

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

Attachment: Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

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

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8892284 [details]
Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

https://reviewboard.mozilla.org/r/163218/#review168574

There are other public functions that do reading, like UncachedReadAt(), don't you need to sanitize the input params on them too?

::: dom/media/MediaResource.cpp:1992
(Diff revision 1)
>        break;
>      default:
>        return NS_ERROR_FAILURE;
>    }
>  
> +  if (mOffset < 0) {

I think you meant:

if (aOffset < 0) { return NS_ERR...; }

Otherwise you're not actually setting avoiding setting mOffset to an invalid value, you'd only detect the error on the subsequent entry to this function. Right?

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8892284 [details]
Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

https://reviewboard.mozilla.org/r/163218/#review168578

Let's make it explicit; I think you should sanitize aOffset on all public functions of MediaResourceIndex.

Comment 7

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

Comment on attachment 8892284 [details]
Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

https://reviewboard.mozilla.org/r/163218/#review168574

> I think you meant:
> 
> if (aOffset < 0) { return NS_ERR...; }
> 
> Otherwise you're not actually setting avoiding setting mOffset to an invalid value, you'd only detect the error on the subsequent entry to this function. Right?

D'oh!
Reviews work.

Also, thinking about it, the other test with `endOffset < 0` could instead do `endOffset < aOffset` to catch large-enough overflows that loops back into positive numbers. (Though it's actually not possible here because of different type sizes, but it's cheap, and would survive a change of types.)

Comment 8

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

Comment on attachment 8892284 [details]
Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

https://reviewboard.mozilla.org/r/163218/#review168578

I intentionally only tested the places that could trip MediaResourceIndex itself, leaving the attached MediaResource be responsible for checking its own inputs.

But it should be easy and cheap to add more checks, so I'll happily do that...

Maybe we should also add checks (if not there yet) in all MediaResource-based classes? (Because not everybody uses MediaResourceIndex.) It could be done in another bug.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes, we should check that the MediaResource handles negative offsets. In another bug is fine.

Comment 10

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

Comment on attachment 8892284 [details]
Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

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

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8892284 [details]
Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

https://reviewboard.mozilla.org/r/163218/#review169070

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71bda1167552
Sanitize offset inputs in MediaResourceIndex - r=cpearce

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/71bda1167552

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here that warrants backport consideration or can this ride the 57 train?

Comment 15

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

Thank you for the suggestion, Ryan.
(That code landed in 55, so esr52 is not affected)

The user impact should be minimal, as things would only go wrong with invalid, and therefore unplayable, media files.

However the fix is trivial (checks+returns), and the patch can be applied as-is, so I think we may as well backport it for peace of mind.

Comment 16

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

Comment on attachment 8892284 [details]
Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1368837 (caching in MediaResourceIndex)
[User impact if declined]: Crashes with some invalid media files
[Is this code covered by automated tests?]: Media playback is tested, including successful code paths, but not the particular error paths added in this patch
[Has the fix been verified in Nightly?]: I tested it manually
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's only adding invalid-offset checks, with early error returns
[String changes made/needed]: None

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8892284 [details]
Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

Fix an assertion error. Beta56+. Should be in 56 beta 2.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/081973e3343d