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

[WIP] Shallow harden #1914

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

[WIP] Shallow harden #1914

wants to merge 3 commits into from

Conversation

mhofman
Copy link
Contributor

@mhofman mhofman commented Dec 29, 2023

refs: #1686

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Quick review on mobile, I like where this is headed!

@@ -64,14 +66,15 @@ let priorRepairIntrinsics;
/** @type {Error=} */
let priorHardenIntrinsics;

// @ts-expect-error harden always defined
if (globalThis.harden) {
deleteProperty(globalThis, 'harden');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, what happens with captured references to the pre-lockdown harden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we need to figure out the story for this. Right now I'm simply removing any harden found, like in the case of our xsnap-worker.

@@ -300,6 +315,16 @@ export const repairIntrinsics = (options = {}) => {
optGetStackString,
);
globalThis.console = /** @type {Console} */ (consoleRecord.console);
// eslint-disable-next-line no-underscore-dangle
if (typeof (/** @type {any} */ (globalThis.console)._times) === 'object') {
hostIntrinsics.SafeMap = getPrototypeOf(
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs more explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, just getting it to pass first. But basically NodeJS has hidden intrinsics, supposed to be internal only but one of them leaks through an "underscore" property of their console.

},
hardenIntrinsics(root) {
intrinsicsWereHardened = true;
return harden(root);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can harden throw? Might need afinally here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it can, and no need for finally. The goal for now is to leave harden permanently broken. We can discuss whether we want this situation to be resolvable somehow.

});
}
}
} else if (setGetSize(protosToCheck) > 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If intrinsics were note hardened, we should fail for any objects we intend to harden, but that were already in the hardenedAtLockdown set. (enforcing the rule that a prototype must be hardened before its instances)

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

The vast majority of of changes in this PR are just generic improvements in use of harden. If you could separate these into a distinct PR, we could probably review and merge that quickly. After that, this PR would also be much smaller and easier to review.

@mhofman
Copy link
Contributor Author

mhofman commented Dec 30, 2023

@erights the use of harden are already in separate commits, but hold off anyway, I'm more thoroughly rewriting the harden implementation to add hooks.

@erights erights self-requested a review December 30, 2023 23:11
@erights
Copy link
Contributor

erights commented Dec 30, 2023

What about Agoric/agoric-sdk#8700 ? It, likewise, seems to be almost all improvements in the use of harden which would be valuable regardless, and could get merged early. Yes?

@mhofman
Copy link
Contributor Author

mhofman commented Jan 9, 2024

The vast majority of of changes in this PR are just generic improvements in use of harden. If you could separate these into a distinct PR, we could probably review and merge that quickly.

@erights I have now done that in #1939. While I have rebased this PR on top of that change (roughly), I do expect to rewrite it significantly to add hooks to fully separate harden from lockdown

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

Now that #1939 is merged, please rebase this one for review brevity. Thx.

@mhofman
Copy link
Contributor Author

mhofman commented Jan 10, 2024

Now that #1939 is merged, please rebase this one for review brevity. Thx.

It's stacked by apparently GH is struggling and didn't update the base ...

@erights
Copy link
Contributor

erights commented Jan 10, 2024

I think if you rebase -i onto current (post merge) master and push --force, that should clean it up. yes?

harden no longer hardens prototypes. Post lock-down asserts that prototypes are already hardened
Add a isHardened predicate which checks if an object is hardened
@erights erights self-requested a review January 20, 2024 04:22
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.

None yet

3 participants