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

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.

Agreed!

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 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 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 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 on attachment 8892284 [details] Bug 1384243 - Sanitize offset inputs in MediaResourceIndex - https://reviewboard.mozilla.org/r/163218/#review169070

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

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

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 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 on attachment 8892284 [details] Bug 1384243 - Sanitize offset inputs in MediaResourceIndex - Fix an assertion error. Beta56+. Should be in 56 beta 2.