runtime: simplify MemoryLike trait #8155
Conversation
Firstly, read_memory_u8 is never used so get rid of it. Secondly, fits_memory is always used in tandem with read_memory or write_memory. Rather than having the check as separate method, include the bounds check in the latter two methods. This changes them from panicking to returning an error when trying to access memory out of bounds. While at it, add tests for WasmerMemory. Esentially copies of tests from Wasmer2Memory.
Co-authored-by: Simonas Kazlauskas <github@kazlauskas.me>
|
Thanks! Let me know if I ought to apply the automerge label for you^^ |
|
Yeah, will need S-automerge here now. |
|
Let’s pause it actually. I’ve realised it introduces DoS. |
How come? My intuition is that this should be a NFC and I’d be super curious to see where it is that I’m wrong. |
|
The change is in memory_get_vec. My change swapped checking memory access and vector allocation: fn memory_get_vec(&mut self, offset: u64, len: u64) -> Result<Vec<u8>> {
self.gas_counter.pay_base(read_memory_base)?;
self.gas_counter.pay_per(read_memory_byte, len)?;
- self.try_fit_mem(offset, len)?;
let mut buf = vec![0; len as usize];
- self.memory.read_memory(offset, &mut buf);
+ self.memory.read_memory(offset, &mut buf).map_err(|_| HostError::MemoryAccessViolation)?;
Ok(buf)
}This means that at least theoretically contract could issue a request to read u64::MAX bytes and VMLogic will try to allocate vector of that size (which would result in a panic). The old code would never get to the vector allocation. There is also gas limit so perhaps this isn’t actually an issue but it’s something that would need to be verified. |
|
Got it. Yeah, this is somewhat nasty. I don’t think the limit is restrictive enough to allocate a |
|
@nagisa, PTAL. |
read_memory and write_memory have to perform the same checks as fits_memory already does. So rather than panicking, change the methods to return an error. This makes the interface panic-free and in some situations allows fits_memory call to be skipped. However, separate fits_memory check may still be necessary so keep the method but document in detail why it’s needed and, to keep interfaces consistent, change it to return a Result. Finally, read_memory_u8 is never used so get rid of it. While at it, add tests for WasmerMemory. Essentially copies of tests from Wasmer2Memory.
read_memory and write_memory have to perform the same checks as
fits_memory already does. So rather than panicking, change the
methods to return an error. This makes the interface panic-free
and in some situations allows fits_memory call to be skipped.
However, separate fits_memory check may still be necessary so
keep the method but document in detail why it’s needed and, to
keep interfaces consistent, change it to return a Result.
Finally, read_memory_u8 is never used so get rid of it.
While at it, add tests for WasmerMemory. Essentially copies
of tests from Wasmer2Memory.