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

Extraneous imported promises cause memory growth #6074

Open
mhofman opened this issue Aug 26, 2022 · 3 comments
Open

Extraneous imported promises cause memory growth #6074

mhofman opened this issue Aug 26, 2022 · 3 comments
Labels
bug Something isn't working liveslots requires vat-upgrade to deploy changes resource-exhaustion Threats to availability from resource exhaustion attacks SwingSet package: SwingSet vaults_triage DO NOT USE

Comments

@mhofman
Copy link
Member

mhofman commented Aug 26, 2022

Description of the problem

One kind of memory consumption that can be inflicted upon an innocent vat is when the inbound dispatch.deliver or dispatch.notify contains promise vrefs (vpids) in the arguments or resolution data. Liveslots creates actual Promise objects for each of these, passes them to userspace, and remembers their resolve/reject functions in the importedVPIDs table.

Each promise incurs memory consumption within the vat worker, which cannot be shed until it receives a dispatch.notify and the Promise can be settled. At that point, the resolved promise loses its identity, and liveslots is allowed to stop tracking it. If userspace does not retain a reference, the Promise can then be garbage-collected by the engine.

The problem is that an attacker might send messages with spurious extra arguments, which contain vpids, to provoke this memory consumption. The victim vat is not looking for extra arguments (or extra properties on normal arguments): it could, but it would be pretty awkward (walking the whole argument object graph). And even if it did that, the memory consumption is triggered when liveslots deserializes the data, so by the time userspace gets them, it is too late.

The same is true for promises that appear in resolution data (when syscall.notify includes capdata with new vpids in slots), but it's even more awkward for userspace to be on the lookout for these. It would have to subscribe to every promise it ever sees, just to learn about even more promises in the resolution.

Root cause and solutions

This kind of leak is caused by a chain of limitations in liveslots of JS engines:

  • Liveslots/eventual-send creates promises which have as prototype the regular Promise.prototype making it impossible to detect when these imported promise are awaited or thened. If using a normal subclass of Promise, it would be possible to detect if/when these promises are awaited, and delay subscription until then. Issue Use thenables for promises as delivery arguments #8469.
  • In the absence of detecting interest from the consumer, liveslots assumes it, and proactively subscribes to all imported promises. In order to react to a later notification of settlement, liveslots holds onto the promise resolvers. Currently all JS engines keep a strong reference from the resolvers to the promise instance, making it impossible to detect that the userland may have dropped the promise. If engines collected these promises referenced only internally through their resolvers, it'd be possible for liveslots to register them in a FinalizationRegistry and unsubscribe when dropped. See Unsubscribe from imported promises dropped by userland #6075.
  • An alternative to dropping these promise would be to explicitly discard them. Currently the userland cannot express where it expects promises on inbound messages. With such a mechanism to inform liveslots, messages with extra promises could be either rejected or the extra promises unsubscribed from. See use method schemas to limit inbound argument shapes #5955 for a description of this approach.
@mhofman mhofman added bug Something isn't working SwingSet package: SwingSet labels Aug 26, 2022
@dckc dckc added the resource-exhaustion Threats to availability from resource exhaustion attacks label Aug 29, 2022
@Tartuffo Tartuffo added this to the Mainnet 1 RC0 milestone Sep 1, 2022
@Tartuffo Tartuffo removed this from the Mainnet 1 RC0 milestone Sep 21, 2022
@dckc
Copy link
Member

dckc commented Nov 1, 2022

@mhofman
Copy link
Member Author

mhofman commented Nov 1, 2022

We are possibly considering the introduction of pseudo-promises for virtual promises, which wouldn't be platform promises, nor thenable. In that case, imported promises would also become pseudo-promises, and we could detect the lifetime of these representatives the same way as remotes.

This is however a breaking API change through the whole stack, from endo, to liveslots and contracts above.

@ivanlei ivanlei added the vaults_triage DO NOT USE label Jan 17, 2023
@warner warner added the liveslots requires vat-upgrade to deploy changes label Jan 24, 2023
@mhofman
Copy link
Member Author

mhofman commented Oct 23, 2023

  • Liveslots/eventual-send creates promises which have as prototype the regular Promise.prototype making it impossible to detect when these imported promise are awaited or thened.

Filed an issue to track this approach: #8469

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working liveslots requires vat-upgrade to deploy changes resource-exhaustion Threats to availability from resource exhaustion attacks SwingSet package: SwingSet vaults_triage DO NOT USE
Projects
None yet
Development

No branches or pull requests

5 participants