Avoid overflow on left shift in HpackVarintDecoder::Resume().

Avoid overflow on left shift in HpackVarintDecoder::Resume().

This CL only changes functionality if |offset_ == MaxOffset()| and
|byte != 0| in the last execution of the do loop.  In this case, the
final value of |offset_| will be different, but this is a private member
with no accessor, and has no effect visible to consumers.  Also,
|value_| will not be incremented in the last cycle, in order to avoid
the runtime error that Clusterfuzz filed this bug for.  However, in this
case decoding fails with kDecodeError, and |value_| is considered
invalid anyway.

BUG=698698
Review-Url: https://codereview.chromium.org/2819873002
Cr-Commit-Position: refs/heads/master@{#465235}
Committed: https://chromium.googlesource.com/chromium/src/+/0b38727639f5a7846099709654d065ab4c90239d

[Bence, 2017-04-14 18:12:34]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 18:12:52]

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

[commit-bot: I haz the power, 2017-04-14 19:21:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 19:21:53]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-04-15 00:03:21]

bnc@chromium.org changed reviewers:
+ jamessynge@chromium.org

[Bence, 2017-04-15 00:03:21]

James: PTAL.

[jamessynge, 2017-04-17 18:19:45]

On 2017/04/15 00:03:21, Bence wrote:
> James: PTAL.

LGTM. I'll see if there is an alternate approach with satisfies both the fuzzer
and my insatiable desire to reduce the number of branches. ;-)

[Bence, 2017-04-17 18:25:08]

bnc@chromium.org changed reviewers:
+ xunjieli@chromium.org

[Bence, 2017-04-17 18:25:09]

James: Thank you for reviewing.
xunjieli: please rubber stamp.  Thank you.

[xunjieli, 2017-04-17 20:51:34]

https://codereview.chromium.org/2819873002/diff/1/net/http2/hpack/decoder/hpa...
File net/http2/hpack/decoder/hpack_varint_decoder.h (right):

https://codereview.chromium.org/2819873002/diff/1/net/http2/hpack/decoder/hpa...
net/http2/hpack/decoder/hpack_varint_decoder.h:98: if (offset_ == MaxOffset() &&
byte != 0)
Can we change this to a while-loop to be more readable? It's hard to know what's
going on in this if-conditional.


    while (offset_ <= std::min(MaxOffset(), sizeof(value_) - sizeof(uint8))) {
      if (db->Empty()) {
        return DecodeStatus::kDecodeInProgress;
      }
      uint8_t byte = db->DecodeUInt8();
      value_ += (byte & 0x7f) << offset_;
      if ((byte & 0x80) == 0) {
          MarkDone();
          return DecodeStatus::kDecodeDone;
      }
      offset_ += 7;
    }

[xunjieli, 2017-04-18 13:37:33]

lgtm
per offline discussion, changing the shared code is a hassle (might involve a
flag flip). This particular case might not be worth it.

[Bence, 2017-04-18 13:37:47]

The CQ bit was checked by bnc@chromium.org

[Bence, 2017-04-18 13:37:56]

Thank you.

[commit-bot: I haz the power, 2017-04-18 13:38:09]

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

[commit-bot: I haz the power, 2017-04-18 14:52:53]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1492522667867240, "parent_rev":
"223b9ac0cbebc9029c09318766c8e944af036588", "commit_rev":
"0b38727639f5a7846099709654d065ab4c90239d"}

[commit-bot: I haz the power, 2017-04-18 14:53:04]

Description was changed from

==========
Avoid overflow on left shift in HpackVarintDecoder::Resume().

This CL only changes functionality if |offset_ == MaxOffset()| and
|byte != 0| in the last execution of the do loop.  In this case, the
final value of |offset_| will be different, but this is a private member
with no accessor, and has no effect visible to consumers.  Also,
|value_| will not be incremented in the last cycle, in order to avoid
the runtime error that Clusterfuzz filed this bug for.  However, in this
case decoding fails with kDecodeError, and |value_| is considered
invalid anyway.

BUG=698698
==========

to

==========
Avoid overflow on left shift in HpackVarintDecoder::Resume().

This CL only changes functionality if |offset_ == MaxOffset()| and
|byte != 0| in the last execution of the do loop.  In this case, the
final value of |offset_| will be different, but this is a private member
with no accessor, and has no effect visible to consumers.  Also,
|value_| will not be incremented in the last cycle, in order to avoid
the runtime error that Clusterfuzz filed this bug for.  However, in this
case decoding fails with kDecodeError, and |value_| is considered
invalid anyway.

BUG=698698

Review-Url: https://codereview.chromium.org/2819873002
Cr-Commit-Position: refs/heads/master@{#465235}
Committed:
https://chromium.googlesource.com/chromium/src/+/0b38727639f5a7846099709654d0...
==========

[commit-bot: I haz the power, 2017-04-18 14:53:05]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/0b38727639f5a7846099709654d0...