TM: record_JSOP_IN can guard on wrong condition

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
7 years ago
6 years ago

People

(Reporter: Igor Bukanov, Unassigned)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(blocking2.0 .x+)

Details

(Reporter)

Description

7 years ago
During recording record_JSOP_IN calls obj->lookupProperty() to get the condition to guard the js_HasNamedProperty invocation on trace. Since obj->lookupProperty can point to a proxy during the recording time that can return a different result from what is expected leading to a wrong guard. The following test demonstrates the problem:

function f() {
    var obj = {x: 1};
    var proxy_was_called = false;
    var proxy = Proxy.create({has: function() {
	if (proxy_was_called)
	    return true;
	proxy_was_called = true;
	return false;
    }});
    
    var array = [obj, obj, obj, obj, obj, obj, obj, proxy, obj, obj];
    
    var s = 0;
    for (var i = 0; i != 10; ++i) {
	if ("x" in array[i])
	    ++s;
    }

    assertEq(s, 9);
}

f();

With jit enabled the last assert is violated as due to the use of lookupProperty the presence of the jit is not transparent.

I close the bug since I do not know if there are implications that are more serious than just bad semantics of user scripts.
(Reporter)

Comment 1

7 years ago
It seems that TraceRecorder::test_property_cache has the same issue with non-transparent jit as it also calls js_LookupPropertyWithFlags that may run proxied code via proxy on the prototype.
(Reporter)

Comment 2

7 years ago
There are quite a few comments and code like 

// js_FindPropertyHelper can reenter the interpreter and kill |this|
if (!localtm.recorder)
    return ARECORD_ABORTED;

I suppose all such places can be used to detect the jit recording and lead to jit evaluation that is different from non-jit one.

Another observation is that with a proxy coming from a different compartment we may end up with running arbitrary scripts under the recording and continue the recording after that finishes. Presumably this is safe.
Whiteboard: [sg:critical?]

Updated

7 years ago
blocking2.0: --- → ?
For now, just looks like it would cause incorrect output with Proxy objects.
blocking2.0: ? → .x
Whiteboard: [sg:critical?]
Opening up, per comment 3.
Group: core-security
Obsolete with the removal of tracejit. FWIW, when I change the AssertEq to line to just print s, I get 9 with Interp, JM, and JM+TI.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.