Catch potential integer overflows in bigint operations.

Catch potential integer overflows in bigint operations.

strlen() returns a size_t which usually is unsigned long. Since we assign it to
intptr_t, we need to check for a potential integer overflow. The (albeit very
unlikely) case is lengths>2G on 32bit platforms.

BUG=
R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27589

[siva, 2013-09-17 00:19:52]

https://codereview.chromium.org/23754006/diff/3001/runtime/vm/bigint_operatio...
File runtime/vm/bigint_operations.cc (right):

https://codereview.chromium.org/23754006/diff/3001/runtime/vm/bigint_operatio...
runtime/vm/bigint_operations.cc:295: }
As discussed offline the overflow could happen before the last computation
making it positive.

We should probably compute a kMaxChunkLen and compare chunk_length against that
and error out.

https://codereview.chromium.org/23754006/diff/3001/runtime/vm/bigint_operatio...
runtime/vm/bigint_operations.cc:369: if (required_size !=
static_cast<intptr_t>(required_size)) {
Ditto comment here too.

[Michael Lippautz (Google), 2013-09-17 03:36:50]

Thanks Siva!

https://codereview.chromium.org/23754006/diff/3001/runtime/vm/bigint_operatio...
File runtime/vm/bigint_operations.cc (right):

https://codereview.chromium.org/23754006/diff/3001/runtime/vm/bigint_operatio...
runtime/vm/bigint_operations.cc:295: }
On 2013/09/17 00:19:52, siva wrote:
> As discussed offline the overflow could happen before the last computation
> making it positive.
> 
> We should probably compute a kMaxChunkLen and compare chunk_length against
that
> and error out.

Done.

https://codereview.chromium.org/23754006/diff/3001/runtime/vm/bigint_operatio...
runtime/vm/bigint_operations.cc:369: if (required_size !=
static_cast<intptr_t>(required_size)) {
On 2013/09/17 00:19:52, siva wrote:
> Ditto comment here too.

Removed the check, because it's already there (as far as I see) :)
line 345: kMaxAllowedDigitLength (conservative estimate)

One more thing: The code here (line 350+) throws an OOM exception, while the new
checks use the FATAL() macro. Change the new checks to throw an OOM exception,
or leave them?

[sra1, 2013-09-17 05:32:30]

dbc

https://codereview.chromium.org/23754006/diff/8001/runtime/vm/bigint_operatio...
File runtime/vm/bigint_operations.cc (right):

https://codereview.chromium.org/23754006/diff/8001/runtime/vm/bigint_operatio...
runtime/vm/bigint_operations.cc:111: if (str_length < 0) {
I don't understand the purpose of this test.

If str_length < 0 then str_length > 2 below will fail and the else branch calls
a function that has its own validation.

[Michael Lippautz (Google), 2013-09-17 17:10:34]

Thanks!

https://codereview.chromium.org/23754006/diff/8001/runtime/vm/bigint_operatio...
File runtime/vm/bigint_operations.cc (right):

https://codereview.chromium.org/23754006/diff/8001/runtime/vm/bigint_operatio...
runtime/vm/bigint_operations.cc:111: if (str_length < 0) {
On 2013/09/17 05:32:31, sra1 wrote:
> I don't understand the purpose of this test.
> 
> If str_length < 0 then str_length > 2 below will fail and the else branch
calls
> a function that has its own validation.

Replaced with a comment saying why we don't need a check.

[siva, 2013-09-17 17:32:21]

lgtm

[Michael Lippautz (Google), 2013-09-18 01:16:14]

Committed patchset #5 manually as r27589 (presubmit successful).