metering: tolerate bound "getMeter" in metered source code

[warner]

Describe the bug

It looks like the identifier getMeter is entirely taboo in source code which is subject to the metering transform. Any source which happens to use this name (even in the normal "non free variable" case, where it defines a new function or variable with the name and references the one it just defined) will be rejected. This can be a bit surprising.

The specific case where we saw source code being rejected was when the Zoe contract facet, bundled and loaded into a new (dynamic, metered) vat, was itself trying to impose metering on the new contract code (at this point in ZCF development, ZCF was using the same evalContractBundle that non-split Zoe was, and non-split zoe must do within-same-vat metering by creating a getMeter endowment; ZCF was then corrected to not impose metering, because it's already running in a metered dynamic vat, and the immediate rejection problem went away).

We certainly need to prohibit this confined code from successfully accessing the getMeter global lexical, because the one that metering references can be used to both increment and decrement the meter, and the confined code should not be able to refill its own meter. The transformation must not allow a free getMeter reference in the input survive to the output.

I think the current transformation accomplishes this by throwing an exception if the parser ever reaches an identifier spelled getMeter. This effectively rejects any reference to getMeter, whether it's bound or not.

Ideally, the metering transform would only pay attention to unbound/free references to getMeter, so that const getMeter = 1; getMeter+2; would pass. That would reduce much of the surprise.

Another approach would be to rename any instances of getMeter into something else (maybe getMeter_1). This would have to be a "Hilbert Hotel" -style renaming, because there might already be a variable named getMeter_1 (which must be renamed to getMeter_2, etc). This changes the behavior of error messages and debug tools, and could break code which does e.g. let x = { getMeter: 1 }; x['get'+'Meter'], but might be easier than a context/scope-sensitive transform.

To Reproduce

Create a dynamic vat like:

export function buildRootObject() {
  const getMeter = 1;
  return harden({});
}

Bundle that and load it into a dynamic vat.

E(vatAdmin).createVat(bundle);

The createVat(bundle) will fail, with an error somewhat like Identifier getMeter is reserved for metering code.

For a quick way to reproduce this, edit packages/SwingSet/test/metering/metered-dynamic-vat.js to insert a const getMeter = 1; anywhere in the file, then run node -r esm test/metering/test-dynamic-vat.js.

cc @michaelfig because he wrote the transform and is probably the only one who understands Babel enough to pull this off.

We might defer fixing this long enough to switch to XS-based metering (and remove the transform altogether), although there a number of non-XS environments where we might still want transform-based metering, in which case this would be nice to have fixed.

[warner]

Hm, I suppose allowing const getMeter = 1; anywhere, even if we can tell it's not referencing the global lexical getMeter, would still threaten the injected code that's also referencing getMeter. The metered (input) code should not be able to confuse the injected metering code into using the wrong getMeter: that would allow it to select the meter being used, which is just as bad as allowing it to access the real meter.

So renaming identifiers sounds like the less-broken approach.

[michaelfig]

Renaming the evaled code identifiers leads to a world of pain, especially once we support direct eval.

Better is to have the injected getMeter be an identifier like $h\u200D_getMeter which would be very unlikely to cause an accidental conflict. Then, return that identifier from the metering-transform maker function, so that it's not hardcoded in the meter-imposing code.

[warner]

we stopped doing code-transformation -based metering ages ago, this is no longer relevant