Restrict promise redirect

[mhofman]

closes: #5023
refs: #4318

Best reviewed commit-by-commit

Description

This changes the kernel and comms promise handling logic to enforce 2 basically existing invariants:

• A promise cannot be resolved to another promise. That would be a redirect, which isn't currently implemented
• A non-pipelining decider vat should not be able to use an imported promise as result of a send

The goal is to limit the situations where messages sent to a promise need to change queues. The only case should now be when a message is sent to a result promise decided by a pipelining vat. Such a message can change either

• from the promise queue to the vat's queue because the send for which the promise is the result has been queued to the vat, or
• from the vat's queue to either the resolved target's vat, or back to the promise queue (if the send for which the promise is the result was just forwarded back).

Security Considerations

There shouldn't be any. All now explicitly disallowed behaviors were not currently happening in legitimate flows.

Documentation Considerations

TODO

Testing Considerations

Both positive and negative tests added for promise resolve as well as re-using of imported result promises.

[mhofman]

@warner I have a little bit of doc to write, and one test to add that I just thought about, but I would already appreciate a preliminary review.

[mhofman]

This is actually ready for review. I still need to see if any doc needs updating. I also looked into a more thorough pipeline test over the weekend, but realized that because of the current kernel runqueue, it wouldn't test anything useful, but I'll it for the next step.

[warner]

I think this also allows the case where the vat is re-using a vpid that it previously exported, and I'm not sure you want that (but I might be wrong, I need to think about it more carefully). The allocatedByVat flag is not a direct indicator of the history of the promise, although possible cases are probably limited (and limited even further by this new assertion).

We might be able to use allocatedByVat to achieve the goal, but maybe the new option should be more like "this must be a brand new promise"? (Note that I just filed #5189 and in that case we might see the vpid in args before we see it as result, which could cause a false negative for a "must be new" check.. we might salvage that by processing result before we translate args).

[warner]

Yeah, thinking about it more, I think the check should be requireNew.

When one of us fixes #5189, the new tests (where userspace includes the result promise in the args of the same message) will fail this requireNew check, and we'll fix it (both here and in liveslots) by translating the result before translating the arguments.

const args = [];
const p = E(target).foo(args);
args.push(p);

[mhofman]

Happy to revert to the initial "fresh" / requireNew approach. After realizing that pipelining vats needed the flexibility, I no longer saw a reason to prevent regular vats from pre-allocating a promise (it is currently marked as supported in the docs).

The allocatedByVat flag is not a direct indicator of the history of the promise

Just to make sure I understand this flag correctly, is there any way for allocatedByVat to be true if the promise was in fact imported and not exported? That's the looser restriction I had settled on after moving away from "fresh", which seemed to satisfy all documented usages (and tests).

[warner]

You're correct, if kernel is the one introducing the promise into the vat, then allocatedByVat will always be false. It's just that I avoid using "import" and "export" for Promises, since that's not nearly as meaningful as which side is the decider. For objects, import-vs-export is a big deal, but not so much for Promises.

[warner]

Yeah, thinking about it more, I think the check should be requireNew.

When one of us fixes #5189, the new tests (where userspace includes the result promise in the args of the same message) will fail this requireNew check, and we'll fix it (both here and in liveslots) by translating the result before translating the arguments.

const args = [];
const p = E(target).foo(args);
args.push(p);

[mhofman]

@warner PTAL

I changed requireVatAllocated into requireNew and updated tests accordingly, as well as added a test which represents what #5189 would end up as in syscalls: 50ec896

I will extract 5c98f5b and 4bd30d9, as it's the trigger for the GC weirdness I experience described in #5202, into a separate PR

[warner]

One comment nit, rest looks good, land after fixing.

[mhofman]

Will squash now, then rebase once #5204 has landed.

Edit: eeb79ec..e5f505b