Bug 104967 – javascript integer overflow

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joe Cackler]

I stumbled upon a case where it appears that JavaScriptCore's JIT is incorrectly assuming an int32 suffices for a variable that represents a numerical sum.  When the sum grows beyond 2^32-1, the value overflows as if 2^32 were subtracted.  When I enable Web Inspector, the bug goes away, which I why I suspect the JIT compiler's at fault.

I'm on Mac OS 10.7, and I can reproduce the bug consistently on the release version of Safari -- 6.0.2 (7536.26.17) -- as well as on the latest WebKit nightly build.  The bug does NOT affect Safari 5.

Here's a demonstration: http://jsfiddle.net/T86va/

[Filip Pizlo]

(In reply to comment #0)
> I stumbled upon a case where it appears that JavaScriptCore's JIT is incorrectly assuming an int32 suffices for a variable that represents a numerical sum.  When the sum grows beyond 2^32-1, the value overflows as if 2^32 were subtracted.  When I enable Web Inspector, the bug goes away, which I why I suspect the JIT compiler's at fault.
> 
> I'm on Mac OS 10.7, and I can reproduce the bug consistently on the release version of Safari -- 6.0.2 (7536.26.17) -- as well as on the latest WebKit nightly build.  The bug does NOT affect Safari 5.
> 
> Here's a demonstration: http://jsfiddle.net/T86va/

Thanks for reporting this!  Seems like a bug creeped into our backwards flow analysis: we always knew that putting a scoped var ('total' is a scoped var in the context of 'total += num') constituted a value escaping, but the format of the PutScopedVar opcode was changed so that the value being put became operand #3 - but the backward flow analysis wasn't updated.

This goes wrong because our backward flow analysis tried to optimize situations like:

total = (total + num) | 0;

In this case, we know that we don't have to detect overflow on 'total + num' because the only user of the plus will truncate to integer anyway.

On the other hand, what is supposed to happen for:

total = total + num;

is that we observe that 'total + num' is used in a context where the value count be observed in an unconstrained way, since 'total' is a scoped variable and is accessible from places not visible to us at time of compilation.  But it's this part that was broken because of the backward flow bug.

Hence we were assuming that:

- Setting total didn't cause the value to escape (because of the bug).
- The act of setting total does not by itself observe fractional or bigger-than-int values (technically, this is true).
- We could perform the integer add without checking overflow because of the above two assumptions.

The fix is just to make sure that the backward flow analysis is updated to treat the operands of PutScopedVar correctly, and flow the fact that child3 (the value being put) is UsedAsValue (i.e. could be used for we-don't-know-what).

[Filip Pizlo]

Created attachment 179809 [details]
the patch

[Mark Hahnenberg]

Comment on attachment 179809 [details]
the patch

r=me

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/137951

[Joe Cackler]

Wow, thanks for the speedy resolution!  I've verified that the bug is fixed in r138017.