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

Guard makeSafe() against failing speculative calls #16259

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

janbiedermann
Copy link

@janbiedermann janbiedermann commented Jan 8, 2025

What does this PR do?

Fix for #16256

Guards internal:primordeals makeSafe() against failing speculative function calls, which can happen if the prototypes of Set or Map have been modified, e.g. functions added to the prototypes.

@DonIsaac
Copy link
Contributor

DonIsaac commented Jan 8, 2025

I don't think this is the correct approach.

The point of makeSafe is to protect internal js code against prototype pollution. Since JS code is loaded in a very lazy way, it is possible to pollute the prototype before primordials are instantiated by polluting before any dependent internal module e.g. require("node:assert").

In the short term, you should be able to fix your linked bugs by making sure you import the internal modules you need before prototype pollution occurs. On our end, we may need to take a more defensive approach and eagerly instantiate SafeMap and SafeSet on startup.

cc @paperclover

@DonIsaac DonIsaac added the node.js Compatibility with Node.js APIs label Jan 8, 2025
@janbiedermann
Copy link
Author

Alright, thanks for the quick response. I will experiment with that idea (early import of some modules) and report back. Gimme a few days or so.

@paperclover
Copy link
Member

js code should just never mutate the global prototype, because it always makes the code more confusing. since that ask is unfortunately not realistic, here is how to fix SafeSet:

I noticed that Bun already has the ability to use Set.prototype and Map.prototype without being affected by prototype pollution, WebKit already snapshots things on startup for their own builtins (see WebKit: SetPrototype::finishCreation for example)

for us to use these, we can edit replacements.ts and add Map and Set to the list export const globalsToPrefix = [.

for all SafeSet/SafeMap usages, replace with new Set / new Map, and then replace all method calls to prefix with $, so set.has -> set.$has. this is equivilent to in JSC internals they always use new @Sap and set.@has. our builtins use $ because it is part of a valid identifier and we are okay with running a light preprocessor.

then SafeSet and SafeMap can be deleted. this is a more robust and performant solution to the primordials.js file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node.js Compatibility with Node.js APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants