fix(ses)!: Withdraw support for muli-lockdown #921

kriskowal · 2021-10-29T23:09:02Z

BREAKING CHANGE: Lockdown can no longer be called more than once. Lockdown no longer returns a boolean to indicate whether it was effective (true) or redundant (false). Instead, Lockdown will return undefined for its first invocation or throw an error otherwise.

Fixes #814

mhofman

Looks fine by me.

Reviewer nit: I usually try to review commit-by-commit, so when code is moved from one file to another, I prefer to have that as a separate commit if new code is added or left behind in the remaining file. For remain behind, I usually try to temporarily rename the source file if most of the content is extracted to another file so that the large moving content appears as a rename in git, and the small remaining content a new file.

mhofman · 2021-10-29T23:31:22Z

packages/ses/NEWS.md

+- *BREAKING CHANGE*: Lockdown may no longer be called more than once.
+  Lockdown no longer returns a boolean to indicate whether it was effective
+  (true) or redundant (false).


Should this mention an error is thrown if trying to lockdown twice? Seems weird to be explicit about former behavior, but not about new one.

Explicated this. Thanks.

I assume we should update the PR description too?

erights

LGTM

erights · 2021-10-30T21:20:08Z

packages/ses/src/lockdown-shim.js

@@ -53,7 +53,8 @@ import { makeCompartmentConstructor } from './compartment-shim.js';

 const { details: d, quote: q } = assert;

-let firstOptions;
+/** @type {Error?} */
+let priorLockdown = null;


Why null rather than undefined?

Suggested change

let priorLockdown = null;

let priorLockdown;

I don't have any strong reason for preferring one over the other. But when there is no other reason, I generally default to undefined.

erights · 2021-10-30T21:23:46Z

packages/ses/error-codes/SES_ALREADY_LOCKED_DOWN.md

+## Related
+
+This error is distinct from `SES_MULTIPLE_INSTANCES`, which attempts to
+distinguish the case where multiple Lockdown functions have been initialized.


The phrasing here is that the other error happens when "multiple Lockdown functions have been initialized", which might be a fine condition to detect and consider an error. However, the explanation in SES_MULTIPLE_INSTANCES.md below explains that error as indicating that multiple calls to lockdown occurred, where the calls were to different instances. From that text I would guess that multiple initializations, where only one is called, would not be an error.

erights · 2021-10-30T21:40:02Z

packages/ses/src/lockdown-shim.js

+  if (priorLockdown !== null) {
+    const error = new TypeError(
+      `Already lockded down (SES_ALREADY_LOCKED_DOWN)`,
+    );
+    // eslint-disable-next-line @endo/no-polymorphic-call
+    assert.note(error, assert.details`Prior call ${priorLockdown}`);
+    throw error;
  }


Suggested simplification. Use only the parts, if any, that you agree are an improvement

Suggested change

if (priorLockdown !== null) {

const error = new TypeError(

`Already lockded down (SES_ALREADY_LOCKED_DOWN)`,

);

// eslint-disable-next-line @endo/no-polymorphic-call

assert.note(error, assert.details`Prior call ${priorLockdown}`);

throw error;

}

assert(

priorLockdown === null,

X`Already locked down at ${priorLockdown} (SES_ALREADY_LOCKED_DOWN)`,

TypeError,

);

assuming an earlier

const { details: X } = assert;

Btw, I really like this technique of capturing the earlier non-erroneous stack trace in an error in case of a later conflict.

erights · 2021-10-30T21:44:44Z

packages/ses/src/lockdown-shim.js

  if (seemsToBeLockedDown()) {
    // eslint-disable-next-line @endo/no-polymorphic-call
-    console.log('Seems to already be locked down. Skipping second lockdown');
-    return alreadyHardenedIntrinsics;
+    throw new TypeError(
+      `Already locked down but not by this SES instance (SES_MULTIPLE_INSTANCES)`,
+    );
  }


