[Webkit-unassigned] [Bug 80465] New: Integer overflow check code in arithmetic operation in classic interpreter

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=80465

           Summary: Integer overflow check code in arithmetic operation in
                    classic interpreter
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: All
        OS/Version: All
            Status: UNCONFIRMED
          Severity: Minor
          Priority: P2
         Component: JavaScriptCore
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: sg5.lee at samsung.com
                CC: fpizlo at apple.com


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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.