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

change "harden" to share a WeakMap of already-frozen objects #195

Closed
warner opened this issue Jul 28, 2018 · 6 comments
Closed

change "harden" to share a WeakMap of already-frozen objects #195

warner opened this issue Jul 28, 2018 · 6 comments

Comments

@warner
Copy link
Contributor

warner commented Jul 28, 2018

Our current def() function currently does too much work: it doesn't remember what's been frozen already, so it will re-freeze things like Function.prototype every time. To fix this, deepFreeze() needs to be turned into a "Freezer" object that retains a WeakMap of everything it has ever frozen, def() should take a Freezer, and the def() exposed as a global should close over the Freezer and deliver it here. deepFreezePrimordials() should use that same Freezer

This isn't a security issue, merely a performance one.

@kumavis
Copy link
Member

kumavis commented Jan 5, 2019

can you use Object.isFrozen(obj)

@erights
Copy link
Contributor

erights commented Jan 5, 2019

No, because Object.isFrozen is not transitive. If def stopped at a frozen object, it could miss non-frozen objects that were reachable from named properties of that frozen object:

const bad = Object.freeze([{}]);
// At this point, we have a frozen array containing 
// a non-frozen object in its transitive API surface.
def(bad);

If the call to def succeeds, then the transitive API surface starting at bad must be frozen. But an isFrozen check would stop too early.

@erights
Copy link
Contributor

erights commented Jan 13, 2019

In addition to labeling this an "enhancement" I also label it a "bug" because it is a huge performance bug. Normally, if there is no deviation from correct overt behavior I avoid "bug". But the performance shortfall here is huge.

@erights
Copy link
Contributor

erights commented Jan 13, 2019

Noting again here that we're likely to change terminology from "deeply frozen" and such to "hardened".

@erights erights changed the title change deepFreeze to share a WeakMap of already-frozen objects change "harden" to share a WeakMap of already-frozen objects Feb 6, 2019
@dckc
Copy link
Contributor

dckc commented Feb 18, 2019

This weakmap seems to be global mutable state. Why is that ok in this case?

Also, is it shared between realms? Between calls to require('harden')?

@warner
Copy link
Contributor Author

warner commented Feb 21, 2019

This weakmap seems to be global mutable state. Why is that ok in this case?

Also, is it shared between realms? Between calls to require('harden')?

I'm going to open a separate issue to discuss this, I think we have an answer but I don't want to lose visibility when I close this as fixed in a few minutes.

warner referenced this issue in Agoric/SES Feb 21, 2019
* remove `Nat` and `def` from the global environment #45
* provide a helper function named `s.makeRequire()` to build a `require`
  endowment. This can be configured to enable `require('@agoric/nat')` or
  `require('@agoric/harden')` (among others), so the same code can work
  either inside or outside of a SES realm. For details of its configuration,
  see the comments in the commit which landed it. #13
* harden() comes from `@agoric/make-hardener`, which doesn't climb
  prototype/inheritance chains, but does complain if the prototype wasn't
  already known to harden(). This avoids the "Ice-9" freeze-the-world
  problem, and also serves to signal when an object from one realm is passed
  into the harden() of a different realm. #15
* harden() now shares a WeakSet of previously-hardened objects #4
* use harden() instead of def() #39
* SES no longer depends upon Nat, but uses it during unit tests. Client code
  that wants Nat should use `require('@agoric/nat')`. #45
* Include AsyncIteratorPrototype in the set of anonIntrinsics #58
* use eslint to format all SES code
@jfparadis jfparadis transferred this issue from Agoric/SES Feb 21, 2020
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

No branches or pull requests

4 participants