wasm,wasi: make sure buffers returned by malloc are not freed until f…

[anuraaga]

…ree is called

I am trying to create a wasm app that links two wasm binaries, one compiled from C++ with clang and one compiled from TinyGo. This is to reuse C++ libraries in a TinyGo app.

I found when doing this, that malloc/free that TinyGo implements to make sure they collaborate with the GC are not actually functional (this is probably what the comment meant by unimplemented) - free being a noop is the obvious hint, but basically the buffer returned by malloc is not referenced by Go so is ready to be GC'd right away, even though it needs to be able to survive until free is called. Hoping the note that if they're needed they can be implemented is true :) I believe these do need to be implemented properly to support polyglot wasm apps built from different compilers.

This PR changes to keep track of allocations within Go to make sure buffers are referenced until free is called. Then, they won't be referenced anymore and will be cleaned up when GC runs.

I couldn't find any similar wasi test in the repo, so would appreciate any pointer on how to write a test for this. I have this repository with a test, maybe it is OK to add as a subdirectory, with the wasm compilation wired to Makefile instead of checked in?

https://github.com/anuraaga/tinygo-wasialloctest

This test shows a malloc'd buffer becoming invalidated with current TinyGo, but not after this change.

[anuraaga]

For some context, running into actual problems because of this does require GC churn, which might be why there haven't been obvious crashes with libc code itself, e.g. fdopendir.

In the test case

https://github.com/anuraaga/tinygo-wasialloctest/blob/main/testdata/main.go#L15

10 or 100 were not enough but 1000 exhibited the issue. So quite a bit of churn was needed before corrupting the malloc'd memory.

[dgryski]

Thinking about other runtime assertions controlled by a build flag (for a different PR), do we want to add a flag to make sure that the pointers passed to free and realloc are valid pointers? (i.e, have entries in the map)

[anuraaga]

Thanks for the idea @dgryski - looking at wasi-libc, it looks like the default is to panic, and there is a PROCEED_ON_ERROR option

https://github.com/WebAssembly/wasi-libc/blob/659ff414560721b1660a19685110e484a081c3d4/dlmalloc/src/malloc.c#L90

As these are meant to be reimplementations of the wasi-libc functions, it's probably good to mimic at least its default behavior so I'll update to panic explicitly on illegal ptr if it makes sense. Could add a proceed-on-malloc-errors build tag or something but don't suspect it to be ever used really so leaning towards not adding unless asked for, what do you think?

[deadprogram]

@anuraaga can you please rebase this branch against the latest dev branch? That should allow it to pass the CI tests.

[anuraaga]

Thanks @deadprogram hadn't noticed that branch, rebased and set the PR base to dev

[anuraaga]

It's not similar to any other tests for being wasi-specific but was able to add a simple one, let me know if the pattern is OK. Because it's all within a single TinyGo program, some care is needed to ensure the GC doesn't end up tracking the malloc'd ptr anyways (I found it did this with a defer, even when calling through a //go:noinline function accepting uintptr 🤷 ). It fails on dev but passes with this PR.

[dgryski]

I'll review this today.

[anuraaga]

Hi @dgryski - just a friendly reminder in case you have a chance to review this. Thanks!

[dgryski]

A few questions, but otherwise LGTM

[dgryski]

Should we change this (and the realloc panic below) to include the routine that's complaining? "free: invalid pointer" ?

[dgryski]

I feel that for safety, we should do the copy before deleting from the allocs map.

[anuraaga]

Thanks a lot @dgryski - addressed the comments

[dgryski]

LGTM

[deadprogram]

Thank you for working on this @anuraaga would you like to squash these commits yourself before merge, or would you prefer that we do it for you?

[anuraaga]

Thanks @deadprogram - if it's OK, it'd be nice if you can squash before merge :)

[deadprogram]

Done, thanks again @anuraaga for the code and to @dgryski for review.

[aykevl]

This PR resulted in an almost 4kB increase in code size for small code samples. I have avoided most of this increase in this PR: #3162