Closed Bug 1053544 Opened 5 years ago Closed 5 years ago

OdinMonkey: issue link-time validation error when given non-primitive import value

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(2 files, 4 obsolete files)

Attached patch fix-non-primitives (obsolete) — Splinter Review
Otherwise, we could run an object.valueOf() observably twice, first in ToInt32() when linking, then again when we re-execute as bytecode after a link failure.
Attachment #8472715 - Attachment is patch: true
Attachment #8472715 - Flags: review?(benj)
Comment on attachment 8472715 [details] [diff] [review]
fix-non-primitives

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

Classic valueOf :)

::: js/src/jit-test/tests/asm.js/testGlobals.js
@@ +118,5 @@
>  assertEq(asmLink(asmCompile('global', 'imp', USE_ASM + "var i=+imp.i; function f() { return +i } return f")(this, {i:1.4})), 1.4);
>  assertEq(asmLink(asmCompile('global', 'imp', USE_ASM + "const i=+imp.i; function f() { return +i } return f")(this, {i:1.4})), 1.4);
>  assertEq(asmLink(asmCompile(USE_ASM + "var g=0; function f() { var i=42; while (1) { break; } g = i; return g|0 } return f"))(), 42);
> +assertAsmLinkFail(asmCompile('glob','foreign', USE_ASM + 'var i = +foreign.x; function f() {} return f'), null, {x:{valueOf:function() { return 42 }}});
> +assertAsmLinkFail(asmCompile('glob','foreign', USE_ASM + 'var i = foreign.x|0; function f() {} return f'), null, {x:{valueOf:function() { return 42 }}});

Bonus points if you add one test with the float32 coercion.
Attachment #8472715 - Flags: review?(benj) → review+
Well, it turns out Emscripten has a bug that passes a function as an int import and this shows up in various tests out there.  So I'm going to weaken the check to only fail link-time validation if the object has valueOf/toString.
Backed out the original patch, new patch in a sec:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c891c7fa1a3
Attached patch fix (obsolete) — Splinter Review
Waldo: can you confirm that this check does what the comment says it does?
Assignee: nobody → luke
Attachment #8472715 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8473125 - Flags: review?(jwalden+bmo)
Attached patch fix (obsolete) — Splinter Review
Oops, proxies.
Attachment #8473125 - Attachment is obsolete: true
Attachment #8473125 - Flags: review?(jwalden+bmo)
Attachment #8473140 - Flags: review?(jwalden+bmo)
Attached patch fix (obsolete) — Splinter Review
qref'd
Attachment #8473140 - Attachment is obsolete: true
Attachment #8473140 - Flags: review?(jwalden+bmo)
Attachment #8473145 - Flags: review?(jwalden+bmo)
Attached patch fixSplinter Review
Ok, sorry about all these; this time with proxies, qref'd and without commenting out half of testGlobals.js.
Attachment #8473145 - Attachment is obsolete: true
Attachment #8473145 - Flags: review?(jwalden+bmo)
Attachment #8473146 - Flags: review?(jwalden+bmo)
Attached patch fix2Splinter Review
... and since I started thinking about proxies, I realized that GetDataProperty(), despite its intentions not to call any user JS, can, in the case of proxies.  We actually had tests for this, but they were coded to indirect proxies and so long ago stopped testing.
Attachment #8473204 - Flags: review?(efaustbmo)
Comment on attachment 8473204 [details] [diff] [review]
fix2

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

I agree. r=me
Attachment #8473204 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/mozilla-central/rev/6a72436c8a75
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Merge of backout (which apparently never got commented about here):
https://hg.mozilla.org/mozilla-central/rev/5c891c7fa1a3
Target Milestone: mozilla34 → ---
Comment on attachment 8473146 [details] [diff] [review]
fix

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

Despite luke's best efforts to explain, it remains unclear to me why this must-be-data-property/must-be-non-object constraint need be imposed on the initialization of asm.js global variables, and as a spec matter require/impose a link failure.  But this does seem to do the trick, correctly, so r+.

::: js/src/asmjs/AsmJSLink.cpp
@@ +132,5 @@
> +            // ToNumber/ToInt32 definitely produces NaN/0. We should remove this
> +            // special case later once most apps have been built with newer
> +            // Emscripten.
> +            jsid toString = NameToId(cx->names().toString);
> +            if (!v.isObject() ||

!v.isPrimitive() is exactly identical to !v.isObject(), so this check is vacuous.  I would probably convert the top-level check to just v.isObject(), rather than discussing the asm.js behavior in terms of its operation on "primitives".

::: js/src/jsobjinlines.h
@@ +758,5 @@
> +
> +// Return whether looking up 'valueOf' on 'obj' resolves to the original
> +// Object.prototype.valueOf.
> +static MOZ_ALWAYS_INLINE bool
> +HasObjectValueOf(JSContext *cx, JSObject *obj)

cx second because infallible.

@@ +760,5 @@
> +// Object.prototype.valueOf.
> +static MOZ_ALWAYS_INLINE bool
> +HasObjectValueOf(JSContext *cx, JSObject *obj)
> +{
> +    JS_ASSERT(!obj->is<ProxyObject>());

Probably an assert of isNative() would be good, seeing as HasDataProperty requires this.  Just for clarity.

@@ +768,5 @@
> +    Value v;
> +    while (!HasDataProperty(cx, obj, valueOf, &v)) {
> +        obj = obj->getProto();
> +        if (!obj || obj->is<ProxyObject>())
> +            return false;

Paranoia probably counsels either adding in isNative() here, or replacing the proxy check with that.
Attachment #8473146 - Flags: review?(jwalden+bmo) → review+
Thanks, and good suggestions.

To be a bit more clear: one of the goals of the link-time constraints is to allow an implementation, when "executing" a call to the module function, to simply walk down the list of validation criteria and, at any point, if there is a failure, bail out, reparse the entire asm.js module from scratch, and then call the reparsed module.  If evaluating any of the validation criteria were to cause a user-visible side effect, then calling the reparsed module would run that side effect twice.  You were suggesting avoiding this problem by executing the module function in the interpreter.  This however has causes significant pain: on validation failure we now have this FrankenJSScript that we can't keep running as is (it's nested functions don't have JSScripts and the current compiled code can't be soundly run) so we either have to mutate the FrankenJSScript or swap in a new JSScript, mid-script.  Also, the outer asm.js module has >150,000 bindings (so bindings would take a bunch of memory) and would require various other hacks to allow it to be emitted by the normal emitter.  ¡Ay caramba!
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> !v.isPrimitive() is exactly identical to !v.isObject(), so this check is
> vacuous.  I would probably convert the top-level check to just v.isObject(),
> rather than discussing the asm.js behavior in terms of its operation on
> "primitives".

Ah, I remember my reasoning for this arrangement: the spec for the ToNumber coercions starts with a ToPrimitive() coercion which is the only place where user-visible side-effects can occur.  Requiring a "primitive" is to say that this coercion is a noop.  (Also, it avoids the corner case of null technically being an object.)  Removing the inner v.isObject() sounds good, though.
https://hg.mozilla.org/mozilla-central/rev/cb0080422f7b
https://hg.mozilla.org/mozilla-central/rev/e7d606eeb52c
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.