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

LibJS: Avoid potential overflow in Array.prototype.toSpliced() #14466

Merged
merged 1 commit into from
Jul 3, 2022

Conversation

linusg
Copy link
Member

@linusg linusg commented Jul 2, 2022

No description provided.

The implementation no longer matches the spec text, but I believe that's
a bug anyway. No point in allowing array lengths up to 2^53 - 1 when the
ArrayCreate AO rejects anything above 2^32 - 1.
@linusg linusg requested a review from IdanHo July 2, 2022 23:45
Comment on lines +1927 to 1931
// FIXME: ArrayCreate throws for any length > 2^32 - 1, so there's no point in letting
// values up to 2^53 - 1 through (spec issue). This also prevents a potential
// overflow when casting from double to size_t, which is 32 bits on x86.
if (new_length_double > NumericLimits<u32>::max())
return vm.throw_completion<TypeError>(global_object, ErrorType::ArrayMaxSize);
Copy link
Member

Choose a reason for hiding this comment

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

That's a good point, generally Array prototype methods allow up to MAX_ARRAY_LIKE_INDEX and not just 2^32 - 1, since they are supposed to be generic, so you can use them on other objects like strings that do not have this limitation, but these toX class of methods create arrays, so they do have to use the smaller maximum, which is the array maximum size.

Copy link
Member Author

Choose a reason for hiding this comment

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

@linusg linusg merged commit ab2574d into SerenityOS:master Jul 3, 2022
@linusg linusg deleted the libjs-fix-overflow branch July 3, 2022 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants