[Codegen] Recursive fallback function call runs out of gas via legacy but does not via IR

[bshastry]

contract C0 {
  fallback() external virtual
  {
    assembly
    {
      calldatacopy(0, not(0), 96)
    }
    (bool l0, bytes memory l1) = address(this).call(bytes("ffffffffffffffffffffffffffffffffffffffffffffffffffff000000000000000000000000000000000000000000000000000000000000"));
  }
}
// ====
// compileViaYul: also
// ----
// () -> FAILURE
// gas irOptimized: 211360

Legacy: FAILURE
IR: Pass

[ekpyron]

I'm labeling this codegen error, to heighten the priority of properly investigating this, even though I still hope, it's mostly a testing issue.

[ekpyron]

I think this may be spurious after all - and just a crazy thing happening here :-).
I've not confirmed entirely yet, but I think
calldatacopy(0, not(0), 96) can be replaced with anything that ends up having the same gas cost.

What I guess will happen is that the last recursive call to this may just report failure, but the contract may still have enough gas to finish returning, depending on legacy vs via-IR. But we have to confirm that everything is sane there wrt. gas forwarding, etc.
It's surprising in any case :-).

[ekpyron]

Or to be even clearer: the maximum forwarded gas is 63/64 of the available gas.
So if the recursive call runs out of gas, we still have 1/64 of the total gas left. This may just be enough for finishing the transaction in via-IR codegen, while it may not be with legacy codegen.

[ekpyron]

Several of the recursive calls may manage to return with the available gas - when I added some event to the code example (which of course skews gas costs, so it won't be exactly the same), I saw somewhere between 100 and 200 calls succeeding.

[bshastry]

@ekpyron Isn't calldatacopy(0, not(0), 96) in this issue's test case also overwriting the free memory pointer. If so, could this also be undefined behavior between legacy and IR?

[ekpyron]

That's true... although my guess would be that one could construct similar cases without that... on the other hand, trying that, it's not that easy :-). Overwriting the free memory pointer there, may mainly result in weird data to be passed during the recursive call, though.

[ekpyron]

Hm... or maybe I was wrong before and there's something else happening here... maybe worth another look in any case :-).

[bshastry]

Okay, in the meantime I will work on avoiding writes to free memory ptr while generating test cases. For the time being, I could template all memory writes to start at 0x60 i.e., mstore(add(loc, 0x60), value), calldatacopy(add(loc, 0x60), from, size) and so on; where loc is a randomly generated value.

[ekpyron]

Make it 0x80 - offset 0x60 is the "zero-memory offset", which is used to initialize zero-length dynamic arrays and always has to remain zero - i.e. writing anything non-zero there will also be undefined :-).

What's strictly speaking always well-defined is would be

mstore(add(mload(0x40), loc), ...)

(given that loc is bounded and the addition does not overflow - or more conservatively add(mload(0x40), and(loc, 0xffffffff)))

And

mstore(and(loc, 0x3f), ...)

But we also need to make sure not to restrict this too much in order not to miss anything...

[bshastry]

mstore(add(mload(0x40), loc), ...)

This would be load from free memory pointer and add an arbitrary bounded value to it, correct? Assuming loc is bounded this should be fine.

mstore(and(loc, 0x3f), ...)

What does this mean? 0x3f would make only 6 addressable bits, right? That would be a lot more conservative than first proposal?

[ekpyron]

I meant choosing between either of them. And actually and(loc, 0x3f) was wrong... 0x1f would work, though...
You're allowed to use any memory past the free memory pointer and any memory between offsets 0 and 0x3f.
mstore(add(mload(0x40), loc), ...) is the former, mstore(and(loc, 0x1f), ...) at least roughly the latter.

But in any case, I don't think we should fully restrict to those cases, i.e. your suggestion of using a fixed offset may be better.

We want the fuzzer to occasionally dirty previously allocated memory after all.
So maybe alternating between mstore(add(0x80, loc), ...) (with bounded loc) and mstore(loc, ...) (with loc being restricted to at most 0x20) is best.

[NunoFilipeSantos]

@bshastry what is the impact of this bug (High, Medium, Low)?

[bshastry]

I would say low if at all. There is the possibility that this is not a bug because overwriting the free memory pointer is essentially undefined behavior.