wabt icon indicating copy to clipboard operation
wabt copied to clipboard

Per instance, cross platform library sandboxing support in wasm2c

Open shravanrn opened this issue 4 years ago • 9 comments

@kripken Per our discussion here is the PR with all the wasm2c changes we needed to make to allow wasm2c to be used to sandbox libraries in the Firefox browser. (Fyi @deian) Briefly here are some of the more notable changes made)

  • Windows/msvc support for runtime, codegen and builtins as well as support for a wider list of platforms/architectures/compilers including Android (arm architectures), Windows 32-bit etc.
  • Support for growable indirect function call tables
  • Support for library sandboxing apis --- wasm function type index lookup (so host can add new callbacks), make heaps aligned to 4gb for compatiblity with the RLBox sandboxing framework
  • Allow multiple sandbox instances of the same library by moving global vars into a context structure that is passed to every function
  • Migrated from emscripten to use of wasi-clang
  • Upstream libc-wasi does not support windows, so I have written a minimal cross platform wasi support which includes just basic wasi function (just enough to get simple IO libraries running. Nothing complicated like networking or filesystem is supported)
  • Removed use of per function call signal handler, setjmp/longjmp as this slows transitions
  • Removed name mangling to simplify function lookup
  • Remove stack depth counting as this adds unnecessary overhead
  • A debugging aid built directly into wasm2c generated code that is similar to valgrind like shadow memory (significantly eases debugging of wasm)
  • A wasm2c runner that can run full applications (code that has a main), when they are compiled via wasm2c to C and then to a shared library (.so/.dll)

I think this PR will likely be a starting point for discussion and we can discuss what we need to change/update so we can land this without issues

shravanrn avatar Oct 06 '21 06:10 shravanrn

Thanks for the PR @shravanrn !

Ideally this would be split into smaller pieces as @sbc100 said, but given that there has been almost no other work in upstream here, and there is little need for bisectability (the submitter is the main production user AFAIK), I think this is a rare case where it makes sense to land a single big PR.

Starting to read now...

kripken avatar Oct 06 '21 17:10 kripken

Thanks for the PR @shravanrn !

Ideally this would be split into smaller pieces as @sbc100 said, but given that there has been almost no other work in upstream here, and there is little need for bisectability (the submitter is the main production user AFAIK), I think this is a rare case where it makes sense to land a single big PR.

Starting to read now...

Just in terms of reviewing though it seem like there are two fairly large changes here being combined:

  1. Make existing wasm2c work on windows
  2. Extent wasm2c in significant ways.

If possible it would be great to separate though, but I can see how it could be difficult. If @kripken (who knows this code better than me) is prepared to review both at once I'll leave that up to him.

sbc100 avatar Oct 06 '21 18:10 sbc100

@sbc100

Just in terms of reviewing though it seem like there are two fairly large changes here being combined

Would it be practical to split those two, @shravanrn ? If so that sounds good to me. The work to split it out might be saved in being able to land one part first and not keep discussions on a single big PR (that waits on all items for landing).

But, if it's not practical to split out, I think it's ok for me to review as it is.

kripken avatar Oct 07 '21 20:10 kripken

Sorry for the delay! Some last minute deadlines on my side. I'll be back to updating this PR and addressing the comments in a day or two

shravanrn avatar Oct 14 '21 20:10 shravanrn

@kripken @sbc100 As you suggested, i think it may be easier to upstream this in batches. So, rather than landing this PR, @talg from our sandboxing team will be submitting PRs to upstream these changes in batches. This should allow us to more easily review and land things. Thanks @talg! :)

Let me know if you have any thoughts or concerns. Will close this PR if the above plan makes sense to you

shravanrn avatar Oct 26 '21 19:10 shravanrn

Sounds like a good plan to me!

kripken avatar Oct 29 '21 16:10 kripken