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

Unable to import 'tape' v4 or v5 after lockdown #293

Closed
warner opened this issue May 9, 2020 · 19 comments
Closed

Unable to import 'tape' v4 or v5 after lockdown #293

warner opened this issue May 9, 2020 · 19 comments
Assignees

Comments

@warner
Copy link
Contributor

warner commented May 9, 2020

This program:

// install-ses.js consists of:
//  import { lockdown } from 'ses';
//  lockdown();
import './install-ses.js';
import { test } from 'tape';

when run against SES-0.7.7, with a node_modules that has tape-4.11.0, fails like this:

$ node -r esm fail-import-tape.js
/home/warner/stuff/agoric/agoric-sdk/node_modules/string.prototype.trim/node_modules/es-abstract/helpers/callBind.js:1
TypeError: Cannot assign to read only property 'apply' of function 'function callBind() {
	return bind.apply($call, arguments);
}'
    at Object.<anonymous> (/home/warner/stuff/agoric/agoric-sdk/node_modules/string.prototype.trim/node_modules/es-abstract/helpers/callBind.js:15:22)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1196:10)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1196:10)

I tried disabling various taming options in lockdown() and it didn't help.

I'm guessing that tape is using some module (es-abstract?) which thinks it can modify the primordials. We could find a way to accomodate that, or find a way to make it stop.

I'm able to import tape before lockdown, and tests seem to work normally, which is what I'll do for now. That makes it a 'vetted shim', though, and it's neither vetted nor a shim. Discuss.

@erights
Copy link
Contributor

erights commented May 9, 2020

This is the override mistake. It is not an attempt to modify a primordial. The relevant code in callBind.js is

module.exports = function callBind() {
	return bind.apply($call, arguments);
};

module.exports.apply = function applyBind() {
	return bind.apply($apply, arguments);

This is merely trying to create an own apply property on the callBind function, which inherits from Function.prototype. This own property would override the Function.prototype.apply. When lockdown makes Function.prototype.apply a non-writable data property, due to the override mistake, that causes an assignment that would merely override the property to fail.

At https://github.com/Agoric/SES-shim/blob/master/packages/ses/src/enablements.js is our whitelist of primordial properties for loackdown to repair, prior to hardening the primordials, so that these properties act as if JS had no override mistake. Under FunctionPrototype we see it repairs bind but not apply.

Try adding apply to this whitelist. This might not work because apply itself may be involved in the mechanism by which the override mistake is fixed. But try it first. If it seems to work it probably does.

@erights
Copy link
Contributor

erights commented May 9, 2020

Repairing apply, even if it works, is expensive. Long term, better would be to push a change upstream to tape to create the callBind.apply property by using defineProperty semantics rather than assignment semantics.

@warner
Copy link
Contributor Author

warner commented May 12, 2020

I think I ran into the identity-discontinuity problem when importing tape (4.11.0) before lockdown(). Tape has an assertion named t.throws(), which takes a closure and asserts that running it throws an exception. The following program:

import { test } from 'tape';
import './install-ses.js';

function parse() {
  throw TypeError('Error parsing');
}

test('parse', t => {
  t.throws(parse, /Error parsing/, 'expected failure');
  t.end();
});

fails with:

not ok 1 expected failure
  ---
    operator: throws
    expected: '/Error parsing/'
    actual:   [TypeError]

and I believe it's because tape is stashing the original Error object, and using it to somehow examine the exception that was raised (which uses a different Error object as its base). I can replace tape's t.throws with the built-in assert module's assert.throws and the test works normally.

@warner
Copy link
Contributor Author

warner commented May 12, 2020

I tried upgrading to tape-5.0.0 (released just a few weeks ago) to see if it helps anything. I get the same error when attempting to install SES first, then import tape. And unfortunately I get a new problem when importing tape before lockdown(): when t.throws is called, I get this error:

TypeError <Object <Object <[Object: null prototype] {}>>>: Cannot assign to read only property 'message' of object ''
    at Test.throws (/home/warner/stuff/agoric/agoric-sdk/packages/SwingSet/node_modules/tape/lib/test.js:584:25)
    at Test.bound [as throws] (/home/warner/stuff/agoric/agoric-sdk/packages/SwingSet/node_modules/tape/lib/test.js:84:32)

which feels like another instance of the override mistake, as tape is (for some reason I don't understand) replacing the error's message with itself:

(tape/lib/test.js around line 584)

try {
  fn();
} catch (err) {
  caught = { error: err };
  if (Object(err) === err && (!isEnumerable(err, 'message') || !has(err, 'message'))) {
    var message = err.message;
    delete err.message;
    err.message = message;
  }
}

@erights
Copy link
Contributor

erights commented May 12, 2020

Is fn at #293 (comment) the same as parse at #293 (comment) ? I see it throws a TypeError

I see at https://github.com/Agoric/SES-shim/blob/23d21d185f3a6e067b44ae9f5652136916510efe/packages/ses/src/enablements.js#L78 that we repair Error.prototype.message but not TypeError.prototype.message.

We should add to the whitelist a repair of message for all the builtin error types.

On the identity discontinuity, the only good solution is to load tape after lockdown(), so I'm glad we're headed in that direction.

@erights
Copy link
Contributor

erights commented May 12, 2020

As you expand this whitelist, please continue the tradition of adding a comment saying what case triggered the need for each additional repair.

@warner
Copy link
Contributor Author

warner commented May 12, 2020

Yes, I believe fn should be the same as parse.

warner added a commit to Agoric/agoric-sdk that referenced this issue May 14, 2020
* disable typescript checking, which used a trick to swap 'harden' out for
  one that was annotated, but now 'harden' is a (frozen) global and cannot be
  swapped out like that
* replace tape's t.throws with one from node's built-in 'assert' module,
  since tape's version is broken by the identity discontinuity that results
  from importing tape before lockdown() (and importing it *after* lockdown()
  runs into an override mistake problem, see
  endojs/endo#293 for details)
@warner
Copy link
Contributor Author

warner commented May 15, 2020

Adding apply: t // set by "tape" to the enablements list allows tape (4.11.0) to be imported after lockdown(), without the TypeError: Cannot assign to read only property 'apply' of function 'function callBind() error. However t.throws still fails. I'll explore further.

@warner
Copy link
Contributor Author

warner commented May 15, 2020

Adding apply: t and TypeErrorPrototype.message: t to enablements, calling lockdown(), and then importing tape-5.0.0 (which I'm testing in parallel) gets me to this error:

$ node -r esm fail-tape-types.js
/home/warner/stuff/agoric/agoric-sdk/node_modules/define-properties/index.js:1
TypeError: Cannot define property getPolyfill, object is not extensible
    at defineProperty (<anonymous>)
    at defineProperty (/home/warner/stuff/agoric/agoric-sdk/node_modules/define-properties/index.js:34:3)
    at defineProperties (/home/warner/stuff/agoric/agoric-sdk/node_modules/define-properties/index.js:52:3)
    at Object.<anonymous> (/home/warner/stuff/agoric/agoric-sdk/node_modules/object.assign/index.js:11:1)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1196:10)

I'm still investigating what it's trying to do. The code in object.assign is:

'use strict';

var defineProperties = require('define-properties');

var implementation = require('./implementation');
var getPolyfill = require('./polyfill');
var shim = require('./shim');

var polyfill = getPolyfill();

defineProperties(polyfill, {
	getPolyfill: getPolyfill,
	implementation: implementation,
	shim: shim
});

module.exports = polyfill;

and it's the defineProperties call that's triggering the failure. That polyfill variable renders as:

[Function: assign] Function <Function <[Object: null prototype] {}>>

The export of ./polyfill is:

module.exports = function getPolyfill() {
	if (!Object.assign) {
		return implementation;
	}
	if (lacksProperEnumerationOrder()) {
		return implementation;
	}
	if (assignHasPendingExceptions()) {
		return implementation;
	}
	return Object.assign;
};

which means it's trying to modify properties of the (frozen) Object.assign, as some sort of default case or reaction to Object.assign not being present or behaving in some bad way.

Feels like a shim gone wrong.

@warner
Copy link
Contributor Author

warner commented May 15, 2020

Adding FunctionPrototype.apply: t and TypeErrorPrototype.message: t to enablements, calling lockdown(), and then importing tape-4.13.2, appears to work normally, including t.throws. I'll add a PR for that.

Still don't know how to make tape-5.0.0 work.

warner added a commit that referenced this issue May 15, 2020
This adds `FunctionPrototype.apply`, as well as `.message` for all the
various Error prototypes, to the enablements list.

This allows tape-4.x to be imported after lockdown, as well as fixing tape's
t.throws() method.

refs #293, but does not close it because tape-5.x is still broken
warner added a commit that referenced this issue May 15, 2020
This adds `FunctionPrototype.apply`, as well as `.message` for all the
various Error prototypes, to the enablements list.

This allows tape-4.x to be imported after lockdown, as well as fixing tape's
t.throws() method.

refs #293, but does not close it because tape-5.x is still broken
warner added a commit that referenced this issue May 15, 2020
This adds `FunctionPrototype.apply`, as well as `.message` for all the
various Error prototypes, to the enablements list.

This allows tape-4.x to be imported after lockdown, as well as fixing tape's
t.throws() method.

refs #293, but does not close it because tape-5.x is still broken
warner added a commit that referenced this issue May 15, 2020
This adds `FunctionPrototype.apply`, as well as `.message` for all the
various Error prototypes, to the enablements list.

This allows tape-4.x to be imported after lockdown, as well as fixing tape's
t.throws() method.

refs #293, but does not close it because tape-5.x is still broken
warner added a commit that referenced this issue May 15, 2020
This adds `FunctionPrototype.apply`, as well as `.message` for all the
various Error prototypes, to the enablements list.

This allows tape-4.x to be imported after lockdown, as well as fixing tape's
t.throws() method.

refs #293, but does not close it because tape-5.x is still broken
warner added a commit that referenced this issue May 15, 2020
This adds `FunctionPrototype.apply`, as well as `.message` for all the
various Error prototypes, to the enablements list.

This allows tape-4.x to be imported after lockdown, as well as fixing tape's
t.throws() method.

refs #293, but does not close it because tape-5.x is still broken
warner added a commit that referenced this issue May 15, 2020
This repairs the override mistake in `FunctionPrototype.apply`, as well as `.message` for all the various Error prototypes.

This allows tape-4.x to be imported after lockdown, as well as fixing tape's t.throws() method.

refs #293, but does not close it because tape-5.x is still broken
@erights
Copy link
Contributor

erights commented May 17, 2020

From the code you show, I think you diagnosed the tape 5 problem correctly. Is the following summary correct?

This is not the override mistake. Rather, it is an attempt to directly modify a primordial in a way that is publicly visible, that modifies the primordial API surface in a way that does not serve to shim and proposed standard. Specifically, it will make the following properties visible on the Object.assign method: getPolyfill, implementation, and shim. This is not part of any proposed standard for enhancing Object.assign. It is probably not their intention to make any such proposal. Thus, this is simply a severe bug in tape 5. We should report it to them.

If we are in a hurry to switch to tape 5, we should fork, repair, and then send them a PR citing our bug report. Otherwise we should just wait for them to fix the bug.

@erights
Copy link
Contributor

erights commented Jul 11, 2020

Moved from "Backlog" to "Reassess" to determine whether we still care about Tape 5.

@erights erights changed the title unable to import 'tape' after lockdown unable to import 'tape' v5 after lockdown Jul 19, 2020
@kriskowal kriskowal changed the title unable to import 'tape' v5 after lockdown Unable to import 'tape' v5 after lockdown Sep 29, 2020
@kriskowal
Copy link
Member

cc @ljharb This is the issue that we encountered while using tape under ses. This motivated our prior solution, fixing overrides.

@warner
Copy link
Contributor Author

warner commented Sep 29, 2020

I just checked briefly, the same error is currently reported in both tape-4.13.3 and tape-5.0.1, running against ses-0.10.4

$ cat t.js
import '@agoric/install-ses'; // calls lockdown()
import { test } from 'tape';

test('demo', t => {
  t.ok(true);
  t.end();
});
$ node -r esm t.js
[/home/warner/stuff/agoric/agoric-sdk/packages/temp/node_modules/es-abstract/helpers/callBind.js:1
TypeError: Cannot assign to read only property 'apply' of function 'function callBind() {
	return $reflectApply(bind, $call, arguments);
}'
  at Object.<anonymous> (/home/warner/stuff/agoric/agoric-sdk/packages/temp/node_modules/es-abstract/helpers/callBind.js:15:22)
  at Object.Module._extensions..js (internal/modules/cjs/loader.js:1272:10)
  at Object.Module._extensions..js (internal/modules/cjs/loader.js:1272:10)]

It looks like the specific assignment is happening in es-abstract, here:

$ cat node_modules/es-abstract/helpers/callBind.js
'use strict';

var bind = require('function-bind');

var GetIntrinsic = require('../GetIntrinsic');

var $apply = GetIntrinsic('%Function.prototype.apply%');
var $call = GetIntrinsic('%Function.prototype.call%');
var $reflectApply = GetIntrinsic('%Reflect.apply%', true) || bind.call($call, $apply);

module.exports = function callBind() {
	return $reflectApply(bind, $call, arguments);
};

module.exports.apply = function applyBind() {
	return $reflectApply(bind, $apply, arguments);
};

@warner warner changed the title Unable to import 'tape' v5 after lockdown Unable to import 'tape' v4 or v5 after lockdown Sep 29, 2020
@kriskowal
Copy link
Member

In summary, we discussed this at the Agoric SES meeting, and we do care about about tape versions running under SES.

@ljharb
Copy link

ljharb commented Sep 29, 2020

I plan to publish a fixed object.assign, and es-abstract, later this week. Hopefully that will also include running their full test suite both with and without SES, to prevent future regressions.

@erights
Copy link
Contributor

erights commented Sep 30, 2020

Does the closing of ljharb/object.assign#79 mean we can now close this one too? Who wants to check?

(Also, @ljharb , thanks for ljharb/object.assign#79 !)

@ljharb
Copy link

ljharb commented Sep 30, 2020

It should! I've published es-abstract as both v1.17.7 and v1.18.0-next.1, so all dep graphs should automatically update to include the fix, modulo lockfiles.

@erights
Copy link
Contributor

erights commented Feb 18, 2021

@warner @kriskowal I'm closing this as irrelevant, but reopen if I'm missing something or you disagree.

@erights erights closed this as completed Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants