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

RESOLVED FIXED in Firefox 56

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: tsmith, Assigned: gerald)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla57
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 wontfix, firefox56 fixed, firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

6 months ago
Created attachment 8890011 [details]
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
(Reporter)

Comment 1

6 months ago
Created attachment 8890013 [details]
test_case.webm
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.
(Assignee)

Comment 3

6 months ago
Agreed!
Assignee: nobody → gsquelart
Comment hidden (mozreview-request)

Comment 5

6 months ago
mozreview-review
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

6 months ago
mozreview-review
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.
Attachment #8892284 - Flags: review?(cpearce) → review-
(Assignee)

Comment 7

6 months ago
mozreview-review-reply
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.)
(Assignee)

Comment 8

6 months ago
mozreview-review-reply
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.
Yes, we should check that the MediaResource handles negative offsets. In another bug is fine.
Comment hidden (mozreview-request)

Comment 11

6 months ago
mozreview-review
Comment on attachment 8892284 [details]
Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

https://reviewboard.mozilla.org/r/163218/#review169070
Attachment #8892284 - Flags: review?(cpearce) → review+
(Assignee)

Updated

6 months ago
Blocks: 1386453

Comment 12

6 months ago
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71bda1167552
Sanitize offset inputs in MediaResourceIndex - r=cpearce

Comment 13

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71bda1167552
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Is there a user impact here that warrants backport consideration or can this ride the 57 train?
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox-esr52: --- → wontfix
Flags: needinfo?(gsquelart)
(Assignee)

Comment 15

5 months ago
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.
status-firefox-esr52: wontfix → unaffected
Flags: needinfo?(gsquelart)
(Assignee)

Comment 16

5 months ago
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
Attachment #8892284 - Flags: approval-mozilla-beta?
Comment on attachment 8892284 [details]
Bug 1384243 - Sanitize offset inputs in MediaResourceIndex -

Fix an assertion error. Beta56+. Should be in 56 beta 2.
Attachment #8892284 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 18

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/081973e3343d
status-firefox56: affected → fixed
You need to log in before you can comment on or make changes to this bug.