Rework `xsnap/src/object-inspect.js` #4767

michaelfig · 2022-03-08T19:57:28Z

closes: #2146
closes: #4814

Description

This is the next step toward improving the code used by xsnap to provide a working console.log.

Security Considerations

Improves the security of our xsnap process by providing a vetted shim that really has been vetted.

Documentation Considerations

Testing Considerations

dckc

this is how far I got before the dinner bell rang...

packages/xsnap/test/test-promise-reject.js

dckc · 2022-03-09T00:26:42Z

packages/xsnap/src/object-inspect.js

+const mapSize = Object.getOwnPropertyDescriptor(Map.prototype, 'size').get;
+const mapForEach = Map.prototype.forEach;
+const setSize = Object.getOwnPropertyDescriptor(Set.prototype, 'size').get;
+const setForEach = Set.prototype.forEach;
+const weakMapHas = WeakMap.prototype.has;
+const weakSetHas = WeakSet.prototype.has;
+const weakRefDeref = WeakRef.prototype.deref;
+const booleanValueOf = Boolean.prototype.valueOf;
+const objectToString = Object.prototype.toString;
+const functionToString = Function.prototype.toString;
+const $match = String.prototype.match;
+const $slice = String.prototype.slice;
+const $replace = String.prototype.replace;
+const $toUpperCase = String.prototype.toUpperCase;
+const $test = RegExp.prototype.test;
+const $concat = Array.prototype.concat;
+const $join = Array.prototype.join;
+const bigIntValueOf = BigInt.prototype.valueOf;
+const getOwnPropertySymbols = Object.getOwnPropertySymbols;
+const symToString = Symbol.prototype.toString;
+// ie, `has-tostringtag/shams
+const toStringTag = Symbol.toStringTag;
+const isEnumerable = Object.prototype.propertyIsEnumerable;
+const dateToISOString = Date.prototype.toISOString;
+
+const gPO = Reflect.getPrototypeOf;
+const hasOwn = Object.prototype.hasOwnProperty;


Why alias all this stuff? I have some vague ideas, but I don't understand well enough to maintain this list. In particular, the mapSize and setSize contortions.

Is this meant to guard against some other vetted shim mutating one of the standard objects? I like defensive, but is that still in the threat model?

Is this meant to guard against some other vetted shim mutating one of the standard objects? I like defensive, but is that still in the threat model?

This is a light refactoring of the original object-inspect.js code. I didn't rewrite it further because I didn't see the need to.

In a locked down environment this shouldn't be necessary. If we do want to adopt this style for this file, maybe we could enable the eslint rule to disable polymorphic calls like the one endo uses: endojs/endo#827

packages/xsnap/src/object-inspect.js

erights · 2022-03-10T01:24:33Z

@FUDCo , @michaelfig is here doing a permissive unprivileged object-quotation system much like I originally wanted to do for q (aka assert.quote). I didn't do that because, as far as I remember, you really wanted the output of q to be within JSON and eventually convinced me to do that. IIUC, you do not recall lobbying for that and are in fact indifferent to the JSON-ability of the q output. I'm just giving background. I don't care what really happened historically.

With that as background, could you confirm that, once this PR is merged, you would not object to q using its form of quotation?

dckc · 2022-03-10T06:09:21Z

How does test coverage for this version compare with what it replaces? Help me figure it out?

https://agoric-sdk-coverage.netlify.app/packages/xsnap/lib/object-inspect.js.html ?
70% Statements 378/540 49.75% Branches 103/207 77.14% Functions 27/35 70% Lines 378/540

michaelfig · 2022-03-10T18:46:17Z

@erights, let's discuss how to review this:

is this okay as a vetted shim? Are we confident it doesn't reach for powerful things?
what level of coverage do we need? https://agoric-sdk-coverage.netlify.app/packages/xsnap/lib/object-inspect.js.html

erights · 2022-03-10T21:14:52Z

is this okay as a vetted shim? Are we confident it doesn't reach for powerful things?

@michaelfig , Why would it need to be a vetted shim, rather than unprivileged code running under normal post-lockdown SES restrictions?

michaelfig · 2022-03-10T22:00:03Z

