Fix out-of-range access in unibrow::Utf8::CalculateValue.

Fix out-of-range access in unibrow::Utf8::CalculateValue.

This code should not access bytes out of the permitted range in order to check
the range of a possible UTF-8 value. Instead, the length check should occur
before such checks.

BUG=chromium:667260, chromium:662822
Committed: https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628
Cr-Commit-Position: refs/heads/master@{#41165}

[jbroman, 2016-11-21 23:53:50]

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

[commit-bot: I haz the power, 2016-11-21 23:53:53]

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

[jbroman, 2016-11-22 00:25:03]

jbroman@chromium.org changed reviewers:
+ vogelheim@chromium.org

[jbroman, 2016-11-22 00:25:04]

https://codereview.chromium.org/2520053003/diff/1/src/unicode-decoder.h
File src/unicode-decoder.h (right):

https://codereview.chromium.org/2520053003/diff/1/src/unicode-decoder.h#newco...
src/unicode-decoder.h:10: #include "src/utils.h"
MemCopy is used in this file.

https://codereview.chromium.org/2520053003/diff/1/src/unicode-decoder.h#newco...
src/unicode-decoder.h:14: class V8_EXPORT_PRIVATE Utf8DecoderBase {
Allows unicode-unittest.cc to directly use this.

https://codereview.chromium.org/2520053003/diff/1/src/unicode.cc
File src/unicode.cc (right):

https://codereview.chromium.org/2520053003/diff/1/src/unicode.cc#newcode238
src/unicode.cc:238: *cursor += count;
I lifted this here, because in both cases it was incremented by count (either
explicitly, or in the success case it's incremented by length, which must be
equal to count at that point). Doing so makes the returns for the failure cases
more concise.

https://codereview.chromium.org/2520053003/diff/1/test/unittests/unicode-unit...
File test/unittests/unicode-unittest.cc (right):

https://codereview.chromium.org/2520053003/diff/1/test/unittests/unicode-unit...
test/unittests/unicode-unittest.cc:31: // Not enough continuation bytes before
string ends.
All of these cases produce AddressSanitizer failures if unittests is run with
is_asan=true.

[commit-bot: I haz the power, 2016-11-22 00:26:58]

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

[commit-bot: I haz the power, 2016-11-22 00:26:58]

Dry run: This issue passed the CQ dry run.

[vogelheim, 2016-11-22 09:24:35]

Description was changed from

==========
Fix out-of-range access in unibrow::Utf8::CalculateValue.

This code should not access bytes out of the permitted range in order to check
the range of a possible UTF-8 value. Instead, the length check should occur
before such checks.

BUG=chromium:667260
==========

to

==========
Fix out-of-range access in unibrow::Utf8::CalculateValue.

This code should not access bytes out of the permitted range in order to check
the range of a possible UTF-8 value. Instead, the length check should occur
before such checks.

BUG=chromium:667260, chromium:662822
==========

[vogelheim, 2016-11-22 09:25:53]

lgtm

Thanks! I really like that this makes the code more readable, too. :-)

https://codereview.chromium.org/2520053003/diff/1/test/unittests/unicode-unit...
File test/unittests/unicode-unittest.cc (right):

https://codereview.chromium.org/2520053003/diff/1/test/unittests/unicode-unit...
test/unittests/unicode-unittest.cc:31: // Not enough continuation bytes before
string ends.
On 2016/11/22 00:25:04, jbroman wrote:
> All of these cases produce AddressSanitizer failures if unittests is run with
> is_asan=true.

Err... I take it they would with the original code, but no longer after this
fix, right?

[vogelheim, 2016-11-22 09:26:00]

The CQ bit was checked by vogelheim@chromium.org

[commit-bot: I haz the power, 2016-11-22 09:26:03]

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, 2016-11-22 09:27:41]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1479806760113620, "parent_rev":
"84f03b7b06580ecdc08509ec125ec8ddd75b73a6", "commit_rev":
"c0b7ce5e17c44f5d863ef286c24e249230221dba"}

[commit-bot: I haz the power, 2016-11-22 09:27:48]

Description was changed from

==========
Fix out-of-range access in unibrow::Utf8::CalculateValue.

This code should not access bytes out of the permitted range in order to check
the range of a possible UTF-8 value. Instead, the length check should occur
before such checks.

BUG=chromium:667260, chromium:662822
==========

to

==========
Fix out-of-range access in unibrow::Utf8::CalculateValue.

This code should not access bytes out of the permitted range in order to check
the range of a possible UTF-8 value. Instead, the length check should occur
before such checks.

BUG=chromium:667260, chromium:662822
==========

[commit-bot: I haz the power, 2016-11-22 09:27:48]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-22 09:28:09]

Description was changed from

==========
Fix out-of-range access in unibrow::Utf8::CalculateValue.

This code should not access bytes out of the permitted range in order to check
the range of a possible UTF-8 value. Instead, the length check should occur
before such checks.

BUG=chromium:667260, chromium:662822
==========

to

==========
Fix out-of-range access in unibrow::Utf8::CalculateValue.

This code should not access bytes out of the permitted range in order to check
the range of a possible UTF-8 value. Instead, the length check should occur
before such checks.

BUG=chromium:667260, chromium:662822

Committed: https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628
Cr-Commit-Position: refs/heads/master@{#41165}
==========

[commit-bot: I haz the power, 2016-11-22 09:28:10]

Patchset 1 (id:??) landed as
https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628
Cr-Commit-Position: refs/heads/master@{#41165}

[jbroman, 2016-11-22 12:40:44]

https://codereview.chromium.org/2520053003/diff/1/test/unittests/unicode-unit...
File test/unittests/unicode-unittest.cc (right):

https://codereview.chromium.org/2520053003/diff/1/test/unittests/unicode-unit...
test/unittests/unicode-unittest.cc:31: // Not enough continuation bytes before
string ends.
On 2016/11/22 at 09:25:53, vogelheim wrote:
> On 2016/11/22 00:25:04, jbroman wrote:
> > All of these cases produce AddressSanitizer failures if unittests is run
with
> > is_asan=true.
> 
> Err... I take it they would with the original code, but no longer after this
fix, right?

Yes, of course. :-)