Skip to content

Commit

Permalink
fix(store): reject empty-object keys which might not retain identity
Browse files Browse the repository at this point in the history
When #2018 changes the interpretation of `harden({})` to become pass-by-copy,
any code which was previously using that to make a "handle" will break,
because the things they send will be treated as pass-by-copy. To catch cases
where this is happening, we forbid such keys from being used in
store/weakstore. Clients should use `Far('interfacename')` to create handles,
and these will be accepted by store/weakstore as keys.
  • Loading branch information
warner committed Mar 10, 2021
1 parent 1fa9b54 commit 21ab960
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 5 deletions.
3 changes: 2 additions & 1 deletion packages/store/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@
},
"homepage": "https://github.com/Agoric/agoric-sdk#readme",
"dependencies": {
"@agoric/assert": "^0.2.2"
"@agoric/assert": "^0.2.2",
"@agoric/marshal": "^0.3.2"
},
"devDependencies": {
"@agoric/install-ses": "^0.5.2",
Expand Down
20 changes: 20 additions & 0 deletions packages/store/src/helper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// @ts-check

import { getInterfaceOf } from '@agoric/marshal';

/**
* Helper function to reject keys which are empty objects but not marked as
* Remotable. This is intended to catch code which uses harden({}) (which
* will become pass-by-copy, see #2018) as a "handle" or "marker object"
* when they should have used Far().
*
* @param { unknown } key
*/
export function isEmptyNonRemotableObject(key) {
return (
typeof key === 'object' &&
Reflect.ownKeys(key).length === 0 &&
getInterfaceOf(key) === undefined
);
}
harden(isEmptyNonRemotableObject);
12 changes: 11 additions & 1 deletion packages/store/src/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// @ts-check

import { assert, details as X, q } from '@agoric/assert';
import { isEmptyNonRemotableObject } from './helper';

/**
* Distinguishes between adding a new key (init) and updating or
Expand All @@ -21,21 +22,30 @@ export function makeStore(keyName = 'key') {
assert(!store.has(key), X`${q(keyName)} already registered: ${key}`);
const assertKeyExists = key =>
assert(store.has(key), X`${q(keyName)} not found: ${key}`);
const assertNotBadKey = key =>
assert(!isEmptyNonRemotableObject(key), X`${q(keyName)} bad key: ${key}`);
return harden({
has: key => store.has(key),
has: key => {
assertNotBadKey(key);
return store.has(key);
},
init: (key, value) => {
assertNotBadKey(key);
assertKeyDoesNotExist(key);
store.set(key, value);
},
get: key => {
assertNotBadKey(key);
assertKeyExists(key);
return store.get(key);
},
set: (key, value) => {
assertNotBadKey(key);
assertKeyExists(key);
store.set(key, value);
},
delete: key => {
assertNotBadKey(key);
assertKeyExists(key);
store.delete(key);
},
Expand Down
12 changes: 11 additions & 1 deletion packages/store/src/weak-store.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// @ts-check

import { assert, details as X, q } from '@agoric/assert';
import { isEmptyNonRemotableObject } from './helper';
import './types';

/**
Expand All @@ -17,21 +18,30 @@ export function makeWeakStore(keyName = 'key') {
assert(!wm.has(key), X`${q(keyName)} already registered: ${key}`);
const assertKeyExists = key =>
assert(wm.has(key), X`${q(keyName)} not found: ${key}`);
const assertNotBadKey = key =>
assert(!isEmptyNonRemotableObject(key), X`${q(keyName)} bad key: ${key}`);
return harden({
has: key => wm.has(key),
has: key => {
assertNotBadKey(key);
return wm.has(key);
},
init: (key, value) => {
assertNotBadKey(key);
assertKeyDoesNotExist(key);
wm.set(key, value);
},
get: key => {
assertNotBadKey(key);
assertKeyExists(key);
return wm.get(key);
},
set: (key, value) => {
assertNotBadKey(key);
assertKeyExists(key);
wm.set(key, value);
},
delete: key => {
assertNotBadKey(key);
assertKeyExists(key);
wm.delete(key);
},
Expand Down
38 changes: 36 additions & 2 deletions packages/store/test/test-store.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,17 @@
/* eslint-disable no-use-before-define */
import '@agoric/install-ses';
import test from 'ava';
import { Far } from '@agoric/marshal';
import { makeStore, makeWeakStore } from '../src/index';
import { isEmptyNonRemotableObject } from '../src/helper';
import '../src/types';

test('empty object check', t => {
const f = isEmptyNonRemotableObject;
t.truthy(f(harden({})));
t.falsy(f(Far()));
});

function check(t, mode, objMaker) {
// Check the full API, and make sure object identity isn't a problem by
// creating two potentially-similar things for use as keys.
Expand Down Expand Up @@ -78,8 +86,34 @@ test('store', t => {
// makeStore
check(t, 'strong', count => count); // simple numeric keys
check(t, 'strong', count => `${count}`); // simple strings
check(t, 'strong', () => harden({}));
check(t, 'strong', () => Far('handle', {}));

// makeWeakStore
check(t, 'weak', () => harden({}));
check(t, 'weak', () => Far('handle', {}));
});

test('reject unmarked empty objects', t => {
// Older client code used harden({}) to create a "handle" that served as an
// otherwise-empty key for a store/weakstore, but ticket #2018 changes
// marshal to treat unmarked empty objects as pass-by-copy, so they won't
// retain identity across messages, breaking old-style handles in
// surprising ways (key collisions). New client code should use Far()
// instead, which arrives here as an object with a non-empty
// getInterfaceOf(). To catch older clients that need to be updated, we
// reject the use of plain empty objects as keys.

const k = harden({});
const s = makeStore('store1');
t.throws(() => s.init(k, 1), { message: /"store1" bad key:/ });
t.throws(() => s.has(k), { message: /"store1" bad key:/ });
t.throws(() => s.get(k), { message: /"store1" bad key:/ });
t.throws(() => s.set(k, 1), { message: /"store1" bad key:/ });
t.throws(() => s.delete(k), { message: /"store1" bad key:/ });

const w = makeWeakStore('store1');
t.throws(() => w.init(k, 1), { message: /"store1" bad key:/ });
t.throws(() => w.has(k), { message: /"store1" bad key:/ });
t.throws(() => w.get(k), { message: /"store1" bad key:/ });
t.throws(() => w.set(k, 1), { message: /"store1" bad key:/ });
t.throws(() => w.delete(k), { message: /"store1" bad key:/ });
});

0 comments on commit 21ab960

Please sign in to comment.