[webkit-reviews] review denied: [Bug 187042] RegExp.exec returns wrong value with a long integer quantifier : [Attachment 344045] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Sukolsak Sakshuwong
<sukolsak at gmail.com>'s request for review:
Bug 187042: RegExp.exec returns wrong value with a long integer quantifier
https://bugs.webkit.org/show_bug.cgi?id=187042

Attachment 344045: Patch

https://bugs.webkit.org/attachment.cgi?id=344045&action=review




--- Comment #3 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 344045
  --> https://bugs.webkit.org/attachment.cgi?id=344045
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344045&action=review

r- because a JSC test is failing.  Please investigate as to why it's failing.

> Source/JavaScriptCore/yarr/YarrParser.h:990
> -	   // check for overflow.
> -	   for (unsigned newValue; peekIsDigit() && ((newValue = n * 10 +
peekDigit()) >= n); ) {
> -	       n = newValue;
> +	   while (peekIsDigit()) {
> +	       unsigned nextDigit = peekDigit();
> +	       if (n > (UINT_MAX - nextDigit) / 10) {
> +		   // Overflow. Skip past remaining decimal digits and return
UINT_MAX.
> +		   do {
> +		       consume();
> +		   } while (peekIsDigit());
> +		   return UINT_MAX;
> +	       }
> +	       n = n * 10 + nextDigit;
>	       consume();
>	   }
>	   return n;

Please look into using Checked<unsigned, RecordOverflow> (grep for other places
where this is used to see how it's used).  You should use that here instead. 
It will simplify this code tremendously.
Also, to be consistent with the rest of Yarr code, you should return
quantifyInfinite here instead of UINT_MAX directly.