Closed Bug 1334971 Opened 3 years ago Closed 3 years ago

InvalidArrayIndex_CRASH from mp4_demuxer::H264::DecodePPS, playing BBB at http://demo.castlabs.com/

Categories

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

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: gerald, Assigned: jya)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

Go to http://demo.castlabs.com/ and start playing "Big Buck Bunny", it crashes with the following stack in a MediaPlayback thread:
(This one on Mac, with a debug build from trunk)
> 0   InvalidArrayIndex_CRASH(unsigned long, unsigned long) + 54 (nsTArray.cpp:35)
> 1   mp4_demuxer::H264::DecodePPS(mozilla::MediaByteBuffer const*, AutoTArray<mp4_demuxer::SPSData, 32ul> const&, mp4_demuxer::PPSData&) + 28904
> 2   mp4_demuxer::H264::DecodePPSDataSetFromExtraData(mozilla::MediaByteBuffer const*, AutoTArray<mp4_demuxer::SPSData, 32ul> const&, AutoTArray<mp4_demuxer::PPSData, 256ul>&) + 835 (H264.cpp:756)
> 3   mp4_demuxer::H264::DecodeSPSFromExtraData(mozilla::MediaByteBuffer const*, mp4_demuxer::SPSData&) + 216 (H264.cpp:616)
> 4   mozilla::AccumulateSPSTelemetry(mozilla::MediaByteBuffer const*) + 43 (MP4Demuxer.cpp:84)
> 5   mozilla::MP4TrackDemuxer::MP4TrackDemuxer(mozilla::MP4Demuxer*, mozilla::UniquePtr<mozilla::TrackInfo, mozilla::DefaultDelete<mozilla::TrackInfo> >&&, nsTArray<mp4_demuxer::Index::Indice> const&) + 605 (MP4Demuxer.cpp:251)
> 6   mozilla::MP4Demuxer::GetTrackDemuxer(mozilla::TrackInfo::TrackType, unsigned int) + 206 (RefPtr.h:108)
> 7   mozilla::TrackBuffersManager::OnDemuxerInitDone(nsresult) + 278 (AlreadyAddRefed.h:116)
> 8   mozilla::MozPromise<nsresult, mozilla::MediaResult, true>::MethodThenValue<mozilla::TrackBuffersManager, void (mozilla::TrackBuffersManager::*)(nsresult), void (mozilla::TrackBuffersManager::*)(mozilla::MediaResult const&)>::DoResolveOrRejectInternal(mozilla::MozPromise<nsresult, mozilla::MediaResult, true>::ResolveOrRejectValue const&) + 58 (MozPromise.h:443)
> 9   mozilla::MozPromise<nsresult, mozilla::MediaResult, true>::ThenValueBase::DoResolveOrReject(mozilla::MozPromise<nsresult, mozilla::MediaResult, true>::ResolveOrRejectValue const&) + 135 (AlreadyAddRefed.h:116)
> 10  mozilla::MozPromise<nsresult, mozilla::MediaResult, true>::ThenValueBase::ResolveOrRejectRunnable::Run() + 132 (RefPtr.h:62)
> 11  mozilla::TaskQueue::Runner::Run() + 420 (TaskQueue.h:164)
...

Nightly & aurora are affected, on at least Mac and Windows.
jya: Is this your area?
Flags: needinfo?(jyavenard)
Blocks: 1321164
Flags: needinfo?(jyavenard) → needinfo?(alwu)
Keywords: regression
Flags: needinfo?(alwu)
Assignee: nobody → jyavenard
Comment on attachment 8833970 [details]
Bug 1334971: P1. Properly handle invalid PPS.

https://reviewboard.mozilla.org/r/110072/#review111226

r+ with commit description nits:

::: media/libstagefright/binding/H264.cpp:759
(Diff revision 2)
> +    if (ppsData.pic_parameter_set_id >= aDest.Length()) {
> +      aDest.SetLength(ppsData.pic_parameter_set_id + 1);

Please mention the why&how of this fix in the commit description.

::: media/libstagefright/binding/H264.cpp:784
(Diff revision 2)
> +  if (aDest.seq_parameter_set_id >= aSPSes.Length()) {
> +    return false;

Please mention the why&how of this fix in the commit description.
Attachment #8833970 - Flags: review?(gsquelart) → review+
Comment on attachment 8833971 [details]
Bug 1334971: P2. Fix coding style.

https://reviewboard.mozilla.org/r/110074/#review111230

r+ with nit:

::: dom/media/platforms/PDMFactory.cpp:260
(Diff revision 2)
>    const TrackInfo& config = aParams.mConfig;
>    supportChecker.AddMediaFormatChecker(config);
>  
>    auto checkResult = supportChecker.Check();
>    if (checkResult.mReason != SupportChecker::Reason::kSupported) {
> -    DecoderDoctorDiagnostics* diagnostics = aParams.mDiagnostics;
> +    DecoderDoctorDiagnostics *diagnostics = aParams.mDiagnostics;

Please don't use C-spirit pointer declarations!
Attachment #8833971 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/mozilla-central/rev/5ef27c9b65ef
https://hg.mozilla.org/mozilla-central/rev/27f9cf49b9fe
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8833970 [details]
Bug 1334971: P1. Properly handle invalid PPS.

Approval Request Comment
[Feature/Bug causing the regression]: 1321164
[User impact if declined]: crashes, change of resolution not detected
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: Follow STR with a debug build
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: We ensure that memory is allocated before using it, before memory was used without ever allocating it.
[String changes made/needed]: none
Attachment #8833970 - Flags: approval-mozilla-aurora?
Comment on attachment 8833970 [details]
Bug 1334971: P1. Properly handle invalid PPS.

Fix a crash. Aurora53+.
Attachment #8833970 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.