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

Document code section type malleability #176

Closed
chfast opened this issue Jan 27, 2025 · 14 comments
Closed

Document code section type malleability #176

chfast opened this issue Jan 27, 2025 · 14 comments
Assignees

Comments

@chfast
Copy link
Member

chfast commented Jan 27, 2025

Having a returning code section, e.g. RETF, its natural type is inputs=0, output=0, max_stack_height=0. However, you can artificially increase these numbers, e.g. inputs=127, output=127, max_stack_height=127, and still have a valid type (with some additional runtime limitation for the caller).

This this in the spec by requiring the values to be minimal.

@pdobacz
Copy link
Member

pdobacz commented Jan 27, 2025

The malleability only goes as far as the particular RETF section in question, when seen in isolation. In order for the container to validate the callers of such section need to be different (satisfy the 0-127 stack items when calling). Does this observation make it less of an issue?

@gumb0
Copy link
Contributor

gumb0 commented Jan 28, 2025

Yes, these are clearly different functions from the point of view of stack validation. Even for RETF in isolation, one case requires 0 values on stack, and another one 127 values.

This this in the spec by requiring the values to be minimal.

How would validation figure out that this is "artificially increased" numbers and not function legitimately taking N inputs, doing some transformations on them and returning N outputs?

@chfast
Copy link
Member Author

chfast commented Feb 4, 2025

The main argument is still that no sane compiler/user would diverge from using the "minimal" type. And it goes along other similar restrictions: no unused code sections, no unreferenced subcontainers, etc...

The validation rule seems easy to implement: you have to track if any of the instructions reached the stack height of 0.

The prototype implementation with some EOF validation tests corrected: ethereum/evmone#1122.

The EEST failures caused by the change:

FAILED tests/osaka/eip7692_eof_v1/converted/efStack_jumpf_to_nonreturning_.py::test_eof_validation[fork_Osaka-eof_test-jumpf_to_nonreturning_2] - ethereum_test_specs.eof.UnexpectedEOFExceptionError: Expected EOF code to be valid, but an exception occurred:
FAILED tests/osaka/eip7692_eof_v1/converted/efStack_jumpf_to_nonreturning_.py::test_eof_validation[fork_Osaka-eof_test-jumpf_to_nonreturning_3] - ethereum_test_specs.eof.UnexpectedEOFExceptionError: Expected EOF code to be valid, but an exception occurred:
FAILED tests/osaka/eip7692_eof_v1/converted/efStack_jumpf_to_nonreturning_variable_stack_.py::test_eof_validation[fork_Osaka-eof_test-jumpf_to_nonreturning_variable_stack_2] - ethereum_test_specs.eof.UnexpectedEOFExceptionError: Expected EOF code to be valid, but an exception occurred:
FAILED tests/osaka/eip7692_eof_v1/converted/efStack_jumpf_with_inputs_stack_overflow_.py::test_eof_validation[fork_Osaka-eof_test-jumpf_with_inputs_stack_overflow_0] - ethereum_test_specs.eof.UnexpectedEOFExceptionError: Expected EOF code to be valid, but an exception occurred:
FAILED tests/osaka/eip7692_eof_v1/converted/efStack_jumpf_with_inputs_stack_overflow_variable_stack_.py::test_eof_validation[fork_Osaka-eof_test-jumpf_with_inputs_stack_overflow_variable_stack_0] - ethereum_test_specs.eof.UnexpectedEOFExceptionError: Expected EOF code to be valid, but an exception occurred:
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_execution.py::test_jumpf_with_inputs_stack_size_1024[fork_Osaka-eof_test] - ethereum_test_specs.eof.UnexpectedEOFExceptionError: Expected EOF code to be valid, but an exception occurred:
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_execution.py::test_jumpf_with_inputs_stack_size_1024[fork_Osaka-state_test] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_execution.py::test_jumpf_with_inputs_stack_size_1024[fork_Osaka-blockchain_test] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_execution.py::test_jumpf_with_inputs_stack_size_1024[fork_Osaka-blockchain_test_engine] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-eof_test-so-2-to-N] - ethereum_test_specs.eof.UnexpectedEOFExceptionError: Expected EOF code to be valid, but an exception occurred:
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-eof_test-so-4-to-N] - ethereum_test_specs.eof.UnexpectedEOFExceptionError: Expected EOF code to be valid, but an exception occurred:
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-eof_test-so-127-to-N] - ethereum_test_specs.eof.UnexpectedEOFExceptionError: Expected EOF code to be valid, but an exception occurred:
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-state_test-so-2-to-N] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-state_test-so-4-to-N] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-state_test-so-127-to-N] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-blockchain_test-so-2-to-N] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-blockchain_test-so-4-to-N] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-blockchain_test-so-127-to-N] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-blockchain_test_engine-so-2-to-N] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-blockchain_test_engine-so-4-to-N] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input
FAILED tests/osaka/eip7692_eof_v1/eip6206_jumpf/test_jumpf_target.py::test_jumpf_target_rules[fork_Osaka-blockchain_test_engine-so-127-to-N] - Exception: failed to evaluate: EOF container at 0x0000000000000000000000000000000000001000 is invalid: unused_input

You can note that all the failing/updated tests have JUMPF. This is because there is a pattern used in these tests to create non-returning code section with non-zero inputs, e.g. (Op.STOP, code_inputs=3, code_outputs=NON_RETURNING). Such sections are not valid according to the new rule.

@gumb0
Copy link
Contributor

gumb0 commented Feb 5, 2025

The validation rule seems easy to implement: you have to track if any of the instructions reached the stack height of 0.

