620424 - OP_callstatic's JIT and interpreter side effects are different

Status: RESOLVED FIXED

(Tamarin Graveyard :: Verifier, defect, P2)

Description

[Edwin Smith]

In the verifier, which drives the JIT through the CodeWriter interface, we have:

Verifier case OP_callstatic:
  emitCoerceArgs(m, argc) // coerce each arg -- can throw.
  emitCheckNull(sp-argc)  // check that the object isn't null. can throw.
  coder->writeOp2()       // does the call.

Interpreter case OP_callstatic:
  env->nullcheck(sp[-i2]) // check the object isn't null
  f->coerceEnter(...)     // coerce args, then call

The other call operators first check for null before resolving the call
target, then coercing the args.  Therefore in this case, the bug is that
the verifier calls emitCoerceArgs() and emitCheckNull() out of order.

A test case that demonstrates the problem would have to pass in a legally-
typed, but null, object, as well as illegally-typed arguments.  The JIT
would fail by throwing a coerce error before throwing a null error.

Comment 1

[William Maddox]

It appears that ASC seldom generates 'callstatic' instructions.  Only two of our acceptance tests contain such an instruction: 

test/acceptance/regress/bug_498979.as
test/acceptance/abcasm/nullCheck/CallStatic.abs

The second of these is hand-coded abcasm.  The first can be illustrated by the following similar example:

var _any:*;

function set any(val:*) {
   _any = val;
}

any = 'hello world';          // setter 'any' is called with 'callstatic'

function foo(val:*) {
   _any = val;
}

foo('hello world');          // function 'foo' is called with 'callproperty'

I asked Jeff Dyer about this, and he said the following:

"It appears that the only way to emit OP_callstatic is by invoking a global
getter or setter. If you have to invoke it on a null reference, then you'll
need to use abcasm."

I conclude that it is unlikely that this issue has ever been observed in the field, and, in any event, may simply be fixed without backward compatibility concerns, e.g., versioning.

Comment 2

[Edwin Smith]

Agreed, if we can affirm that callstatic is rare-to-nonexistant in the wild, I'm for a non-versioned fix.  and of course a test case either way.

Comment 3

[William Maddox]

I hacked tr-spicy to print a diagnostic when the verifier sees 'callstatic'.
Using my local Brightspot snapshot and -Dverifyonly, there appear to be no occurrences at all, other than what may be hiding in SWFs that fail to verify.  (This is not uncommon, due to external references that fail to resolve.)  My snapshot was taken a few months ago, when the 2/2010 dataset was the most current.  I notice we've got some new data, and some reorganization of the Brightspot directories.  I'll give the experiment another try when I update my snapshot.

Comment 4

[William Maddox]

Attachment: Test case

This is an 'abcasm' test.  I believe that this issue cannot be reproduced in AS3 code compiled with ASC, as it would be necessary for the global object to be null, if Jeff's claim is correct.

Comment 5

[William Maddox]

Attachment: Trivial (unversioned) fix

Comment 6

[Tamarin Bot]

changeset: 7237:f6f3c2a7ca12
user:      William Maddox <[e-mail]>
summary:   Bug 620424: Order side-effects in OP_callstatic consistently between JIT and interpreter (r=edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/f6f3c2a7ca12