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

RESOLVED FIXED in mozilla34

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla34
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8472715 [details] [diff] [review]
fix-non-primitives

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.
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
Backed out the original patch, new patch in a sec:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c891c7fa1a3
(Assignee)

Comment 5

4 years ago
Created attachment 8473125 [details] [diff] [review]
fix

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)
(Assignee)

Comment 6

4 years ago
Created attachment 8473140 [details] [diff] [review]
fix

Oops, proxies.
Attachment #8473125 - Attachment is obsolete: true
Attachment #8473125 - Flags: review?(jwalden+bmo)
Attachment #8473140 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 7

4 years ago
Created attachment 8473145 [details] [diff] [review]
fix

qref'd
Attachment #8473140 - Attachment is obsolete: true
Attachment #8473140 - Flags: review?(jwalden+bmo)
Attachment #8473145 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

4 years ago
Created attachment 8473146 [details] [diff] [review]
fix

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)
(Assignee)

Comment 9

4 years ago
Created attachment 8473204 [details] [diff] [review]
fix2

... 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
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 14

4 years ago
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!
(Assignee)

Comment 15

4 years ago
(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
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.