Bug 104967 - javascript integer overflow
Summary: javascript integer overflow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.7
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-13 15:38 PST by Joe Cackler
Modified: 2012-12-18 10:08 PST (History)
3 users (show)

See Also:


Attachments
the patch (14.16 KB, patch)
2012-12-17 15:08 PST, Filip Pizlo
mhahnenberg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Cackler 2012-12-13 15:38:44 PST
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/
Comment 1 Filip Pizlo 2012-12-17 14:07:50 PST
(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).
Comment 2 Filip Pizlo 2012-12-17 15:08:35 PST
Created attachment 179809 [details]
the patch
Comment 3 Mark Hahnenberg 2012-12-17 15:10:39 PST
Comment on attachment 179809 [details]
the patch

r=me
Comment 4 Filip Pizlo 2012-12-17 15:11:50 PST
Landed in http://trac.webkit.org/changeset/137951
Comment 5 Joe Cackler 2012-12-18 10:08:43 PST
Wow, thanks for the speedy resolution!  I've verified that the bug is fixed in r138017.