Tyson Smith [:tsmith] Reporter
	Description • 7 years ago

Found with mozilla-central asan debug buildID=20170327212148

==59301==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004edccf bp 0x7f25ed607490 sp 0x7f25ed607480 T55)
==59301==The signal is caused by a WRITE memory access.
==59301==Hint: address points to the zero page.
    #0 0x4edcce in mozalloc_abort(char const*) /home/worker/workspace/build/src/memory/mozalloc/mozalloc_abort.cpp:33:5
    #1 0x7f2612d0b9a5 in Abort(char const*) /home/worker/workspace/build/src/xpcom/base/nsDebugImpl.cpp:441:3
    #2 0x7f2612d0b67c in NS_DebugBreak /home/worker/workspace/build/src/xpcom/base/nsDebugImpl.cpp:428:7
    #3 0x7f261a89b8aa in fpehandler(int, siginfo*, void*) /home/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp:156:5
    #4 0x7f26302a138f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1138f)
    #5 0x7f2612c5b664 in stagefright::unitsToUs(long, long) /home/worker/workspace/build/src/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp:55:32
    #6 0x7f2612c60f51 in stagefright::MPEG4Extractor::parseChunk(long*, int) /home/worker/workspace/build/src/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp:1847:35
    #7 0x7f2612c5eba7 in stagefright::MPEG4Extractor::parseChunk(long*, int) /home/worker/workspace/build/src/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp:807:32
    #8 0x7f2612c5eba7 in stagefright::MPEG4Extractor::parseChunk(long*, int) /home/worker/workspace/build/src/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp:807:32
    #9 0x7f2612c5c657 in stagefright::MPEG4Extractor::readMetaData() /home/worker/workspace/build/src/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp:407:15
    #10 0x7f2612c5c4c4 in stagefright::MPEG4Extractor::getMetaData() /home/worker/workspace/build/src/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp:342:16
    #11 0x7f2612c4c430 in mp4_demuxer::MP4MetadataStagefright::MP4MetadataStagefright(mp4_demuxer::Stream*) /home/worker/workspace/build/src/media/libstagefright/binding/MP4Metadata.cpp:491:47
    #12 0x7f2612c479ef in mozilla::detail::UniqueSelector<mp4_demuxer::MP4MetadataStagefright>::SingleObject mozilla::MakeUnique<mp4_demuxer::MP4MetadataStagefright, mp4_demuxer::Stream*&>(mp4_demuxer::Stream*&) /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/UniquePtr.h:680:27
    #13 0x7f2612c4785f in mp4_demuxer::MP4Metadata::MP4Metadata(mp4_demuxer::Stream*) /home/worker/workspace/build/src/media/libstagefright/binding/MP4Metadata.cpp:231:17
    #14 0x7f26173b6d9b in mozilla::MP4Demuxer::Init() /home/worker/workspace/build/src/dom/media/fmp4/MP4Demuxer.cpp:138:28
...
see log.txt

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 2 • 7 years ago

Div by 0; We should just add a simple `hz==0` test in unitsToUs.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 5 • 7 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fd4b644cb991a8bdf0d37ed3989b5102386c8c1

	Alfredo Yang (:alfredo)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8851883 [details]
Bug 1351094 - Catch div/0 when hz==0 in MPEG4Extractor's unitsToUs -

https://reviewboard.mozilla.org/r/124116/#review126640

	Alfredo Yang (:alfredo)
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8851884 [details]
Bug 1351094 - gtest -

https://reviewboard.mozilla.org/r/124118/#review126642

	Pulsebot
	Comment 8 • 7 years ago

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95c849509574
Catch div/0 when hz==0 in MPEG4Extractor's unitsToUs - r=alfredo
https://hg.mozilla.org/integration/autoland/rev/bfbdd72a4445
gtest - r=alfredo

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 9 • 7 years ago

Thank you Alfredo for all the reviews today.

	Alfredo Yang (:alfredo)
	Comment 10 • 7 years ago

(In reply to Gerald Squelart [:gerald] from comment #9)
> Thank you Alfredo for all the reviews today.

No problem. :-)

	Carsten Book [:Tomcat]
	Comment 11 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/95c849509574
https://hg.mozilla.org/mozilla-central/rev/bfbdd72a4445

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 7 years ago

Is this something we should consider backporting to affected branches? AFAICT, this goes back to Fx51.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 13 • 7 years ago

Sounds right.

Happy to request uplift, easy enough as the patch applies directly to esr52, beta, and aurora.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) Assignee
	Comment 14 • 7 years ago

Comment on attachment 8851883 [details]
Bug 1351094 - Catch div/0 when hz==0 in MPEG4Extractor's unitsToUs -

Uplifts request for *this* patch only (not the gtest).

Aurora 54, Beta 53:
Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Possible crashes from malformed MP4 files.
[Is this code covered by automated tests?]: Yes, in Nightly.
[Has the fix been verified in Nightly?]: Yes, through new gtest with PoC test case.
[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?]: Just a 0-check with early error return.
[String changes made/needed]: None.

ESR 52:
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible crashes from malformed MP4 files.
Fix Landed on Version: 55.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

	Gerry Chang [:gchang]
	Comment 15 • 7 years ago

Comment on attachment 8851883 [details]
Bug 1351094 - Catch div/0 when hz==0 in MPEG4Extractor's unitsToUs -

Fix a crash. Aurora54+ & Beta53+.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/b75313f1ebf4
https://hg.mozilla.org/releases/mozilla-aurora/rev/d27dba6e5aa7

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/ecfa2c31fd86
https://hg.mozilla.org/releases/mozilla-beta/rev/74a231f942d8

	Julien Cristau [:jcristau]
	Comment 18 • 7 years ago

Comment on attachment 8851883 [details]
Bug 1351094 - Catch div/0 when hz==0 in MPEG4Extractor's unitsToUs -

crash fix for esr52

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/4c1383e76adc
https://hg.mozilla.org/releases/mozilla-esr52/rev/371202fc44ae

	Andrei Vaida [:avaida]
	Comment 20 • 7 years ago

Setting qe-verify- based on Gerald's assessment on manual testing needs (see Comment 14) and the fact that this fix has automated coverage.