Bad things happen when you exceed the size of the scratch arena #146

New Issue

bvisness · 2023-09-27T20:48:32Z

bvisness commented

2023-09-27 20:48:32 +00:00

I attempted to allocate more memory than the scratch pool could contain, and got bad accesses. This makes me sad.

It seems like it should either behave like a ring buffer, or just assert that we haven't overflowed our backing memory?

I attempted to allocate more memory than the scratch pool could contain, and got bad accesses. This makes me sad. It seems like it should either behave like a ring buffer, or just assert that we haven't overflowed our backing memory?

MartinFouilleul commented

2023-09-27 23:03:37 +00:00

it should allocate a new chunk (calling oc_mem_grow if needed) and continue allocating from this chunk...

are you on latest main?
is allocating a buffer > 1G in on_init() sufficient to repro? Otherwise could you give me a small repro case?

it should allocate a new chunk (calling `oc_mem_grow` if needed) and continue allocating from this chunk... - are you on latest main? - is allocating a buffer > 1G in on_init() sufficient to repro? Otherwise could you give me a small repro case?

bvisness commented

2023-09-27 23:35:18 +00:00

I'm not sure how to easily send you a repro, but this is the structure of what failed:

oc_arena_scope scratch = oc_scratch_begin();
void* base = oc_arena_push(scratch.arena, 1024);
u64 len = 1024;
for (int i = 0; i < something; i++) {
  oc_arena_push(scratch.arena, 1024);
  len += 1024;
}
memcpy(someDest, base, len); // this crashed for me
oc_scratch_end(scratch);

The issue, presumably, was reading off the end of the allocated memory. I would have expected a crash or error log or something earlier while pushing chunks onto the arena.

I'm not sure how to easily send you a repro, but this is the structure of what failed: ```c oc_arena_scope scratch = oc_scratch_begin(); void* base = oc_arena_push(scratch.arena, 1024); u64 len = 1024; for (int i = 0; i < something; i++) { oc_arena_push(scratch.arena, 1024); len += 1024; } memcpy(someDest, base, len); // this crashed for me oc_scratch_end(scratch); ``` The issue, presumably, was reading off the end of the allocated memory. I would have expected a crash or error log or _something_ earlier while pushing chunks onto the arena.

MartinFouilleul commented

2023-09-28 10:47:55 +00:00

Oh, ok. You just can't do that. Allocations are not guaranteed to be contiguous, because the arenas implementation must linked chunks within the current contraints of wasm memory.
So (assuming you were writing something to those allocations), you were at some point overwriting the header of the next chunk, meaning the next allocations would bound check on a garbage chunk cap. I was able to repro this, here's a log of what happens when your buffer catches up with the next chunk header:

Info: oc_on_init() in src/main.c:61: iteration 1085, base = 4522024, len = 1113088
Info: oc_arena_push() in ../../src/util/memory.c:102: try push size = 1024: chunk->ptr = 5636096, chunk->offset = 61480, chunk->cap = 1581056
Info: oc_arena_push() in ../../src/util/memory.c:107: nextOffset = 62504, chunk->cap = 1581056
Info: oc_arena_push() in ../../src/util/memory.c:149: end alloc: chunk->ptr = 5636096, chunk->offset = 62504, chunk->cap = 1581056
Info: oc_on_init() in src/main.c:61: iteration 1086, base = 4522024, len = 1114112
Info: oc_arena_push() in ../../src/util/memory.c:102: try push size = 1024: chunk->ptr = 5636096, chunk->offset = 62504, chunk->cap = 15996785876421582848
Info: oc_arena_push() in ../../src/util/memory.c:107: nextOffset = 63528, chunk->cap = 15996785876421582848
Info: oc_arena_push() in ../../src/util/memory.c:149: end alloc: chunk->ptr = 5636096, chunk->offset = 63528, chunk->cap = 15996785876421582848

You can see that after iteration 1085, the end of your buffer (which lags behind the actual memory allocated by the arena) overlaps the beginning of the current arena chunk. The next iteration the chunk has garbage cap, so way later you can allocate past wasm memory.

So... I don't know what we should/could do about this. At the very least we should emphasize this in the doc. We could also have canaries in the chunk headers in debug mode to detect that kind of things, but ultimately, there's not much we can do (at least in C) to catch that kind of error before it happens (for instance we can't check that people correctly use memory returned by malloc either).

As to what you should do to achieve what you want, if you need your data in a continuous array and can't compute the size beforehand at all, you should chain your allocations in a list, and do a collect pass once you know the total size.

Oh, ok. You just can't do that. Allocations are not guaranteed to be contiguous, because the arenas implementation must linked chunks within the current contraints of wasm memory. So (assuming you were writing something to those allocations), you were at some point overwriting the header of the next chunk, meaning the next allocations would bound check on a garbage chunk cap. I was able to repro this, here's a log of what happens when your buffer catches up with the next chunk header: ``` Info: oc_on_init() in src/main.c:61: iteration 1085, base = 4522024, len = 1113088 Info: oc_arena_push() in ../../src/util/memory.c:102: try push size = 1024: chunk->ptr = 5636096, chunk->offset = 61480, chunk->cap = 1581056 Info: oc_arena_push() in ../../src/util/memory.c:107: nextOffset = 62504, chunk->cap = 1581056 Info: oc_arena_push() in ../../src/util/memory.c:149: end alloc: chunk->ptr = 5636096, chunk->offset = 62504, chunk->cap = 1581056 Info: oc_on_init() in src/main.c:61: iteration 1086, base = 4522024, len = 1114112 Info: oc_arena_push() in ../../src/util/memory.c:102: try push size = 1024: chunk->ptr = 5636096, chunk->offset = 62504, chunk->cap = 15996785876421582848 Info: oc_arena_push() in ../../src/util/memory.c:107: nextOffset = 63528, chunk->cap = 15996785876421582848 Info: oc_arena_push() in ../../src/util/memory.c:149: end alloc: chunk->ptr = 5636096, chunk->offset = 63528, chunk->cap = 15996785876421582848 ``` You can see that after iteration 1085, the end of your buffer (which lags behind the actual memory allocated by the arena) overlaps the beginning of the current arena chunk. The next iteration the chunk has garbage cap, so way later you can allocate past wasm memory. So... I don't know what we should/could do about this. At the very least we should emphasize this in the doc. We could also have canaries in the chunk headers in debug mode to detect that kind of things, but ultimately, there's not much we can do (at least in C) to catch that kind of error before it happens (for instance we can't check that people correctly use memory returned by malloc either). As to what you should do to achieve what you want, if you need your data in a continuous array and can't compute the size beforehand at all, you should chain your allocations in a list, and do a collect pass once you know the total size.

bvisness commented

2023-09-28 17:32:10 +00:00

Sounds good, thanks!

bvisness closed this issue

2023-09-28 17:32:10 +00:00

Sign in to join this conversation.