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

Don't use array.push(...spread) with unknown input #7848

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

DingoEatingFuzz
Copy link
Contributor

Using array.push(...spread) is quite nice and convenient, but unfortunately providing too many arguments to a function call can cause a stack overflow, which happened to us.

Minimum reproduction Twiddle

Also unfortunately, using Array#concat changes the API, since it returns a new array rather than modifying the original. Also also unfortunately, simply iterating over an array and pushing one-by-one is significantly slower than using spread or concat.

Performance tests JSBin

This implementation is a hybrid approach of looping and spreading that remains stack safe.

If we can guarantee that using concat is safe, that'd be an even better implementation since it's both part of the standard lib and also faster.

@runspired
Copy link
Contributor

Using concat here should be safe

@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue 🎯 canary PR is targeting canary (default) labels Jan 27, 2022
@snewcomer
Copy link
Contributor

@DingoEatingFuzz Is this something you would like to push across the line with concat? Happy to help if you don't have time as well.

@DingoEatingFuzz
Copy link
Contributor Author

Hey @snewcomer, I'm working on updating the branch right now. Should be done today 🙂

@DingoEatingFuzz DingoEatingFuzz force-pushed the bug-array-spread branch 3 times, most recently from 8398e11 to 3f2bfaa Compare January 29, 2022 21:49
Using array.push(...spread) is unsafe when used with unknown input.
Providing too many arguments to any function call can cause a stack
overflow.
Copy link
Contributor

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok! tests in the console seem to verify!

const source = new Array(130000);
const input = [];
input(...source);

@snewcomer snewcomer merged commit bb83d8b into emberjs:master Feb 2, 2022
@DingoEatingFuzz DingoEatingFuzz deleted the bug-array-spread branch February 2, 2022 19:25
mmun pushed a commit to Addepar/ember-data that referenced this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎯 canary PR is targeting canary (default) 🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants