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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

jya: Is this your area?

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1334971: P1. Properly handle invalid PPS.

Also fix H264::DecodePPS which incorrectly always returned an error when parsing a valid PPS.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1334971: P2. Fix coding style.

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

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8833970 [details]
Bug 1334971: P1. Properly handle invalid PPS.

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

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8833971 [details]
Bug 1334971: P2. Fix coding style.

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

Comment 6

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

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.

Comment 7

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

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!

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8833970 [details]
Bug 1334971: P1. Properly handle invalid PPS.

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

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8833971 [details]
Bug 1334971: P2. Fix coding style.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ef27c9b65ef
P1. Properly handle invalid PPS. r=gerald
https://hg.mozilla.org/integration/autoland/rev/27f9cf49b9fe
P2. Fix coding style. r=gerald

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ef27c9b65ef
https://hg.mozilla.org/mozilla-central/rev/27f9cf49b9fe

Comment 12

[Jean-Yves Avenard [:jya]]

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

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8833970 [details]
Bug 1334971: P1. Properly handle invalid PPS.

Fix a crash. Aurora53+.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/019dfad9dcb7
https://hg.mozilla.org/releases/mozilla-aurora/rev/fb6d519b2298