1298271 - Fix libstagefright's use of ByteReader's remaining bytes

Status: RESOLVED FIXED

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

Description

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

Follow-up to bug 1296532 comment 14.

If libstagefright (particularly in MoofParser), ByteReader is used to safely read MP4 boxes.
However the checking of Remaining(), and use of DiscardRemaining(), are inconsistent, which in the worst case could lead to assertion crashes in debug mode.

We could import VideoUtils' AutoByteReader, and stop using DiscardRemaining() to simplify the code.

Note that some videos might then get accepted even though some of their boxes are wider than expected, but I think it's not a risk, and in fact a useful attempt at accommodating imperfect videos.

Comment 1

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

Correction: AutoByteReader is in FlacFrameParser.cpp.

Comment 2

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

Attachment: Bug 1298271 - Make BoxReader use AutoByteReader -

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

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785274 [details]
Bug 1298271 - Make BoxReader use AutoByteReader -

https://reviewboard.mozilla.org/r/74542/#review72392

::: dom/media/flac/FlacFrameParser.cpp:63
(Diff revision 1)
>  {
>    if (aLength < 4 || aPacket[0] == 0xff) {
>      // Not a header block.
>      return false;
>    }
> -  AutoByteReader br(aPacket, aLength);
> +  mp4_demuxer::AutoByteReader br(aPacket, aLength);

ID prefer to have a using namespace mp4_demuxer at the top.

::: media/libstagefright/binding/Box.cpp:146
(Diff revision 1)
>    }
>    return Box(mContext, mChildOffset, this);
>  }
>  
>  bool
>  Box::Read(nsTArray<uint8_t>* aDest)

IS this function still of any use?

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8785274 [details]
Bug 1298271 - Make BoxReader use AutoByteReader -

https://reviewboard.mozilla.org/r/74542/#review72400

Comment 5

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

Comment on attachment 8785274 [details]
Bug 1298271 - Make BoxReader use AutoByteReader -

https://reviewboard.mozilla.org/r/74542/#review72392

> ID prefer to have a using namespace mp4_demuxer at the top.

Importing a whole namespace for just three uses of the same name seems a bit over the top! I'll just import the one name I need.

> IS this function still of any use?

Right, I've effectively replaced it with my version; so I'll remove the old one.

Comment 6

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

Comment on attachment 8785274 [details]
Bug 1298271 - Make BoxReader use AutoByteReader -

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f302a3d9df0
Make BoxReader use AutoByteReader - r=jya

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Gerald - AutoByteReader makes no sense. Just change the assertion in ByteReader to propagate the error in a different way or simply get rid of the whole mechanism.

Comment 9

[Jean-Yves Avenard [:jya]]

I think the entire checking that there are bytes remaining can be removed now. There's no security risk when finding cases where we didn't read the whole content.

Comment 10

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

Opened follow-up bug 1298710 to deal with AutoByteReader.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f302a3d9df0