Skip to content

Commit

Permalink
refactor(ses): remove unused expensive diagnostic from harden (#2235)
Browse files Browse the repository at this point in the history
## Description

All this `path` maintenance within `harden` was there in case we
encountered failures to harden that were hard to diagnose. They
accumulate the path accumulated from the harden root to the thing that
failed to be hardened. But it was never used to produce a better error
message. Rather, they could have been useful from a debugger's eye view.
But in all the years we've paid the price for this diagnostic, we have
not made use of it. The operations used to accumulate the path are
expensive string operations and WeakSet operations.

Because the diagnostic information was never actually shown in any
diagnostic, this change should be completely without any observable
effect. Hence, this PR is a pure refactor.

### Security Considerations

Mostly none. By making `harden` cheaper, perhaps it'll be used more,
which would help security. But we have not generally avoided harden due
to its runtime cost, with the exception of the SwingSet kernel.

### Scaling Considerations

The point.

Note though that on XS we're already using the native harden that does
not pay this price. The SwingSet kernel `harden` is already suppressed.
So this PR only affects use of `harden` not on XS and not within the
SwingSet kernel itself.

@gibson042 , before merging this, I'd like to work with you to quantify
its performance impact. If it has none, perhaps it isn't work doing and
we should keep the diagnostic info, just in case?

### Documentation Considerations

none. 

### Testing Considerations

none

### Compatibility Considerations

none

However, I noticed this while working on #2232 because I had to modify
the same code for different purposes. There will be minor merge
conflicts coming between these two PRs which I'll resolve after one of
them is merged.

### Upgrade Considerations

none

- ~[ ] Includes `*BREAKING*:` in the commit message with migration
instructions for any breaking change.~
- ~[ ] Updates `NEWS.md` for user-facing changes.~
  • Loading branch information
erights committed Apr 29, 2024
1 parent d27a0cc commit 31f055b
Showing 1 changed file with 5 additions and 13 deletions.
18 changes: 5 additions & 13 deletions packages/ses/src/make-hardener.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import {
Set,
String,
TypeError,
WeakMap,
WeakSet,
globalThis,
apply,
Expand All @@ -45,8 +44,6 @@ import {
setHas,
toStringTagSymbol,
typedArrayPrototype,
weakmapGet,
weakmapSet,
weaksetAdd,
weaksetHas,
FERAL_STACK_GETTER,
Expand Down Expand Up @@ -147,15 +144,13 @@ export const makeHardener = () => {
*/
harden(root) {
const toFreeze = new Set();
const paths = new WeakMap();

// If val is something we should be freezing but aren't yet,
// add it to toFreeze.
/**
* @param {any} val
* @param {string} [path]
*/
function enqueue(val, path = undefined) {
function enqueue(val) {
if (!isObject(val)) {
// ignore primitives
return;
Expand All @@ -171,7 +166,6 @@ export const makeHardener = () => {
}
// console.warn(`adding ${val} to toFreeze`, val);
setAdd(toFreeze, val);
weakmapSet(paths, val, path);
}

/**
Expand All @@ -196,13 +190,11 @@ export const makeHardener = () => {

// get stable/immutable outbound links before a Proxy has a chance to do
// something sneaky.
const path = weakmapGet(paths, obj) || 'unknown';
const descs = getOwnPropertyDescriptors(obj);
const proto = getPrototypeOf(obj);
enqueue(proto, `${path}.__proto__`);
enqueue(proto);

arrayForEach(ownKeys(descs), (/** @type {string | symbol} */ name) => {
const pathname = `${path}.${String(name)}`;
// The 'name' may be a symbol, and TypeScript doesn't like us to
// index arbitrary symbols on objects, so we pretend they're just
// strings.
Expand All @@ -215,10 +207,10 @@ export const makeHardener = () => {
// whether 'value' is present or not, which tells us for sure that
// this is a data property.
if (objectHasOwnProperty(desc, 'value')) {
enqueue(desc.value, `${pathname}`);
enqueue(desc.value);
} else {
enqueue(desc.get, `${pathname}(get)`);
enqueue(desc.set, `${pathname}(set)`);
enqueue(desc.get);
enqueue(desc.set);
}
});
};
Expand Down

0 comments on commit 31f055b

Please sign in to comment.