Author: esprehn
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/7dc7c0fc073b84f3050726b52ceec1bbcb304ba3
Time: Fri Jun 10 04:35:59 2016
The CL last changed line 74 of file StringToNumber.cpp, which is stack frame 0.

@esprehn: Request you to please take a look into it. Please help us to reassign if not with respect to your change.

Thanks.!

The test case is just:
<ol start=-2147483648>

That regression range doesn't make sense, I don't know why it thinks that range. Maybe the clang roll?

Anyway CF thinks it's the code here:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/text/StringToNumber.cpp?sq=package:chromium&type=cs&l=74

        value = base * value + digitValue;

I'm not sure if this one is worth fixing, all browsers I tested (Safari, Chrome, Firefox) give the same output from doing document.querySelector("ol").start which is -2147483648 so they all convert the value into the same thing, which is the same as the text value. So if we overflow we apparently still get the right result.

That said this loop has this code in it:

        if (value > maxMultiplier || (value == maxMultiplier && digitValue > (integralMax % base) + isNegative))
            goto bye;

which seems like it's trying to compensate for overflow already? Maybe jyasskin@ or yutak@ know if we're doing something wrong here.

If value == 214748364 and digitValue == 8 == 2147483647 % 10 + 1, then we don't goto bye, and we run value = base * value + digitValue, which overflows 2147483648 to -2147483648. Then the 'if (isNegative)' block negates that to its original value, which is another undefined overflow.

What would you recommend doing to fix it? :)

We could copy the range checking code from base:
https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.cc?rcl=1470439186&l=226

we could also just switch to base's converter and use the StringPiece or StringPiece16 methods, but we'd need to fix the whitespace checks to be locale independent:

https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.cc?rcl=1470439186&l=95

They're using isspace and iswspace, and we can't do that since it changes the web exposed behavior.

+1 for re-using code from base/.

https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h isn't designed to depend on the user's locale, so it's probably an improvement to remove naïve uses of isspace.

Dupe of 634796 ?

Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot