wmemcheck support for additional allocation functions and granular memory #9641
Conversation
github-actions
bot
added
the
wasmtime:api
crates/cranelift/src/func_environ.rs
Outdated
| } else if func_name == Some("free") { | ||
| self.check_free_start(builder); | ||
| match self.current_func_name(builder) { | ||
| Some("__wrap_malloc") | Some("malloc") => |
There was a problem hiding this comment.
Can we add a comment here noting where the __wrap_* variants come from? That's useful both for its own sake (readers' understanding) but also so that if things change in the future with the external thing causing this need, we can re-evaluate.
crates/environ/src/builtin.rs
Outdated
| // Invoked when wasm stack pointer is updated. | ||
| #[cfg(feature = "wmemcheck")] | ||
| update_stack_pointer(vmctx: vmctx, value: i32); | ||
| // Invoked before memory.grow is called. | ||
| #[cfg(feature = "wmemcheck")] | ||
| update_mem_size(vmctx: vmctx, num_bytes: i32); | ||
|
|
||
| // Invoked before stuff is called. |
There was a problem hiding this comment.
can we be more specific than "stuff" here?
| // Hook for validating calloc using wmemcheck_state. | ||
| #[cfg(feature = "wmemcheck")] | ||
| unsafe fn check_calloc(store: &mut dyn VMStore, instance: &mut Instance, addr: u32, count: u32, size: u32) -> Result<u32> { | ||
| check_malloc(store, instance, addr, count * size) |
There was a problem hiding this comment.
Can we add a note here that since we aren't tracking undefined-ness, we don't have to care about the other semantic difference between calloc and malloc, namely that memory is zeroed?
| #[cfg(feature = "wmemcheck")] | ||
| unsafe fn check_malloc_usable_size(store: &mut dyn VMStore, instance: &mut Instance, len: u32, addr: u32) -> Result<u32> { | ||
| check_free(store, instance, addr)?; | ||
| check_malloc(store, instance, addr, len) |
There was a problem hiding this comment.
Is this a correct implementation of malloc_usable_size's semantics? The man page on my system says that it returns the size of a memory block; unclear to me whether we need a hook for that, even, to track alloc'd/free state?
| for (_, entry) in instance.exports() { | ||
| if let wasmtime_environ::EntityIndex::Memory(mem_index) = entry { | ||
| let mem = instance.get_memory(*mem_index); | ||
| let out = *(mem.base.offset(outptr as isize) as *mut u32); |
There was a problem hiding this comment.
We definitely need to do a bounds-check here -- could you use the safe APIs for the memory to access the out-pointer?
This is also little-endian-specific code; we'll want to get a [u8; 4] and do an explicit conversion with u32::from_le_bytes. (Wasmtime supports one big-endian architecture, s390x!)
| } | ||
| unsafe { | ||
| print!("{:02x} ", *mem.base.offset((prev_malloc + i) as isize)); | ||
| } |
There was a problem hiding this comment.
This looks like code that could be useful for debugging, but as above with the posix_memalign out-pointer fetch, the memory accesses are unsafe, and also dumping the contents of previously allocated blocks might be too verbose. I think I'd prefer to omit this code for now; ultimately the right answer for any problem that needs examination of memory state like this is probably using a debugger (we have better support planned!) possibly with breakpoints on allocation events or trap-to-debugger on wmemcheck errors.
| @@ -140,6 +158,9 @@ impl Wmemcheck { | |||
|
|
|||
| /// Updates memory checker memory state metadata when free is called. | |||
| pub fn free(&mut self, addr: usize) -> Result<(), AccessError> { | |||
| if addr == 0 { | |||
There was a problem hiding this comment.
Good find! Can we add a comment here noting the implemented semantics (free of a null pointer is a no-op)?
crates/wmemcheck/src/lib.rs
Outdated
| @@ -101,12 +119,12 @@ impl Wmemcheck { | |||
| len: len, | |||
| }); | |||
| } | |||
| MemState::ValidToWrite => { | |||
| /* MemState::ValidToWrite => { | |||
There was a problem hiding this comment.
Why is this commented out? (If the checks were causing issues, could we put the exclusion under an appropriately named and documented option?)
crates/wmemcheck/src/lib.rs
Outdated
| pub fn malloc(&mut self, addr: usize, len: usize) -> Result<(), AccessError> { | ||
| pub fn malloc(&mut self, addr: usize, start_len: usize) -> Result<(), AccessError> { | ||
| // round up to multiple of 4 | ||
| let len = (start_len + 3) & !3; |
There was a problem hiding this comment.
Can we put this under an option? Perhaps "wmemcheck granularity" or similar?
I was recently tracking down a memory corruption bug in swift and found the wmemcheck tool to be very helpful. Since the runtime of swift is fairly large, it required some extra features to remove false positives. I was able to modify wmemcheck to be useful enough for the job.
I'd like to contribute the changes, many of which overlap with parts of issue #7037 . In particular, I needed to add the following:
The last 2 possibly should be hidden behind options, but I'm not familiar enough with the code base or how others use this tool to know what the right approach would be.