Some observations: It means for legitimately unused arguments (e.g. callback implementation not caring about some of arguments) compilers will have to insert additional POP instructions. This goes a bit against the relaxed handling of terminating instructions, where no extra POPs are needed.

Actually one SWAPN/DUPN/EXCHANGE would be sufficient instead of series of POPs, that's what I would probably do in some of those faling tests.

@chfast
Copy link
Member Author

chfast commented Feb 5, 2025

It means for legitimately unused arguments (e.g. callback implementation not caring about some of arguments) compilers will have to insert additional POP instructions. This goes a bit against the relaxed handling of terminating instructions, where no extra POPs are needed.

If there are legitimate use cases for it, I'm happy to take it back. I guess you indicated it already: function pointer, virtual method? However, they are not directly implementable in EOFv1.

@gumb0
Copy link
Contributor

gumb0 commented Feb 5, 2025

It means for legitimately unused arguments (e.g. callback implementation not caring about some of arguments) compilers will have to insert additional POP instructions. This goes a bit against the relaxed handling of terminating instructions, where no extra POPs are needed.

If there are legitimate use cases for it, I'm happy to take it back. I guess you indicated it already: function pointer, virtual method? However, they are not directly implementable in EOFv1.

I think it can be like compile-time polymorphism: you link with some library code, that requires callback with certain name and signature to be implemented?

@frangio
Copy link

frangio commented Feb 5, 2025

callback implementation not caring about some of arguments

The callback would still be expected to clear the stack of those arguments so it would naturally already have to POP them.

Hm, I guess that's not true if it's a non-returning callback...

What is the concrete issue caused by malleability?

@chfast
Copy link
Member Author

chfast commented Feb 18, 2025

I've tried to find a good use case justifying not adding this validation rule. I explored the function pointers in solidity because these feature enforces a specific function signature on a group of functions.

contract Storage {

    function fa(uint256 arg) private pure returns(uint256) {
        return arg;
    }

    function fb(uint256) private pure returns(uint256) {
        return 0;
    }

    function test(bool cond, uint256 x) external pure returns(uint256) {
        function(uint256) pure returns(uint256) ptr = fa;
        if (cond)
            ptr = fb;
        
        return ptr(x);
    }
}

In the example we have two functions fa and fb with the same signature required by the function pointer. The fa is some kind of "forward" function where it forwards the input to the output. I believe this is a valid use case.

At first, the best implementation of such function seems to be a code section of type (1,1,1) with just RETF. However, such implementation would break the validation rule proposed here.

However, you can notice that in EOF there is no "indirect call" instruction (an instruction calling a function by taking its index from the stack). So the implementation of the function pointer invocation must be implemented by RJUMPV + CALLFs where CALLFs are bound to the target functions statically. In the end the compiler can still assign type (0, 0, 0) to the fa or even eliminate a CALLF to fa entirely knowing that the function doesn't do anything.

In other words, solidity function type don't have to match the EOF function type.

@chfast chfast changed the title Fix code section type malleability Document code section type malleability Feb 18, 2025
@frangio
Copy link

frangio commented Feb 18, 2025

Great example. The argument changes a little with #134 though, right? The code section type would be (1, 1, 0) which is minimal.

@pdobacz
Copy link
Member

pdobacz commented Feb 19, 2025

Great example. The argument changes a little with #134 though, right? The code section type would be (1, 1, 0) which is minimal.

I might be wrong, but I think the argument is the same. The "best implementation" would be (1,1,0), and fa can be minimized to (0,0,0) and do nothing. The only change is notation from max_stack_height=1 to max_stack_increase=0.

@chfast
Copy link
Member Author

chfast commented Feb 19, 2025

I talked with Solidity and they convinced me not to do this. Although this is technically doable in the compiler it has some level of annoyance.

@cameel:

Yeah, seems doable just the question is if it's worth it. It does not make anything impossible, but it does add some more hoops to jump through.
I'd expect it to potentially by an annoyance for very simple compilers. Or for people writing EVM assembly by hand.

@ekpyron:

I'd definitely say that a function only containing RETF should pass validation whenever number of inputs match number of outputs. I would argue that RETF should be taken into account as consuming output-many arguments when checking whether stack height zero is reached.

This is very nice observation about RETF. The EIP-4750 says that RETF "Pops nothing and pushes nothing to operand stack.". However, evmone's implementation sets the required stack height to the number of function outputs (https://github.com/ethereum/evmone/blob/45cbad601786ac5bddcfea98f82e837d01247e08/lib/evmone/eof.cpp#L483). So in some other words the RETF already consumes all remaining stack operands.

@chfast
Copy link
Member Author

chfast commented Feb 19, 2025

@cameel
Copy link

cameel commented Feb 19, 2025

There was some more discussion after the comments quoted above so I'll clarify that our stance on this is actually ambivalent.

We later realized that this would be much simpler to satisfy than we assumed at first. I.e. it's just a matter of setting the right input/output number in the header and does not require altering the code at all (neither the code section nor the call site; no POPs or any other changes required). If this is the case then this validation seems in line with things like disallowing unused containers or functions and would indeed make some sense. It would be a bit of an annoyance, but you could say the same thing about those other validation already.

On the other hand, the points quoted by @chfast still stand, so it does have some minor downsides too. We don't really have a strong opinion one way or the other.

BTW, all of this assumes that we're talking only about the case of returning functions. For non-returning ones it does not seem like a good idea, but @chfast said that that idea was dropped.

@pdobacz
Copy link
Member

pdobacz commented Mar 10, 2025

will be resolved in ethereum/EIPs#9382

@pdobacz pdobacz closed this as completed Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants