620376 - ArrayToIdVector from jsproxy.cpp should check for operation callback invocations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

ArrayToIdVector from jsproxy.cpp that is used to convert an array-like objects into a vector of ids contains the following code:

     js_GetLengthProperty(cx, obj, &length)
...
     for (jsuint n = 0; n < length; ++n) {
         if (!js_IndexToId(cx, n, idr.addr()))
             return false;
...
     }

Since array-like objects can have holes, the length here can be arbitrary even if the array has no elements. So for a length like 2**32 - 1 the code may loop for very long time without querying for the operation callback invocation. The following test shows this ill-loping in the js shell:

~/m/tm/js/src> cat ~/s/x.js
function test() {
    if (typeof timeout != "function")
	return;

    var p = Proxy.create({ enumerate: function() { return Array(1e9); }});

    timeout(0.001);

    var n = 0;
    for (i in p) { ++n;}
    return n;
}

test();
~/m/tm/js/src> ~/b/js/tmdbg64/js ~/s/x.js
Script runs for too long, terminating.

where the script continue to loop despite the termination flag set.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch adds missing JS_CHECK_OPERATION_LIMIT. It also removes erroneous JS_ReportErrorNumber after the failed js_GetLengthProperty as error should be already reported at that moment.

Comment 2

[Andreas Gal :gal]

Comment on attachment 498741 [details] [diff] [review]
v1

Thanks.

Comment 3

[Igor Bukanov]

Nominating for 2.0: the bug fixes a denial-of-service regression.

Comment 4

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/e30d605097be

Comment 5

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/c166bfc18951 - follow up to fix the landed test to work under browser

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/c166bfc18951

Comment 7

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)