Sanity check parsed QP values from H264 bitstream

Sanity check parsed QP values from H264 bitstream

BUG=chromium:663610
Committed: https://crrev.com/b336392562dfd2febd66fcc2bdd531ea27c721bf
Cr-Commit-Position: refs/heads/master@{#15377}

[kthelgason, 2016-11-28 15:26:43]

kthelgason@webrtc.org changed reviewers:
+ magjed@webrtc.org, pbos@webrtc.org

[kthelgason, 2016-11-28 15:26:44]

This aborts parsing if the parsed QP values are out of reasonable bounds. This
should prevent UB if the parsed values are large enough to overflow when added.

[magjed_webrtc, 2016-11-28 16:37:05]

Description was changed from

==========
Sanity check parsed QP values from H264 bitstream

BUG=chromium:66361
==========

to

==========
Sanity check parsed QP values from H264 bitstream

BUG=chromium:663610
==========

[magjed_webrtc, 2016-11-28 16:41:32]

https://codereview.webrtc.org/2532973002/diff/1/webrtc/common_video/h264/h264...
File webrtc/common_video/h264/h264_bitstream_parser.cc (right):

https://codereview.webrtc.org/2532973002/diff/1/webrtc/common_video/h264/h264...
webrtc/common_video/h264/h264_bitstream_parser.cc:254: if (last_slice_qp_delta >
52 || last_slice_qp_delta < -52) {
Is the constant 52 specified by the spec? We should name it something, like
kMaxAbsQpDeltaValue.

[pbos-webrtc, 2016-11-28 17:15:46]

https://codereview.webrtc.org/2532973002/diff/1/webrtc/common_video/h264/h264...
File webrtc/common_video/h264/h264_bitstream_parser.cc (right):

https://codereview.webrtc.org/2532973002/diff/1/webrtc/common_video/h264/h264...
webrtc/common_video/h264/h264_bitstream_parser.cc:254: if (last_slice_qp_delta >
52 || last_slice_qp_delta < -52) {
On 2016/11/28 16:41:32, magjed_webrtc wrote:
> Is the constant 52 specified by the spec? We should name it something, like
> kMaxAbsQpDeltaValue.

I think you should also add range checking in GetLastSliceQp which should be the
last step, e.g. something like:

26 + pps_->pic_init_qp_minus26 + *last_slice_qp_delta_ in [0,51]? Otherwise
return false.

If the pps_ should have been set before, then you can make this range check
here.

[kthelgason, 2016-11-29 10:27:12]

https://codereview.webrtc.org/2532973002/diff/1/webrtc/common_video/h264/h264...
File webrtc/common_video/h264/h264_bitstream_parser.cc (right):

https://codereview.webrtc.org/2532973002/diff/1/webrtc/common_video/h264/h264...
webrtc/common_video/h264/h264_bitstream_parser.cc:254: if (last_slice_qp_delta >
52 || last_slice_qp_delta < -52) {
On 2016/11/28 at 16:41:32, magjed_webrtc wrote:
> Is the constant 52 specified by the spec? We should name it something, like
kMaxAbsQpDeltaValue.

Yes, and done.

https://codereview.webrtc.org/2532973002/diff/1/webrtc/common_video/h264/h264...
webrtc/common_video/h264/h264_bitstream_parser.cc:254: if (last_slice_qp_delta >
52 || last_slice_qp_delta < -52) {
On 2016/11/28 at 17:15:45, pbos-webrtc wrote:
> On 2016/11/28 16:41:32, magjed_webrtc wrote:
> > Is the constant 52 specified by the spec? We should name it something, like
> > kMaxAbsQpDeltaValue.
> 
Yes, and done.


> I think you should also add range checking in GetLastSliceQp which should be
the last step, e.g. something like:
> 
> 26 + pps_->pic_init_qp_minus26 + *last_slice_qp_delta_ in [0,51]? Otherwise
return false.
> 
> If the pps_ should have been set before, then you can make this range check
here.

I added range checks in the pps parser instead, seemed more appropriate.

[kthelgason, 2016-11-29 10:27:17]

The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 10:27:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[magjed_webrtc, 2016-11-29 10:34:32]

lgtm

https://codereview.webrtc.org/2532973002/diff/20001/webrtc/common_video/h264/...
File webrtc/common_video/h264/h264_bitstream_parser.cc (right):

https://codereview.webrtc.org/2532973002/diff/20001/webrtc/common_video/h264/...
webrtc/common_video/h264/h264_bitstream_parser.cc:23: int kMaxAbsQpDeltaValue =
51;
nit: const

https://codereview.webrtc.org/2532973002/diff/20001/webrtc/common_video/h264/...
File webrtc/common_video/h264/pps_parser.cc (right):

https://codereview.webrtc.org/2532973002/diff/20001/webrtc/common_video/h264/...
webrtc/common_video/h264/pps_parser.cc:26: int kMaxAbsPicInitQpDeltaValue = 25;
nit: const

[pbos-webrtc, 2016-11-29 15:31:50]

https://codereview.webrtc.org/2532973002/diff/40001/webrtc/common_video/h264/...
File webrtc/common_video/h264/h264_bitstream_parser.cc (right):

https://codereview.webrtc.org/2532973002/diff/40001/webrtc/common_video/h264/...
webrtc/common_video/h264/h264_bitstream_parser.cc:258: if
(abs(last_slice_qp_delta) > kMaxAbsQpDeltaValue) {
"The value of slice_qp_delta shall be limited such that SliceQPY is in the range
of −QpBdOffsetY to +51, inclusive."

Shouldn't this be between 0 and 51, not -51?

https://codereview.webrtc.org/2532973002/diff/40001/webrtc/common_video/h264/...
File webrtc/common_video/h264/pps_parser.cc (right):

https://codereview.webrtc.org/2532973002/diff/40001/webrtc/common_video/h264/...
webrtc/common_video/h264/pps_parser.cc:170: if (abs(pps.pic_init_qp_minus26) >
kMaxAbsPicInitQpDeltaValue) {
"The value of pic_init_qp_minus26 shall be in the range of
−(26 + QpBdOffsetY ) to +25, inclusive."

Assuming that QpBdOffsety is 0, which I think it should be given I420 (I don't
like reading this standard at all).

So < -26 or > 25 seems to be the correct check if we assume I420. Not sure where
that offset is parsed otherwise.

[pbos-webrtc, 2016-11-29 16:12:43]

https://codereview.webrtc.org/2532973002/diff/40001/webrtc/common_video/h264/...
File webrtc/common_video/h264/h264_bitstream_parser.cc (right):

https://codereview.webrtc.org/2532973002/diff/40001/webrtc/common_video/h264/...
webrtc/common_video/h264/h264_bitstream_parser.cc:258: if
(abs(last_slice_qp_delta) > kMaxAbsQpDeltaValue) {
On 2016/11/29 15:31:50, pbos-webrtc wrote:
> "The value of slice_qp_delta shall be limited such that SliceQPY is in the
range
> of −QpBdOffsetY to +51, inclusive."
> 
> Shouldn't this be between 0 and 51, not -51?

Sorry, e.g. should this range be depending on pps_, instead of -51 and 51?

[kthelgason, 2016-11-30 08:42:06]

On 2016/11/29 16:12:43, pbos-webrtc wrote:
>
https://codereview.webrtc.org/2532973002/diff/40001/webrtc/common_video/h264/...
> File webrtc/common_video/h264/h264_bitstream_parser.cc (right):
> 
>
https://codereview.webrtc.org/2532973002/diff/40001/webrtc/common_video/h264/...
> webrtc/common_video/h264/h264_bitstream_parser.cc:258: if
> (abs(last_slice_qp_delta) > kMaxAbsQpDeltaValue) {
> On 2016/11/29 15:31:50, pbos-webrtc wrote:
> > "The value of slice_qp_delta shall be limited such that SliceQPY is in the
> range
> > of −QpBdOffsetY to +51, inclusive."
> > 
> > Shouldn't this be between 0 and 51, not -51?
> 
> Sorry, e.g. should this range be depending on pps_, instead of -51 and 51?

Yes, technically. I thought this would be simpler, and good enough.

[kthelgason, 2016-11-30 08:45:37]

On 2016/11/29 15:31:50, pbos-webrtc wrote:
> "The value of pic_init_qp_minus26 shall be in the range of
> −(26 + QpBdOffsetY ) to +25, inclusive."
> 
> Assuming that QpBdOffsety is 0, which I think it should be given I420 (I don't
> like reading this standard at all).
> 
> So < -26 or > 25 seems to be the correct check if we assume I420. Not sure
where
> that offset is parsed otherwise.

You're right that QpBdOffsety is in most cases 0, I must've read the spec wrong.
Will fix.

[pbos-webrtc, 2016-11-30 16:41:45]

On 2016/11/30 08:45:37, kthelgason wrote:
> On 2016/11/29 15:31:50, pbos-webrtc wrote:
> > "The value of pic_init_qp_minus26 shall be in the range of
> > −(26 + QpBdOffsetY ) to +25, inclusive."
> > 
> > Assuming that QpBdOffsety is 0, which I think it should be given I420 (I
don't
> > like reading this standard at all).
> > 
> > So < -26 or > 25 seems to be the correct check if we assume I420. Not sure
> where
> > that offset is parsed otherwise.
> 
> You're right that QpBdOffsety is in most cases 0, I must've read the spec
wrong.
> Will fix.

I think the range checks might be good enough where they are, especially so that
we can parse broken bitstreams that has a PPS or SPS dropped perhaps, but can
you add a range check inside GetLastSliceQp so that it cannot return an invalid
QP value? [0,51] inclusive I believe is the valid range, but by now I assume you
know this better than I do. :)

[kthelgason, 2016-12-01 12:55:07]

On 2016/11/30 16:41:45, pbos-webrtc wrote:
> I think the range checks might be good enough where they are, especially so
that
> we can parse broken bitstreams that has a PPS or SPS dropped perhaps, but can
> you add a range check inside GetLastSliceQp so that it cannot return an
invalid
> QP value? [0,51] inclusive I believe is the valid range, but by now I assume
you
> know this better than I do. :)

I'm not sure I understand what you mean, we can't parse bitstreams where the PPS
is missing, they won't make any sense?

In any case, I added one last check in GetLastSliceQp, that should make sure
that we don't return invalid values.

[pbos-webrtc, 2016-12-01 19:01:51]

On 2016/12/01 12:55:07, kthelgason wrote:
> On 2016/11/30 16:41:45, pbos-webrtc wrote:
> > I think the range checks might be good enough where they are, especially so
> that
> > we can parse broken bitstreams that has a PPS or SPS dropped perhaps, but
can
> > you add a range check inside GetLastSliceQp so that it cannot return an
> invalid
> > QP value? [0,51] inclusive I believe is the valid range, but by now I assume
> you
> > know this better than I do. :)
> 
> I'm not sure I understand what you mean, we can't parse bitstreams where the
PPS
> is missing, they won't make any sense?

I meant if dropping a PPS (and a minus26 in the middle) makes the future QP
values bogus (out of range). No good way to guard against that as far as I'm
aware though.

> In any case, I added one last check in GetLastSliceQp, that should make sure
> that we don't return invalid values.

Exactly what I wanted, thanks. lgtm

[kthelgason, 2016-12-02 08:41:58]

The CQ bit was checked by kthelgason@webrtc.org

[kthelgason, 2016-12-02 08:41:58]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2532973002/#ps80001 (title:
"add one last range check")

[commit-bot: I haz the power, 2016-12-02 08:42:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-02 09:29:48]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480668118124280,
"parent_rev": "824a23064023671420858f072f8451fcdda36e96", "commit_rev":
"97d235930c45a59b224b6f33aaadb42354872261"}

[commit-bot: I haz the power, 2016-12-02 09:29:50]

Description was changed from

==========
Sanity check parsed QP values from H264 bitstream

BUG=chromium:663610
==========

to

==========
Sanity check parsed QP values from H264 bitstream

BUG=chromium:663610
==========

[commit-bot: I haz the power, 2016-12-02 09:29:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-02 09:30:00]

Description was changed from

==========
Sanity check parsed QP values from H264 bitstream

BUG=chromium:663610
==========

to

==========
Sanity check parsed QP values from H264 bitstream

BUG=chromium:663610

Committed: https://crrev.com/b336392562dfd2febd66fcc2bdd531ea27c721bf
Cr-Commit-Position: refs/heads/master@{#15377}
==========

[commit-bot: I haz the power, 2016-12-02 09:30:01]

Patchset 5 (id:??) landed as
https://crrev.com/b336392562dfd2febd66fcc2bdd531ea27c721bf
Cr-Commit-Position: refs/heads/master@{#15377}