648029 - Side effects of name resolution are out-of-order

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Edwin Smith]

Property access (get/set/call/delete) with a multiname that has runtime values (name or namespace) involves resolving the runtime value into a name, which can call toString() or valueOf(), showing side effects.

ergo, name resolution has side effects, in general.

The interpreter and JIT execute the side effects in different order relative to the null check and argument coersion.

for OP_callproperty: (and likely others, I haven't done a full audit):
jit:  nullcheck, initMultiname, coerce args
interpreter: initMultiname, nullcheck, coerce args.

Here's a test case

  function f(object, name) {
    object[name](1)
  }
  var name = { toString: function() { print('tostring'); return "foo" } }
  f(null, name)

When JIT compiled, it just throws the NPE.  In the interpreter, it prints 'tostring' then throws the NPE, showing the early name resolution.

The order of values on the abc stack is:  object [ns] [name] args..., so if you go by that, the evaluation order ought to be:

  nullcheck object, [toString(name)], coerce(args)...

If we agree, then the JIT is okay and we need to fix the interpreter.  We also need to run through the other opcodes and build test cases for everything.

Comment 1

[Edwin Smith]

This bug applies to the -super opcodes as well (getsuper, setsuper, callsuper).  Not only is then nullcheck apparently out of order but coerce of the receiver object also needs to be made consistent.

Comment 2

[William Maddox]

Attachment: AS3 utilities for abcasm test

Comment 3

[William Maddox]

Attachment: Patch to 'abcasm' to allow runtime names in more contexts

This patch allows runtime names in more contexts than previously allowed.  Note that 'abcasm' is incomplete, and there are TODO's in the code for dynamic names, e.g., regarding stack depth accounting.  I would not rely on this patch any further than the test case at hand.

Comment 4

[William Maddox]

Attachment: Test case for ordering of runtime name side-effects (abcasm)

To run:
avmshell coerce_test_helper.abc coerce_rtname_test.abc

Comment 5

[William Maddox]

Attachment: Result of running test with 32-bit debugger shell

Shows what normal output looks like, with informative error diagnostics on exceptions.  Note that many property accesses that were not intended to fail nonetheless have issues -- the test needs work.  Nonetheless, we can see when
the coercion side-effects happen relative to the other effects.

Comment 6

[William Maddox]

Attachment: Diff between results with interpreter and JIT

Results from 32-bit debug (non-debugger) shell.  64-bit gives the same results.

Comment 7

[William Maddox]

Comment on attachment 603611 [details] [diff] [review]
Diff between results with interpreter and JIT

Mark diff as patch for more readable presentation in Bugzilla.

Comment 8

[William Maddox]

Attachment: In the interpreter, null-check receiver before coercing runtime name

By inspection, there looks to be a pre-existing bug in initMultinameNoXMLList(), noted in a comment.  I have not yet investigated this further.

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 603627 [details] [diff] [review]
In the interpreter, null-check receiver before coercing runtime name

Review of attachment 603627 [details] [diff] [review]:
-----------------------------------------------------------------

::: core/Interpreter.cpp
@@ +2343,3 @@
>                      multiname = &aux_memory->multiname2;
>                  }
>                  sp[0] = env->getdescendants(sp[0], multiname);

IIUC, before your change, a null on this path would have caused a kDescendentsError.  With your change, it is now a kConvertNullToObjectError.

Am I right?  If so, was this change planned/on-purpose?  (If so, I'll leave it up to you whether this warrants the introduction of version-checks and the corresponding alternate paths and/or changes to your helper functions' interfaces.)

@@ +2346,5 @@
>                  NEXT;
>              }
>  
>              INSTR(checkfilter) {
>                  SAVE_EXPC;

A couple cases below here we have the INSTR(findproperty) case, which is the only one left that still calls the bare initMultiname with no null check.

That's the case where the global object is pushed on the stack, right?  (I'm trying to do a quick inference based on the doxygen docs for findproperty.)  I wouldn't mind a one-line comment above the call saying that, so someone casually reading the interpreter code won't think that the bare initMultiname was an oversight.

@@ +3644,5 @@
>              name.setNamespace(env->internRtns(*(sp--)));
>  
>          return sp;
>      }
>  

Refactoring suggestion: The bodies of initMultiname and checkNullAndInitMultiname are (rightfully) nearly identical.

I don't see any reason not to create a shared routine with an extra boolean flag (that signals whether to do the env->nullcheck(*sp) calls, and then make both functions call out to the new shared routine.

(But I trust you to make the right call here, if you have some reason not to do this.)

@@ +3692,5 @@
> +        return sp;
> +    }
> +
> +    // FIXME: It appears that isXMLList(sp[0]) is intended to operate on the base object,
> +    // but this will not be the case if there is a runtime name or namespace!

Create a Bugzilla ticket to track this (unless one already exists, I haven't checked), and change the comment to point to the ticket.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.