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

Mismatch in argument count when calling function doResolve #3172

Open
andrey-kuprianov opened this issue May 25, 2021 · 2 comments
Open

Mismatch in argument count when calling function doResolve #3172

andrey-kuprianov opened this issue May 25, 2021 · 2 comments
Assignees
Labels
audit-informal From @informalsystems audit bug Something isn't working SwingSet package: SwingSet

Comments

@andrey-kuprianov
Copy link

andrey-kuprianov commented May 25, 2021

Surfaced from @informalsystems Agoric Audit of Agoric/agoric-sdk/SwingSet hash 23ed67c

Uncovered by renaming kernel.js to kernel.ts and running tsc.

src/kernel/kernel.ts:405:7 - error TS2554: Expected 3 arguments, but got 2.

405       resolveToError(msg.result, VAT_TERMINATION_ERROR);
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  src/kernel/kernel.ts:339:44
    339   function resolveToError(kpid, errorData, expectedDecider) {
                                                   ~~~~~~~~~~~~~~~
    An argument for 'expectedDecider' was not provided.

In multiple places in kernel.js function resolveToError is called with 2 arguments instead of 3. At the same time, in 2 places it is actually called with 3 arguments. Here is the function:

  function resolveToError(kpid, errorData, expectedDecider) {
    doResolve(expectedDecider, [[kpid, true, errorData]]);
  }

It calls doResolve:

function doResolve(vatID, resolutions) {
    if (vatID) {
      insistVatID(vatID);
    }
    for (const resolution of resolutions) {
      const [kpid, rejected, data] = resolution;
      insistKernelType('promise', kpid);
      insistCapData(data);
      const p = kernelKeeper.getResolveablePromise(kpid, vatID);
      const { subscribers, queue } = p;
      let idx = 0;
      for (const dataSlot of data.slots) {
        kernelKeeper.incrementRefCount(dataSlot, `resolve|s${idx}`);
        idx += 1;
      }
      kernelKeeper.resolveKernelPromise(kpid, rejected, data);
      notifySubscribersAndQueue(kpid, vatID, subscribers, queue);
      const tag = rejected ? 'rejected' : 'fulfilled';
      if (p.policy === 'logAlways' || (rejected && p.policy === 'logFailure')) {
        console.log(
          `${kpid}.policy ${p.policy}: ${tag} ${JSON.stringify(data)}`,
        );
      } else if (rejected && p.policy === 'panic') {
        panic(`${kpid}.policy panic: ${tag} ${JSON.stringify(data)}`);
      }
    }
  }

This function in turn passes the (possibly undefined) vatID to getResolveablePromise and notifySubscribersAndQueue.

  function getResolveablePromise(kpid, expectedDecider) {
    insistKernelType('promise', kpid);
    if (expectedDecider) {
      insistVatID(expectedDecider);
    }
    const p = getKernelPromise(kpid);
    assert(p.state === 'unresolved', X`${kpid} was already resolved`);
    if (expectedDecider) {
      assert(
        p.decider === expectedDecider,
        X`${kpid} is decided by ${p.decider}, not ${expectedDecider}`,
      );
    } else {
      assert(!p.decider, X`${kpid} is decided by ${p.decider}, not the kernel`);
    }
    return p;
  }

This function seems to expect that expectedDecider may be undefined. On the other hand,

  function notifySubscribersAndQueue(kpid, resolvingVatID, subscribers, queue) {
    insistKernelType('promise', kpid);
    for (const vatID of subscribers) {
      if (vatID !== resolvingVatID) {
        notify(vatID, kpid);
      }
    }
    // omitted for brevity
  }

doesn't seem to be caring about resolvingVatID being possibly undefined.

Recommendation: While the current code may work fine with the mismatching argument lists, this may lead to errors in the future, as it introduces implicit assumptions down the whole call chain. We recommend to refactor all relevant functions into two variants: one with the VatID argument present, and another, without it.

@andrey-kuprianov andrey-kuprianov added the bug Something isn't working label May 25, 2021
@erights erights added the audit-informal From @informalsystems audit label May 26, 2021
@erights
Copy link
Member

erights commented May 26, 2021

Recommendation: While the current code may work fine with the mismatching argument lists, this may lead to errors in the future, as it introduces implicit assumptions down the whole call chain. We recommend to refactor all relevant functions into two variants: one with the VatID argument present, and another, without it.

@andrey-kuprianov ,
Is there a reason to do that rather than explicitly declare expectedDecider to be an optional argument?

I certainly agree we should do one or the other. In general our style has been to make generous use of optional arguments but only stingy use of duplicated code logic. The later is a refactoring hazard.

@andrey-kuprianov
Copy link
Author

I agree, it is completely fine to declare expectedDecider an optional argument. The only point I wanted to make is that for each function it should be completely transparent about its calling conventions. Implicit assumptions become especially dangerous with time, when new developers come: they don't share those assumptions, and can thus violate them.

I would like to draw a parallel with a similar issue we've encountered during the IBC audit, where implicit assumptions have also almost lead to a bug: cosmos/ibc-go#68.

@warner warner self-assigned this Aug 3, 2021
@dckc dckc added the SwingSet package: SwingSet label Nov 9, 2021
@Tartuffo Tartuffo added MN-1 and removed MN-1 labels Feb 2, 2022
@Tartuffo Tartuffo added this to the Mainnet 1 milestone Mar 23, 2022
@Tartuffo Tartuffo modified the milestones: Mainnet 1, RUN Protocol RC0 Apr 5, 2022
@Tartuffo Tartuffo assigned mhofman and unassigned warner Jun 9, 2022
@Tartuffo Tartuffo removed this from the Mainnet 1 milestone Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-informal From @informalsystems audit bug Something isn't working SwingSet package: SwingSet
Projects
None yet
Development

No branches or pull requests

6 participants