is this okay as a vetted shim? Are we confident it doesn't reach for powerful things?

@michaelfig , Why would it need to be a vetted shim, rather than unprivileged code running under normal post-lockdown SES restrictions?

Sorry, my confusion. Yes, it's unprivileged code in the post-lockdown start compartment. I'd like ideally to run it in a compartment, but am unsure about how.

erights · 2022-03-10T22:09:59Z

Yes, it's unprivileged code in the post-lockdown start compartment. I'd like ideally to run it in a compartment, but am unsure about how.

Excellent. Yes, let's find time to discuss. Thanks!

dckc · 2022-03-11T00:15:58Z

Sorry, my confusion. Yes, it's unprivileged code in the post-lockdown start compartment.

Really? It looks to me like we run this to build the console that we give to SES:

agoric-sdk/packages/xsnap/lib/ses-boot.js

Lines 1 to 2 in 17e161e

    
           import './console-shim.js'; 
        
           import '@endo/init';

agoric-sdk/packages/xsnap/lib/console-shim.js

Line 3 in 17e161e

import inspect from '../src/object-inspect.js';

michaelfig · 2022-03-11T03:05:02Z

It looks to me like we run this to build the console that we give to SES:

Ahh, yes. The reason it's a vetted shim is because of initialisation ordering in SES, not because we actually need any of that power.

Instinctively, we should be able to relax that constraint and install the object inspector from a post-lockdown compartment.

dckc · 2022-03-11T03:28:15Z

... initialisation ordering in SES ...

Instinctively, we should be able to relax that constraint and install the object inspector from a post-lockdown compartment.

Yes, we used to have a lo-fi console arg quoter that we used for SES initialization, and then we replaced the lo-fi console quoter with assert.quote. I guess we don't want to go back to exactly that, but something analagous seems workable...

https://github.com/Agoric/agoric-sdk/blob/a3306d01d8e87c4bc7483a61e42cc30b006feb81/packages/xsnap/lib/console-shim.js
https://github.com/Agoric/agoric-sdk/blob/a3306d01d8e87c4bc7483a61e42cc30b006feb81/packages/xsnap/lib/ses-boot.js

erights · 2022-03-11T05:12:27Z

Yes, we used to have a lo-fi console arg quoter that we used for SES initialization, and then we replaced the lo-fi console quoter with assert.quote. I guess we don't want to go back to exactly that, but something analagous seems workable...

https://github.com/Agoric/agoric-sdk/blob/a3306d01d8e87c4bc7483a61e42cc30b006feb81/packages/xsnap/lib/console-shim.js https://github.com/Agoric/agoric-sdk/blob/a3306d01d8e87c4bc7483a61e42cc30b006feb81/packages/xsnap/lib/ses-boot.js

TIL setQuote. I had no idea. Neat!

FUDCo

ya shoor okay

erights

Lots of minor nits but one genuine bug -- where an implementation limitation of the SES shim led you astray.

packages/xsnap/src/object-inspect.js

packages/xsnap/test/test-inspect.js

packages/xsnap/src/object-inspect.js

mhofman · 2022-03-29T00:53:00Z

packages/xsnap/src/object-inspect.js

+const mapSize = Object.getOwnPropertyDescriptor(Map.prototype, 'size').get;
+const mapForEach = Map.prototype.forEach;
+const setSize = Object.getOwnPropertyDescriptor(Set.prototype, 'size').get;
+const setForEach = Set.prototype.forEach;
+const weakMapHas = WeakMap.prototype.has;
+const weakSetHas = WeakSet.prototype.has;
+const weakRefDeref = WeakRef.prototype.deref;
+const booleanValueOf = Boolean.prototype.valueOf;
+const objectToString = Object.prototype.toString;
+const functionToString = Function.prototype.toString;
+const $match = String.prototype.match;
+const $slice = String.prototype.slice;
+const $replace = String.prototype.replace;
+const $toUpperCase = String.prototype.toUpperCase;
+const $test = RegExp.prototype.test;
+const $concat = Array.prototype.concat;
+const $join = Array.prototype.join;
+const bigIntValueOf = BigInt.prototype.valueOf;
+const getOwnPropertySymbols = Object.getOwnPropertySymbols;
+const symToString = Symbol.prototype.toString;
+// ie, `has-tostringtag/shams
+const toStringTag = Symbol.toStringTag;
+const isEnumerable = Object.prototype.propertyIsEnumerable;
+const dateToISOString = Date.prototype.toISOString;
+
+const gPO = Reflect.getPrototypeOf;
+const hasOwn = Object.prototype.hasOwnProperty;


