reject imported memories early #8146
Conversation
Ekleog-NEAR
force-pushed
the
reject-imported-mem-early
branch
from
f3b89fd to
fadef43
Compare
| _ => continue, | ||
| // TODO: it’s a protocol change because the gas amount burned would change for some tests, but here we should | ||
| // _ => return Err(PrepareError::Instantiate), |
There was a problem hiding this comment.
Allowing imports of globals etc. seems fine to me, even if we don’t have any currently. As long as we confirm that they must come from env which we do, it is fine.
There was a problem hiding this comment.
Hmm so I was coming from the idea that right now we validate func types again (though I’m not really sure why), but we just ignore globals etc. Anyway it doesn’t change anything, so I’m removing this comment :)
| // This test assumes that maximum page number is configured to a certain number. | ||
| assert_eq!(VMConfig::test().limit_config.max_memory_pages, 2048); | ||
|
|
||
| let r = parse_and_prepare_wat(r#"(module (import "env" "memory" (memory 1 1)))"#); | ||
| assert_matches!(r, Ok(_)); | ||
| assert_matches!(r, Err(PrepareError::Memory)); |
There was a problem hiding this comment.
So in the past the preparation would pass, and then the code would get passed into Wasmer2VM::compile_uncached() which then returned WasmerCompileError { ... } rather than a PrepareError::Memory.
This then moves on to be encoded into, I believe, the chain as the error message. At least that’s what I think matklad has explained to me in the past. So in the end this is still a protocol change, isn’t it?
There was a problem hiding this comment.
Based on the chat here, I think it isn’t :)
Ekleog-NEAR
force-pushed
the
reject-imported-mem-early
branch
2 times, most recently
from
58882e5 to
894f16d
Compare
They were already rejected by wasmer2 before this change, just after preparation. So this should not be a protocol change.
Ekleog-NEAR
force-pushed
the
reject-imported-mem-early
branch
from
894f16d to
c49257c
Compare
They were already rejected by wasmer2 before this change, just after preparation. So this should not be a protocol change.
Second attempt at #8029, without a protocol change this time