This makes clear which of the two conditions we treat as an error: where both lockdowns are called. This seems sensible.

erights · 2021-10-30T21:46:23Z

packages/ses/src/lockdown-shim.js

+   * We define hardenIntrinsics here so that options are in scope, but return
+   * it to the caller so we can benchmark repair separately from hardening.


The other motivation is to prepare for vetted shims, which would run after repair and before hardening. In fact, this was the original and primary motivation.

mhofman · 2021-10-30T22:43:09Z

packages/ses/src/lockdown-shim.js

-    domainTaming,
-    __allowUnsafeMonkeyPatching__,
-  };
+  priorLockdown = new TypeError('Prior lockdown (SES_ALREADY_LOCKED_DOWN)');


FYI this will keep the scope of this function and of its predecessors in the stack around since engines (well at least v8) lazily generate the stack string. While this probably happens early enough in the program execution, it might be worth "touching" the priorLockdown.stack for good measure?

Really? I have no direct knowledge, but I would have kept only a stack of PC addresses.

Apparently JSC automatically generates the stack string if the scope would otherwise have been collected: https://twitter.com/filpizlo/status/1365140028247007234

Maybe v8 could perform some optimization in the case of strict functions and only keep the function name, and location around for Error.prepareStackTrace. If you look at the Twitter thread, they apparently ran into some other perf issues.

*BREAKING CHANGE*: Lockdown can no longer be called more than once. Lockdown no longer returns a boolean to indicate whether it was effective (true) or redundant (false). Fixes #814

kriskowal requested review from erights and mhofman October 29, 2021 23:09

kriskowal force-pushed the 814-once-lockdown branch 3 times, most recently from 60e6264 to 139ac27 Compare October 29, 2021 23:24

mhofman reviewed Oct 29, 2021

View reviewed changes

mhofman approved these changes Oct 30, 2021

View reviewed changes

erights approved these changes Oct 30, 2021

View reviewed changes

mhofman reviewed Oct 30, 2021

View reviewed changes

kriskowal added 6 commits November 1, 2021 15:51

fix(ses)!: Withdraw support for muli-lockdown

6a94a9b

*BREAKING CHANGE*: Lockdown can no longer be called more than once. Lockdown no longer returns a boolean to indicate whether it was effective (true) or redundant (false). Fixes #814

fix: Explicate new lockdown signature in NEWS

f9ab9d6

fix: Simplify prior lockdown detector

4ee56e8

fix: Clarify motivation for defering harden intrinsics

1a8abbb

fix: Release lockdown closures

a772eda

fix: Prose fix for multiple lockdown instances called

6bf5b9a

kriskowal force-pushed the 814-once-lockdown branch from fbb1a10 to 6bf5b9a Compare November 1, 2021 23:24

kriskowal merged commit 99752b0 into master Nov 1, 2021

kriskowal deleted the 814-once-lockdown branch November 1, 2021 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ses)!: Withdraw support for muli-lockdown #921

fix(ses)!: Withdraw support for muli-lockdown #921

kriskowal commented Oct 29, 2021 •

edited

Loading

mhofman left a comment

mhofman Oct 29, 2021

kriskowal Oct 30, 2021

mhofman Oct 30, 2021

erights left a comment

erights Oct 30, 2021

erights Oct 30, 2021

erights Oct 30, 2021

erights Oct 30, 2021

erights Oct 30, 2021

erights Oct 30, 2021

mhofman Oct 30, 2021

erights Oct 30, 2021

mhofman Oct 30, 2021

		* We define hardenIntrinsics here so that options are in scope, but return
		* it to the caller so we can benchmark repair separately from hardening.

fix(ses)!: Withdraw support for muli-lockdown #921

fix(ses)!: Withdraw support for muli-lockdown #921

Conversation

kriskowal commented Oct 29, 2021 • edited Loading

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Oct 29, 2021 •

edited

Loading