Bug 129768 – llint_slow_path_check_has_instance() should not adjust PC before accessing operands

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

[Mark Lam]

When evaluating "a instanceof b" where b is an object that ImplementsHasInstance and OverridesHasInstance (e.g. a bound function), the LLINT will take the slow path llint_slow_path_check_has_instance(), and execute a code path that does the following:
1. Adjusts the byte code PC to the jump target PC.
2. For the purpose of storing the result, get the result registerIndex from the 1st operand using the PC as if the PC is still pointing to op_check_has_instance bytecode.

The result is that whatever value resides after where the jump target PC is will be used as a result register value.  Depending on what that value is, the result can be:
1. the code coincidently works correctly
2. memory corruption
3. crashes

The fix is to only adjust the byte code PC after we have stored the result.

ref: <rdar://problem/15394423>

[Mark Lam]

Created attachment 225934 [details]
the patch.

[Mark Hahnenberg]

You should audit the other uses of LLINT_RETURN to make sure we're not doing this in any other places.

[Mark Lam]

(In reply to comment #2)
> You should audit the other uses of LLINT_RETURN to make sure we're not doing this in any other places.

Already did.  I audited adjustments made to the pc and made sure that they did not precede a LLINT_RETURN().

[WebKit Commit Bot]

Attachment 225934 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:125:  __r_returnValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

(In reply to comment #3)
> (In reply to comment #2)
> > You should audit the other uses of LLINT_RETURN to make sure we're not doing this in any other places.
> 
> Already did.  I audited adjustments made to the pc and made sure that they did not precede a LLINT_RETURN().

Cool :-)

[Mark Hahnenberg]

Comment on attachment 225934 [details]
the patch.

r=me

[WebKit Commit Bot]

Comment on attachment 225934 [details]
the patch.

Clearing flags on attachment: 225934

Committed r165162: <http://trac.webkit.org/changeset/165162>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.