In a locked down environment this shouldn't be necessary. If we do want to adopt this style for this file, maybe we could enable the eslint rule to disable polymorphic calls like the one endo uses: endojs/endo#827

packages/xsnap/src/object-inspect.js

mhofman · 2022-03-29T00:55:24Z

packages/xsnap/src/object-inspect.js

+const gPO = Reflect.getPrototypeOf;
+const hasOwn = Object.prototype.hasOwnProperty;
+
+export default function inspect0(obj, opts = {}, depth = 0, seen = new Set()) {


Nit: s/obj/val

mhofman · 2022-03-29T00:56:38Z

packages/xsnap/src/object-inspect.js

+  if (typeof depth === 'undefined') {
+    depth = 0;
+  }


We're already setting a default above, aren't we?

mhofman · 2022-03-29T01:03:51Z

packages/xsnap/src/object-inspect.js

+  const indent = getIndent(opts, depth);
+
+  if (seen.has(obj)) {
+    return '[Circular]';


Can't duplicated values can also trigger this. E.g.

const obj = {}; inspect0({foo: obj, bar: obj});

mhofman · 2022-03-29T01:49:54Z

packages/xsnap/src/object-inspect.js

+    return false;
+  }
+  try {
+    weakRefDeref.call(x);


This is sad as it forces the target to stay alive another turn

Good point!

Anything I should try to do about that?

I don't see another way. deref is the only thing that brand checks the [[WeakRefTarget]] slot.

mhofman · 2022-03-29T01:51:32Z

packages/xsnap/src/object-inspect.js

+      remaining > 1 ? 's' : ''
+    }`;
+    return (
+      inspectString($slice.call(str, 0, opts.maxStringLength), opts) + trailer


technically this can cut in the middle of a surrogate pair.

Anything I should do about that?

We add a trailer which starts with a non surrogate char, so it may just technically be an invalid string, but all implementations handle malformed strings like that.

@mhofman I don't see how that trailer helps. In the case in question, it would already be invalid as a unicode string.

I think what I meant is that this won't be concatenated with another string which may start with the second half of another surrogate pair, so if we split in the middle, the first half will just be detected as a broken pair, and processed as such by every algorithms which handles strings.

Thanks. I understand now, and it is a valid concern.

packages/xsnap/src/object-inspect.js

mhofman · 2022-03-29T02:01:15Z

packages/xsnap/src/object-inspect.js

+      continue;
+    }
+    if ($test.call(/[^\w$]/, key)) {
+      xs.push(`${inspect(key, obj)}: ${inspect(obj[key], obj)}`);


Confused why obj is included in ${inspect(key, obj)} but is not in the symbol version below: ${inspect(syms[j])}

The symbol cannot possibly be the same as the object, so no need to check it for circularity.

But the symbol's value can refer to the object, can't it?

But the symbol's value can refer to the object, can't it?

As far as I read it, the inspect(syms[j]) inspects the symbol itself, not the symbol property's value.

warner · 2022-04-02T02:20:25Z

y'all are reviewing this better than I could possibly do, I'll defer to you

erights · 2022-04-09T01:18:23Z

packages/xsnap/lib/confined-object-inspect.js

+const src = objectInspectSources.replace(
+  /(^|\s)(export\s+default)(\s+)/g,
+  '$1/* $2 */$3',
+);


OMG. Definitely worth a comment to explain why.

Definitely!

erights · 2022-04-09T01:21:52Z

packages/xsnap/src/object-inspect.js

+      remaining > 1 ? 's' : ''
+    }`;
+    return (
+      inspectString($slice.call(str, 0, opts.maxStringLength), opts) + trailer


@mhofman I don't see how that trailer helps. In the case in question, it would already be invalid as a unicode string.

packages/xsnap/test/test-inspect.js

erights · 2022-04-09T01:38:43Z

packages/xsnap/src/object-inspect.js

+    const registered = symKeyFor(obj);
+    if (registered !== undefined) {
+      // Registered symbol.
+      return `Symbol.for(${registered})`;


packages/xsnap/src/object-inspect.js

erights

LGTM, thanks!

michaelfig added the xsnap the XS execution tool label Mar 8, 2022

michaelfig requested review from dckc and erights March 8, 2022 19:57

michaelfig self-assigned this Mar 8, 2022

dckc reviewed Mar 9, 2022

View reviewed changes

dckc mentioned this pull request Mar 10, 2022

review or replace object-inspect.js used by xsnap console #4814

Closed

michaelfig requested a review from warner March 10, 2022 18:14

michaelfig force-pushed the mfig-xsnap-logging branch from ac9e24a to 0c64c64 Compare March 28, 2022 21:15

michaelfig requested review from kriskowal, FUDCo and mhofman as code owners March 28, 2022 21:15

FUDCo approved these changes Mar 28, 2022

View reviewed changes

michaelfig force-pushed the mfig-xsnap-logging branch from 0c64c64 to 57c121f Compare March 28, 2022 21:49

erights requested changes Mar 29, 2022

View reviewed changes

mhofman reviewed Mar 29, 2022

View reviewed changes

warner removed their request for review April 2, 2022 02:20

erights mentioned this pull request Apr 2, 2022

SES-shim's permissive undeclared variable behavior can lead new code astray endojs/endo#1136

Closed

michaelfig force-pushed the mfig-xsnap-logging branch from 57c121f to 6041179 Compare April 8, 2022 22:43

michaelfig requested a review from erights April 8, 2022 22:44

michaelfig force-pushed the mfig-xsnap-logging branch 2 times, most recently from 116c1f7 to e1f4ff6 Compare April 9, 2022 00:49

erights reviewed Apr 9, 2022

View reviewed changes

erights approved these changes Apr 9, 2022

View reviewed changes

michaelfig added 3 commits April 8, 2022 20:29

fix(xsnap): adopt src/object-inspect.js

4f23da2

feat(xsnap): load object-inspect in its own Compartment

05ceb87

refactor: make confined-object-inspect.js more obvious

4ca22b4

michaelfig force-pushed the mfig-xsnap-logging branch from d78fc36 to a92037a Compare April 9, 2022 02:30

michaelfig added 4 commits April 8, 2022 20:57

refactor(object-inspect): clean up and clarify

67a52d5

test(SwingSet): update golden xsnap images

8fd739a

feat(object-inspect): group bigint digits in threes

a21d3b4

test(object-inspect): expand object graphs even with cyclic nodes

e075ea0

michaelfig force-pushed the mfig-xsnap-logging branch from a92037a to e075ea0 Compare April 9, 2022 02:57

michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Apr 9, 2022

mergify bot merged commit fa068d1 into master Apr 9, 2022

mergify bot deleted the mfig-xsnap-logging branch April 9, 2022 03:13

Rework xsnap/src/object-inspect.js #4767

Rework xsnap/src/object-inspect.js #4767

Conversation

michaelfig commented Mar 8, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

dckc 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

erights commented Mar 10, 2022

dckc commented Mar 10, 2022

michaelfig commented Mar 10, 2022

erights commented Mar 10, 2022

michaelfig commented Mar 10, 2022

erights commented Mar 10, 2022

dckc commented Mar 11, 2022

michaelfig commented Mar 11, 2022

dckc commented Mar 11, 2022

erights commented Mar 11, 2022

FUDCo left a 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

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

warner commented Apr 2, 2022

Choose a reason for hiding this comment

michaelfig Apr 9, 2022 • edited Loading

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

Rework `xsnap/src/object-inspect.js` #4767

Rework `xsnap/src/object-inspect.js` #4767

michaelfig commented Mar 8, 2022 •

edited

Loading

michaelfig Apr 9, 2022 •

edited

Loading