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

Fix Array.prototype.toString() stackoverflow #1976

Merged
merged 6 commits into from
Oct 6, 2024

Conversation

xBaank
Copy link
Contributor

@xBaank xBaank commented Oct 4, 2024

Closes #1975

{
return value.IsNullOrUndefined()
return value.IsNullOrUndefined() || thisObject == value
Copy link
Collaborator

Choose a reason for hiding this comment

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

for clarity, I think this should be ReferenceEquals(thisObject, value), the operator overloads can be a bit quirky

@xBaank
Copy link
Contributor Author

xBaank commented Oct 5, 2024

I modified the code because Array.prototype.toString.call((c = [1, 2, 3, 4], b = [1, 2, 3, 4], b[1] = c, c[1] = b, c)) could still produce a StackOverflow. Now it checks if the array is already visited.

@lahma
Copy link
Collaborator

lahma commented Oct 5, 2024

This will make join calls very costly as now every time another array is being created, or did I misunderstand?

@xBaank
Copy link
Contributor Author

xBaank commented Oct 5, 2024

This will make join calls very costly as now every time another array is being created, or did I misunderstand?

Yes, maybe a better aproach would be to pass down the visited hashset along with thisObject and arguments so no arrays are created?

@lahma
Copy link
Collaborator

lahma commented Oct 5, 2024

I haven't dug deep into this problem, but I would assume that other types would also be problematic, not just concrete JsArrays if called with different thisObject. Like objects having indexes pointing to circular references, or to interop array-like wrappers.

@lahma
Copy link
Collaborator

lahma commented Oct 6, 2024

By looking at V8 code, it seems that at least following are vulnerable to cyclic behavior (V8 has safeguards):

  • ArrayPrototype.join
  • ArrayPrototype.toLocaleString
  • TypedArrayPrototype.join
  • TypedArrayPrototype.toLocaleString

@lahma
Copy link
Collaborator

lahma commented Oct 6, 2024

There's ObjectTraverseStack that could probably be used for this, adding TryEnter which wouldn't throw like the Enter does (Enter would TryEnter and throw).

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

Looking good, thank you for taking the time iterating!

@lahma lahma merged commit 0eee47d into sebastienros:main Oct 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants