1322961 - [CID 1397065] [CID 1397066] Comparing uint8_t with 256 in mp4_demuxer::H264

Status: RESOLVED FIXED

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

Description

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

Found by Coverity:
> *** CID 1397065:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
> /media/libstagefright/binding/H264.cpp: 715 in
> mp4_demuxer::H264::DecodePPS(const mozilla::MediaByteBuffer *, const AutoTArray<mp4_demuxer::SPSData, (unsigned long)32> &, mp4_demuxer::PPSData &)()
>    CID 1397065:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
>    "aDest.pic_parameter_set_id >= 256" is always false regardless of the values of its operands. This occurs as the logical first operand of "||".
> 715       if (aDest.pic_parameter_set_id >= MAX_PPS_COUNT ||
> 716           aDest.seq_parameter_set_id >= MAX_SPS_COUNT) {
> 717         return false;
> 718       }

> *** CID 1397066:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
> /media/libstagefright/binding/H264.cpp: 666 in
> mp4_demuxer::H264::DecodePPSDataSetFromExtraData(const mozilla::MediaByteBuffer *, const AutoTArray<mp4_demuxer::SPSData, (unsigned long)32> &, AutoTArray<mp4_demuxer::PPSData, (unsigned long)256>&)()
>    CID 1397066:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
>    "numPps <= 256" is always true regardless of the values of its operands. This occurs as the logical operand of "!".
> 666       NS_ASSERTION(numPps <= MAX_PPS_COUNT, "Exceed the maximum PPS counts!");

Probably not a real issue, but it would be nice to verify that, and maybe fix it by doing appropriate casts.

Alastor, this code comes from bug 1321164, could you please have a look?

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1322961: Check SPS and PPS attributes value.

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

Comment 2

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

Comment on attachment 8817956 [details]
Bug 1322961: Check SPS and PPS attributes value.

https://reviewboard.mozilla.org/r/98140/#review98340

Quite a lot more than I expected for this bug! But it makes sense to add these checks.

r+ with probable issue addressed:

::: media/libstagefright/binding/H264.cpp:850
(Diff revision 1)
> +    if (aDest.second_chroma_qp_index_offset < 12
> +        || aDest.second_chroma_qp_index_offset > 12) {

Don't you mean `< -12`, like at line 797?

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8817956 [details]
Bug 1322961: Check SPS and PPS attributes value.

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

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8817956 [details]
Bug 1322961: Check SPS and PPS attributes value.

rewrote it all using macro. much simpler that way, and allows to keep using 8 bits fields.

Comment 5

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

Comment on attachment 8817956 [details]
Bug 1322961: Check SPS and PPS attributes value.

https://reviewboard.mozilla.org/r/98140/#review98350

r+ assuming try is green.

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8817956 [details]
Bug 1322961: Check SPS and PPS attributes value.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a3e0c8b26f2
Check SPS and PPS attributes value. r=gerald

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a3e0c8b26f2