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

promise.domain interferes with harden() in non-SES code and node REPL #22

Closed
warner opened this issue Mar 23, 2019 · 3 comments
Closed
Assignees
Labels
security SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Mar 23, 2019

We learned that Promise instances acquire an extra property in code executed from the Node REPL:

$ node
> p = new Promise(r => 1)
> Object.getOwnPropertyNames(p)
[ 'domain' ]
> Object.getPrototypeOf(p.domain)
Domain {
  members: undefined,
  _errorHandler: [Function],
  enter: [Function],
  exit: [Function],
  add: [Function],
  remove: [Function],
  run: [Function],
  intercept: [Function],
  bind: [Function] }
> 

This seems to exist to support Node's deprecated "Domain" concept (https://nodejs.org/api/domain.html#domain_domains_and_promises), which provides a sort of on-exit notification when execution has left a particular region of the code (think server event handlers, and cleaning up resources allocated for a single event).

The .domain property is an object whose prototype is not a normal JS primordial.

When we run bin/vat shell --no-ses, any code which tries to harden() a Promise will fail, because it tries to harden p.domain, and then it sees that the prototype is not in the "fringe" (the set of all the primordials that SES would have frozen). In this way, our non-SES harden() behavior is successfully detecting something strange: a form of mutability that would have violated the goals of hardening.

But in practice, it's kind of a drag. Running the vat with --no-ses is awfully handy for tracking down the line numbers of crashes, so it's annoying that the code can't work without SES.

One fix might be to find this domain prototype and add it to the @agoric/harden fringe. Another might be for the vat to monkeypatch Promise (somehow?) to stop adding these domain properties. Since this is specific to the Node REPL, there may be some hook we can use to control the behavior without needing to muck with Promise too much.

@erights
Copy link
Member

erights commented Mar 23, 2019

We should also file this as an issue against Node at https://github.com/nodejs/node/issues/new?template=1-bug-report.md

I suggest that we remove the extra property until we find that we broke something that we care about.

@warner warner transferred this issue from Agoric/SwingSet Dec 1, 2019
@warner warner added the SwingSet package: SwingSet label Dec 1, 2019
dckc pushed a commit to dckc/agoric-sdk that referenced this issue Dec 5, 2019
dckc pushed a commit to dckc/agoric-sdk that referenced this issue Dec 5, 2019
Fix Agoric#22: Remove domain property from promise
dckc pushed a commit to dckc/agoric-sdk that referenced this issue Dec 5, 2019
dckc pushed a commit to dckc/agoric-sdk that referenced this issue Dec 5, 2019
Remove corkboardAssay concept + trivial name cleanups
dckc pushed a commit to dckc/agoric-sdk that referenced this issue Dec 5, 2019
sort out unfulfilled handlers and forwarding
@erights erights self-assigned this Feb 26, 2021
@erights
Copy link
Member

erights commented Feb 26, 2021

Despite the title of this issue, the domain property that Node adds it might add to anything, not just promises.

We've discussed this with @bmeck and at the SES meetings. An interesting development is that the domain property or the code using it seems to be broken in Node, so perhaps there's hope of getting them to remove it. In any case, we cannot use Node securely until we're confident that it is not adding this as a property.

Assigning to myself to track this. But there's not much more we can do on our side other than remember that we cannot use node securely.

@erights
Copy link
Member

erights commented Feb 15, 2022

@kriskowal closed this in endo.

@kriskowal , if I'm missing something, please reopen.

@erights erights closed this as completed Feb 15, 2022
anilhelvaci referenced this issue in Jorge-Lopes/agoric-sdk Feb 16, 2024
# This is the 1st commit message:

create dedicated files for test assertions and tools

# This is the commit message #2:

Add new test file for liquidation visibility

# This is the commit message #3:

auctioneer snapshot

# This is the commit message #4:

Update visibility tests and helper functions

# This is the commit message #5:

add tools and assertions to match subscriber with vstorage data

# This is the commit message #6:

update test visibility of vault liquidation

# This is the commit message #7:

fix(liquidationVisibility): fix linting errors

# This is the commit message #8:

fix(liquidationVisibility): type error

# This is the commit message #9:

chore(liquidationVisibility): #4 testing tools setup

# This is the commit message #10:

chore(liquidationVisibility): #4 improve testing tools

# This is the commit message #11:

chore(liquidationVisibility): #4 improve testing tools

# This is the commit message #12:

fix(liquidationVisibility): linting fixes

# This is the commit message #13:

chore(liquidationVisibility): fix test names

# This is the commit message #14:

fix(liquidationVisibility): lint fix

# This is the commit message #15:

chore(liquidationVisibility): #4 implement `assertNodeInStorage`

# This is the commit message #16:

fix(liquidationVisibility): #4 lint fix

# This is the commit message #17:

chore(liquidationVisibility): #4 test skeleton is ready

# This is the commit message #18:

chore(liquidationVisibility): #4 add marshaller for comparing data from the vstorage

# This is the commit message #19:

chore(liquidationVisibility): sample test for `preAuction` and `postAuction` data fields

# This is the commit message #20:

chore(liquidationVisibility): make sure `assertStorageData` works

# This is the commit message #21:

chore(liquidationVisibility): #4 add test for case 2b, uncomment assertions for running the tests

# This is the commit message #22:

feat(liquidationVisibility): create liquidation storageNodes and recorderKits

BREAKING CHANGE: Introduced the `_timestamp` as an argument for the vaultManager liquidateVaults method.

feat(liquidationVisibility): write preAuctionState and auctionResultState to Vstorage

BREAKING CHANGE: a getVaultId method was included in the Vault interface, which returns the vault `idInManager`

fix(liquidationVisibility): fix type definitions errors and concurrently await multiple promises

feat(liquidationVisibility): write postAuctionState to Vstorage

BREAKING CHANGE: the getVaultId method of vaults interface was updated to getVaultState, which will return the vault phase as well

chore(liquidationVisibility): update helper methods, type definitions and names

fix(liquidationVisibility): lint fix

fix(liquidationVisibility): update sequence to write auction state to Vstorage and its structure

The helper methods built for this purpose became unnecessary and were removed.

fix(liquidationVisibility): update postAuctionState structure and change to writeFinal recorder method

fix(liquidationVisibility): remove temporary changes made to tests

fix(liquidationVisibility): #4 test for scenario 2b passed, test api updated, #7 is fixed

chore(liquidationVisibility): #4 test for scenario 2a passed

chore(liquidationVisibility): #4 update scenario 1

Squashed commit of the following:

commit 728d695
Author: anilhelvaci <anilhelvaci96@gmail.com>
Date:   Wed Jan 31 14:28:14 2024 +0300

    chore(liquidationVisibility): uncomment post auction assertion in `liq-result-scenario-1`

commit dd3fbdb
Author: anilhelvaci <anilhelvaci96@gmail.com>
Date:   Wed Jan 31 14:25:22 2024 +0300

    fix(liquidationVisibility): lint fix

commit 6920d1a
Author: anilhelvaci <anilhelvaci96@gmail.com>
Date:   Wed Jan 31 14:22:28 2024 +0300

    fix(liquidationVisibility): explain Promise.allSettled

commit 732e1d7
Author: anilhelvaci <anilhelvaci96@gmail.com>
Date:   Wed Jan 31 11:37:45 2024 +0300

    feat(liquidationVisibility): handle errors that might arise from other vats

commit 683f56d
Author: anilhelvaci <anilhelvaci96@gmail.com>
Date:   Tue Jan 30 14:31:13 2024 +0300

    feat(liquidationVisibility): add LiquidationVisibilityWriters to improve readability, fetch schedule during the auction itself

commit 4c45f2a
Author: anilhelvaci <anilhelvaci96@gmail.com>
Date:   Tue Jan 30 10:30:02 2024 +0300

    fix(liquidationVisibility): add pattern matcher to `getVaultState`

chore(liquidationVisibility): #4 add auctioneer wrapper and update setupBasics

chore(liquidationVisibility): #4 add mock makeChainStorageNode

chore(liquidationVisibility): #4 add tests for no vaults and rejected schedule

fix(liquidationVisibility): #4 add setBlockMakeChildNode method and update file name

fix(liquidationVisibility): #4 update import path

chore(liquidationVisibility): #4 add liq-rejected-timestampStorageNode test

fix(liquidationVisibility): #4 fix bug with at makeChildNode

fix(liquidationVisibility): #4 update test names and comments

fix(liquidationVisibility): #4 lint fix

chore(internal): create key-value pairs for Promise.allSettled values

fix(internal): make allValuesSettled a mapper for resolved promises and silently handles rejected ones

fix(internal): fix doc

fix(liquidationVisibility): make sure promises are assigned in key-value fashion, lint fixes.

chore(liquidationVisibility): extend liq-rejected-timestampStorageNode and clean outdated comments

fix(liquidationVisibility): revert update made to package.json

chore(liquidationVisibility): update snapshot generated by unit tests

fix(liquidationVisibility): lint fix

chore(liquidationVisibility) #4 test multiple vaultManagers

fix(liquidationVisibility): #4 lint fix

fix(liquidationVisibility): add pattern matcher to `getVaultState`

feat(liquidationVisibility): add LiquidationVisibilityWriters to improve readability, fetch schedule during the auction itself

chore(liquidationVisibility): init work for bootstrap tests

chore(liquidationVisibility): created test-liquidation-visibility.ts and started building a test suite

chore(liquidationVisibility): upgrade tests are implemented.

Refs: #15

fix(liquidationVisibility): vaults are now displayed in the correct order at `vaults.preAuction`

Refs: #13

fix(liquidationVisibility): vault phases are now displayed correctly at `vaults.postAuction`

Refs: #14

chore(liquidationVisibility): add storage snapshot

Refs: #15

fix(liquidationVisibility): don't reverse `vaultData` for preAuction storage node

Refs: #13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security SwingSet package: SwingSet
Projects
None yet
Development

No branches or pull requests

2 participants