Wasmtime GitHub notifications bot (Nov 20 2024 at 23:52): Wasmtime GitHub notifications bot (Nov 20 2024 at 23:52):cobbal opened PR #9641 from cobbal:cobbal/wmemcheck-updates to bytecodealliance:main:
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 https://github.com/bytecodealliance/wasmtime/issues/7037 . In particular, I needed to add the following:
- support for the other memory functions exported by wasi-libc (calloc, realloc, posix_memalign, aligned_alloc, malloc_usable_size) from https://github.com/WebAssembly/wasi-libc/blob/c47daaf6c69f82835fc92d9e14361803dfa794fa/dlmalloc/src/dlmalloc.c#L67
- tracking memory in 4-byte chunks instead of 1-byte. e.g. wasi-libc's strlen loads strings 4 bytes at a time, and possibly beyond the end of the string for efficiency.
- disabling the check for reading uninitialized memory.
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.
Wasmtime GitHub notifications bot (Nov 21 2024 at 00:00):cfallin requested cfallin for a review on PR #9641.
Wasmtime GitHub notifications bot (Nov 21 2024 at 03:10):cobbal updated PR #9641.
Wasmtime GitHub notifications bot (Nov 21 2024 at 22:51):cfallin submitted PR review:
Thanks very much for this! Some initial thoughts below.
I'm really happy
wmemcheckwas actually useful for you, and I think most of this can be upstreamed with no problem. Just a few thoughts below.
Wasmtime GitHub notifications bot (Nov 21 2024 at 22:51):cfallin created PR review 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.
Wasmtime GitHub notifications bot (Nov 21 2024 at 22:51):cfallin created PR review comment:
can we be more specific than "stuff" here?
Wasmtime GitHub notifications bot (Nov 21 2024 at 22:51):cfallin created PR review 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?
Wasmtime GitHub notifications bot (Nov 21 2024 at 22:51):cfallin created PR review comment:
This looks like code that could be useful for debugging, but as above with the
posix_memalignout-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.
Wasmtime GitHub notifications bot (Nov 21 2024 at 22:51):cfallin created PR review comment:
Good find! Can we add a comment here noting the implemented semantics (
freeof a null pointer is a no-op)?
Wasmtime GitHub notifications bot (Nov 21 2024 at 22:51):cfallin created PR review 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
callocandmalloc, namely that memory is zeroed?
Wasmtime GitHub notifications bot (Nov 21 2024 at 22:51):cfallin created PR review 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 withu32::from_le_bytes. (Wasmtime supports one big-endian architecture, s390x!)
Wasmtime GitHub notifications bot (Nov 21 2024 at 22:51):cfallin created PR review comment:
Can we put this under an option? Perhaps "wmemcheck granularity" or similar?
Wasmtime GitHub notifications bot (Nov 21 2024 at 22:51):cfallin created PR review comment:
Why is this commented out? (If the checks were causing issues, could we put the exclusion under an appropriately named and documented option?)
Last updated: Nov 22 2024 at 16:03 UTC