memory fault in emmalloc prev_region #421
Comments
Wild guess: maybe UB-related optimization nerfed the code in the |
|
A couple of questions:
|
|
I have not seen any issues like this with dlmalloc. I haven't tried with a C program -- I imagine it would have the same issue, because Rust is just calling |
|
I can reproduce it with this C program: #include <stdlib.h>
int main() {
char *p = malloc(1);
for (unsigned i = 1; i < 20; ++i) {
p = realloc(p, 1 << i);
}
}It crashes at |
|
This one is a little different: #include <stdlib.h>
int main() {
char *p = malloc(1);
p = realloc(p, 12);
p = malloc(1);
} |
|
Ouch, yes, both test programs also crash when compiled with |
|
As a point of comparison both those programs run fine when build with emscripten + run under node. Perhaps something how about sbrk() differs? Or local diffs with emmalloc itself? |
|
One thing I found is that this calls Line 843 in 7018e24 In emscripten, the constructor adds at least one region, but that's commented out here in wasi: Lines 642 to 643 in 7018e24 I tried adding |
|
Just adding a couple |
|
By the way, you should declare |
|
I'm not optimizing the I have found that |
|
In the |
|
Oh, one other difference is that |
|
This barrier makes it work: @@ -1046,10 +1046,11 @@ static int attempt_region_resize(Region *region, size_t size)
assert(HAS_ALIGNMENT(newNextRegionStartPtr, sizeof(size_t)));
// Next region does not shrink to too small size?
if (newNextRegionStartPtr + sizeof(Region) <= nextRegionEndPtr)
{
unlink_from_free_list(nextRegion);
+ __asm__ __volatile__("":::"memory"); // barrier
create_free_region(newNextRegionStartPtr, nextRegionEndPtr - newNextRegionStartPtr);
link_to_free_list((Region*)newNextRegionStartPtr);
create_used_region(region, newNextRegionStartPtr - (uint8_t*)region);
return 1;
}There's quite a bit of type punning throughout -- maybe we should just add |
Shouldn't a barrier be added every time the size of a different region is accessed/modified? diff --git a/emmalloc/emmalloc.c b/emmalloc/emmalloc.c
index c98e42e..41dd308 100644
--- a/emmalloc/emmalloc.c
+++ b/emmalloc/emmalloc.c
@@ -302,10 +302,13 @@ static int compute_free_list_bucket(size_t allocSize)
return bucketIndex;
}
+#define MEMORY_BARRIER __asm__ __volatile__("":::"memory")
+
#define DECODE_CEILING_SIZE(size) ((size_t)((size) & ~FREE_REGION_FLAG))
static Region *prev_region(Region *region)
{
+ MEMORY_BARRIER;
size_t prevRegionSize = ((size_t*)region)[-1];
prevRegionSize = DECODE_CEILING_SIZE(prevRegionSize);
return (Region*)((uint8_t*)region - prevRegionSize);
@@ -318,6 +321,7 @@ static Region *next_region(Region *region)
static size_t region_ceiling_size(Region *region)
{
+ MEMORY_BARRIER;
return ((size_t*)((uint8_t*)region + region->size))[-1];
}
@@ -363,6 +367,7 @@ static void create_used_region(void *ptr, size_t size)
assert(size >= sizeof(Region));
*(size_t*)ptr = size;
((size_t*)ptr)[(size/sizeof(size_t))-1] = size;
+ MEMORY_BARRIER;
}
static void create_free_region(void *ptr, size_t size)
@@ -374,6 +379,7 @@ static void create_free_region(void *ptr, size_t size)
Region *freeRegion = (Region*)ptr;
freeRegion->size = size;
((size_t*)ptr)[(size/sizeof(size_t))-1] = size | FREE_REGION_FLAG;
+ MEMORY_BARRIER;
}
static void prepend_to_free_list(Region *region, Region *prependTo)
@@ -712,6 +718,7 @@ static void *attempt_allocate(Region *freeRegion, size_t alignment, size_t size)
// region as used memory, not leaving a free memory region behind.
// Initialize the free region as used by resetting the ceiling size to the same value as the size at bottom.
((size_t*)((uint8_t*)freeRegion + freeRegion->size))[-1] = freeRegion->size;
+ MEMORY_BARRIER;
}
#ifdef __EMSCRIPTEN_TRACING__
@@ -986,6 +993,7 @@ void emmalloc_free(void *ptr)
#endif
// Check merging with left side
+ MEMORY_BARRIER;
size_t prevRegionSizeField = ((size_t*)region)[-1];
size_t prevRegionSize = prevRegionSizeField & ~FREE_REGION_FLAG;
if (prevRegionSizeField != prevRegionSize) // Previous region is free?
@@ -1038,6 +1046,7 @@ static int attempt_region_resize(Region *region, size_t size)
// is a free region.
Region *nextRegion = next_region(region);
uint8_t *nextRegionEndPtr = (uint8_t*)nextRegion + nextRegion->size;
+ MEMORY_BARRIER;
size_t sizeAtCeiling = ((size_t*)nextRegionEndPtr)[-1];
if (nextRegion->size != sizeAtCeiling) // Next region is free?
{
@@ -1392,6 +1401,7 @@ static int trim_dynamic_heap_reservation(size_t pad)
return 0; // emmalloc is not controlling any dynamic memory at all - cannot release memory.
uint8_t *previousSbrkEndAddress = listOfAllRegions->endPtr;
assert(sbrk(0) == previousSbrkEndAddress);
+ MEMORY_BARRIER;
size_t lastMemoryRegionSize = ((size_t*)previousSbrkEndAddress)[-1];
assert(lastMemoryRegionSize == 16); // // The last memory region should be a sentinel node of exactly 16 bytes in size.
Region *endSentinelRegion = (Region*)(previousSbrkEndAddress - sizeof(Region)); |
I don't think the barrier is a real solution, more of a hack that I was using to figure out where it was going wrong. It's still UB to violate strict aliasing, but the barrier only works to impede the main optimization opportunity that would take advantage of aliasing. I think |
In Fedora, we are building wasi-libc with
MALLOC_IMPL=emmalloc, because dlmalloc's CC0 is problematic (#319). However, in further testing I have run into a null pointer crash in this emmalloc port. My reproducer is:MALLOC_IMPL=emmalloc[target.wasm32-wasi] wasi-root = "path/to/sysroot", and./x build library --target wasm32-wasi.cargo new --lib foo; cd foo.cargo +stage1 test --no-run --target wasm32-wasi.wasmtime -- ./target/wasm32-wasi/debug/deps/foo-*.wasm --help.I get similar crash running with
wasmertoo. I only see this when wasi-libc is built with-O2 -DNDEBUG(and I added-gfor backtraces), but it doesn't crash or hit any assertions without-DNDEBUG. I also tried with (rebased) #378 and it was no better.In that line of code, it seems clear that
0xfffffffcmust be a null pointer wrapping around by[-1].wasi-libc/emmalloc/emmalloc.c
Line 309 in aecd368
clang-analyzeralso finds a null pointer error on that line: report-3472e3.html.gz. That error path includes some of the initialization inclaim_more_memorythat's different than the original emscripten code, but it's not the same path as what I actually hit at runtime.Full wasmtime backtrace:
The text was updated successfully, but these errors were encountered: