Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hang for fake arrays with huge lengths #207

Closed
NeilFraser opened this issue Mar 6, 2021 · 4 comments
Closed

Hang for fake arrays with huge lengths #207

NeilFraser opened this issue Mar 6, 2021 · 4 comments
Assignees
Labels

Comments

@NeilFraser
Copy link
Owner

The following takes forever to execute:
Array.prototype.lastIndexOf.call({0: true, length: 'Infinity'}, true);
This hangs all browsers, node, and the JS-Interpreter.

@NeilFraser NeilFraser added the bug label Mar 6, 2021
@NeilFraser NeilFraser self-assigned this Mar 6, 2021
@NeilFraser
Copy link
Owner Author

NeilFraser commented Mar 6, 2021

Testing this class of problem in the Chrome/Firefox consoles is tricky, since just typing/pasting the statement without pressing enter crashes the tab, since the console attempts to show a preview of the result as you type.

Other functions which are similarly affected include:

Array.prototype.indexOf.call({0: true, length: 'Infinity'}, false);
Array.prototype.shift.call({0: true, length: 'Infinity'});
Array.prototype.reverse.call({0: true, length: 'Infinity'});
Array.prototype.sort.call({0: true, length: 'Infinity'});

I can't find any pathological behaviour for pop. And push, unshift, splice, slice, and join all appear to be safe since the native interpreter throws either RangeError or TypeError. And every, filter, foreach, map, reduce, reduceRight, some, and toLocaleString are all polyfills, so if they loop forever that's not our problem.

My suggestion is that all non-polyfilled functions (including the ones that apparently don't need it) have something like this added to the top of them:

if (isNaN(Interpreter.legalArrayLength(thisInterpreter.getProperty(this, 'length') || 0))) {
  thisInterpreter.throwException(thisInterpreter.RANGE_ERROR, 'Invalid array length');
}

Worth noting that this solution changes the behaviour in the case of a length getter:

var o = {0: true, get length() {return 'Infinity'}};
alert(Array.prototype.lastIndexOf.call(o, true));

In native JavaScript this hangs the thread (but is trying to return 0). In the current JS-Interpreter it promptly returns -1 (which is wrong). Under the proposed solution it would throw 'Error: Getter not supported in that context' which is consistent with us not supporting getters and setters on arrays. I think this change is positive.

@cpcallen Can I get your LGTM on this approach, as well as your eyes on the equivalent parts of Code City?

NeilFraser added a commit that referenced this issue Mar 6, 2021
NeilFraser added a commit that referenced this issue Mar 10, 2021
This adds support for getters and setters on arrays, while also eliminating infinite loops as detailed in issue #207
@NeilFraser
Copy link
Owner Author

Filed this bug for V8: https://bugs.chromium.org/p/v8/issues/detail?id=11574

@NeilFraser
Copy link
Owner Author

@NeilFraser
Copy link
Owner Author

Filed a bug for Mozilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1699351

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant