Bug 80465 – Integer overflow check code in arithmetic operation in classic interpreter

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[SangGyu Lee]

Classic Interpreter checks whether integer arithmetic operation will cause overflow in privateExecute().

Currently the check code is like followings:

    DEFINE_OPCODE(op_add) {
        /* add dst(r) src1(r) src2(r)

           Adds register src1 and register src2, and puts the result
           in register dst. (JS add may be string concatenation or
           numeric add, depending on the types of the operands.)
        */
        int dst = vPC[1].u.operand;
        JSValue src1 = callFrame->r(vPC[2].u.operand).jsValue();
        JSValue src2 = callFrame->r(vPC[3].u.operand).jsValue();
        if (src1.isInt32() && src2.isInt32() && !(src1.asInt32() | (src2.asInt32() & 0xc0000000))) // no overflow            callFrame->uncheckedR(dst) = jsNumber(src1.asInt32() + src2.asInt32());
        else {
            JSValue result = jsAdd(callFrame, src1, src2);
            CHECK_FOR_EXCEPTION();
            callFrame->uncheckedR(dst) = result;
        }


I can't understand the test condition

   ... && !(src1.asInt32() | (src2.asInt32() & 0xc0000000))) // no overflow

If src1 is not zero, it would falling to else condition (jsAdd).

For simple example, both src1 and src2 is 1, it fails the condition:

   ... && !((src1.asInt32() | src2.asInt32()) & 0xc0000000))


I wonder if is a mistake, and the original intention is 

   ... && !((src1.asInt32() | src2.asInt32()) & 0xc0000000))


Please see the change in parenthesis.

If both of src1 and src2 have most two significant bits as zero, no overflow can occurs.
Although, it is conservative, but is fast and simple check.

op_sub has same condition.


I have same question about op_mul.


        if (src1.isInt32() && src2.isInt32() && !(src1.asInt32() | src2.asInt32() >> 15)) // no overflow


the test expression should be like followings:


 ... && !( (src1.asInt32() | src2.asInt32()) >> 15)) // no overflow


Please see added parenthesis. 
Since bit-shift operator's precedence is high than bit OR, parenthesis is necessary.

If both src1 and src2 have most 16 significant bits as zero, no overflow can occurs.

[Gavin Barraclough]

(In reply to comment #0)
> I wonder if is a mistake, and the original intention is 

Ouch, yes, I think you're right.

> Although, it is conservative, but is fast and simple check.

Yes, that is the intention.

> If both src1 and src2 have most 16 significant bits as zero, no overflow can occurs.

Yes, again I think you're right.

One detail, actually you need the top 17 bits to be zero.  Multiplying two 16 bit values requires nearly the full 32-bit range (2^16 - 1)^2 == 2^32 - 2^17 + 1, so cannot be  represented with a signed integer.

[SangGyu Lee]

Created attachment 131266 [details]
Patch

[Gavin Barraclough]

Comment on attachment 131266 [details]
Patch

See my last comment, changing the value '15' to '16' is incorrect, you will fail to detect overflow in the case of values such as 0xFFFF * 0xFFFF.

[SangGyu Lee]

I am sorry for my mistake. I incorrectly assumed the operand is a unsigned number. As your kind and concise comment, 16 bit is not enough for the cases like 0xFFFF x 0xFFFF.

[SangGyu Lee]

Created attachment 131274 [details]
Patch

[Gavin Barraclough]

Comment on attachment 131274 [details]
Patch

Sorry, still not quite there! - we need to check the top 17 bits are all zero - we can ignore the bottom 15 bits.  So the shift count needs to be 15, not 17!

[SangGyu Lee]

I am sorry, and thank you for fixing my stupid mistake.

[SangGyu Lee]

Created attachment 131299 [details]
Patch

[Gavin Barraclough]

Comment on attachment 131299 [details]
Patch

Looks good, great catch - thanks for fixing this.

[WebKit Review Bot]

Comment on attachment 131299 [details]
Patch

Clearing flags on attachment: 131299

Committed r110443: <http://trac.webkit.org/changeset/110443>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.