Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
326 stars 206 forks source link

vatPowers.exitWithFailure() should override later vatPowers.exit() #5731

Open warner opened 2 years ago

warner commented 2 years ago

What is the Problem Being Solved?

As part of #5730 , I inadvertently regressed some behavior. If vat userspace does:

vatPowers.exitWithFailure(data1);
vatPowers.exit(data2);

then the vat will be terminated without unwinding the state. This might be surprising if two different parts of the vat code have access to vatPowers, one of them is trying to abandon the delivery (Death Before Confusion!), and the other attempts a preemptory non-WithFailure exit to preserve state changes or messages that it sent.

Those vatPowers calls turn into:

syscall.exit(false, data1);
syscall.exit(true, data2);

which result in two successive calls to:

  let vatRequestedTermination;

  function requestTermination(vatID, reject, info) {
    insistCapData(info);
    vatRequestedTermination = { reject, info };
  }

The second call overwrites the data from the first call, which can lose the reject: true flag, which is what later code uses to decide whether the delivery's changes should be unwound or not.

The E(adminFacet).done() promise will be fulfilled with data2, because it was the last data submitted to requestTermination().

We should also make a conscious choice about what we should do in the face of other calling patterns:

vatPowers.exit(data3);
vatPowers.exit(data4); // which data gets to done() ?
vatPowers.exit(data5);
vatPowers.exitWithFailure(data6); // does done() reject or fulfill?

My instinct is to say that exitWithFailure overrides any (earlier or subsequent) exit. I'm also inclined to say that, apart from this rule, the first info submitted is sticky (rather than the last). But I'd like to hear other opinions.

Description of the Design

With that sort of policy, requestTermination() would look something like:

function requestTermination(vatID, reject, info) {
  insistCapInfo(info);
  if (!vatRequestedTermination ||
      (reject && !vatRequestedTermination.reject)) {
    vatRequestedTermination = { reject, info };
  }
}

However I'd also like to handle GC references on info.slots correctly. That might not require doing increfs/decrefs within requestTermination: it might be sufficient to wait for terminateVat() or notifyTermination() to incref the slots of the last-assigned value of vatRequestedTermination.info. (It might do this already, I'll have to check).

Security Considerations

Allowing two competing entities to share control over vatPowers is not a good idea, but I'd like the outcome to be predictable and safe anyways.

Test Plan

unit tests

warner commented 2 years ago

Incidentally, the previous version shared a single flag between vatFatalSyscall and vatRequestedTermination, so it was important that the flag-setting function explicitly prevent vatPowers.exit() (without rewind) from overriding a fatal syscall (which demands rewind). This new version keeps those in separate flags, so there's no concern about collision between the two, but that's what caused the regression between multiple calls to vatPowers.exit().