Library sandboxing changes - support for per instance, cross platform, wasi, some debugging

[shravanrn]

List of features

• 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
• Option to 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)

[shravanrn]

Rebased PR for updates to wasm2c to support library sandboxing (fyi @deian)

@kripken @sbc100 Here is the rebased changes. I think in our discussion last time, we tried to see if we could separate the commits. I think this is proving too complicated/too time consuming. I think we are probably better landing this as a commit and continuing to fix or tweak the design here as needed. If possible could we continue the PR we started in #1721

I believe this should also proving the per-instance support being looked at in #1814

I have made sure the fac example has also been updated, but I am less clear on how we need to update any wasm2c specific unit tests. Please let me know what I need to do here, and I can update this PR as needed.

[kripken]

I'm interested to hear what others think, but I am worried about landing all of this together. I thought it could be a reasonable decision last year, but that was when no other code was landing at all, so I saw it as a "reboot" of a project with no users. We now know there are users, and in fact we have wasm2c PRs coming in and landing as we speak.

What changed since this comment where the plan was to split things up? The back-and-forth is confusing me.

But with all that said, if everyone else is on board with a single large PR, then I could be ok with it. cc'ing people for feedback: @yhdengh @keithw @sbc100

[shravanrn]

What changed since #1721 (comment) where the plan was to split things up? The back-and-forth is confusing me.

The honest answer is that such a split up requires a lot of dev cycles which are hard to come by at the moment. In fact, the reason for the delay in the prior PR is that the work to split this up kept getting pushed in favor of more feature work, bug fixes etc. which is something I fear may happen again if we choose to attempt to split this up. If everyone is comfortable, my suggestion is that we look at a way to include this as one PR as this would greatly simplify the development effort. For instance, upcoming feature work (we are looking at SIMD and some more debugging functionality) can be done directly against the wabt repo rather than on our fork.

[yhdengh]

Thanks for looping us in. We visited @shravanrn and @deian yesterday and had a chance to talk this over in person. From our point of view, we like some of the features, but it would be nice if this pull request can be done in a more incremental/understandable way. For module instancing specifically, we'd like host functions to access the whole instance, including all memories and globals, and each instance can have different imports, which is how we ended up with the design #1814 . Happy to discuss in more detail.

[kripken]

@yhdengh

Thanks for looping us in. We visited @shravanrn and @deian yesterday and had a chance to talk this over in person. From our point of view, we like some of the features, but it would be nice if this pull request can be done in a more incremental/understandable way. For module instancing specifically, we'd like host functions to access the whole instance, including all memories and globals, and each instance can have different imports, which is how we ended up with the design #1814

Is there a difference in terms of how host functions work compared to #1814 ? I believe both add an instance parameter to each function, but it is a little hard to see in this large PR to compare. Perhaps attaching some sample output code would help @shravanrn ? (we have sample code for the other PR already) But this may be something you have already discussed offline meanwhile.

I agree making the PR more understandable is important. Just one possible idea, perhaps annotated C output could help, like just mentioned, maybe with annotations/explanations? Just a thought.

Overall I am hoping that this PR is compatible with your goals @yhdengh , or that we can make it so. After the discussions so far I think it is more work to split this PR up than is practical for @shravanrn atm, and landing it will be of substantial benefit for the largest user of the code (Firefox, who are shipping using a fork right now, and who could stop using a fork). So while I definitely agree than smaller PRs would be better, I support landing it as one PR. But we do need to make sure that doing so will not cause problems or limit any of the use cases people have right now (like multi-memory).

[keithw]

I think from our perspective (the Stanford group working on this project, i.e. me, @yhdengh, @angelamontemayor), we love Firefox, and we greatly respect our UCSD colleagues and enjoyed meeting with them, but we don't see a route forward here that doesn't involve somebody doing a bunch of work.

Our sense is that if you want to pass all the spec tests while providing the ability to instantiate the same WebAssembly module multiple times (allowing each instance to have its own imports and exports), the approach ultimately will end up spiritually resembling the one we took in #1814 . We don't doubt that the code in this PR works great for the Firefox sandboxing use case, but based on our own experience, we think it will take some substantial code and API changes for it to pass the spec tests, especially the ones that test imports and exports.

E.g., the code in this PR gives all instances the same type name. We suspect this will probably need to change in order to pass the tests that depend on the ability to export from one module and import into another. This code also puts imports and exports in the global scope, which isn't something the spec tests prevent, but it's also something that needs to be changed if you want the host to be able to keep multiple instances of a module around and let each instance have its own imports and exports.

There's some other stuff here that we also don't love (we really don't like hardcoding a bunch of state for a particular WASI implementation inside the instance structure -- we don't like tying wasm2c to wasi_snapshot_preview1, much less a particular implementation of wasi_snapshot_preview1), but the big thing is that, at least in our view, this code is going to have to change in some semi-fundamental ways before it can pass the testsuite, as opposed to supporting the particular sandboxing use-case, and it's hard to concretely talk about the differences when the PR here has to be considered provisional in that sense. I realize our UCSD colleagues don't necessarily share that view, but to know for sure, somebody's going to have to do the work of making the tests pass so we can see.

Here's something maybe we can contribute: we can try to rebase the #1833 PR on top of our module-instancing approach in #1814, in a way that still compiles (and if UCSD/Mozilla has tests for this code that pass now, we can keep those passing). That wouldn't prove that all of #1833 is mergeable and can pass the spec tests, but it would at least break it up and demonstrate that this code can be built on top of a "test-passing" approach to module-instancing without harming anything.

[sbc100]

Regarding wasi support.. we already include third_party/uvwasi as a submodule which can be used as a wasi implementation. I would hope that we would no need another one. I also agree that it would be good is including wasi was a core part of wasm2c, but something that we can opt into. We already have WITH_WASI config option which enables wasi in the interpreter, perhaps that could also be used to optionally enable WASI in wasm2c?

@keithw, what you say about instancing makes sense, and I think spec conformance should certainly be our goal (at least by default). I also like you idea of trying to rebase this change on top of yours. We really should make this PR pass all the test first though, so we can confirm that the rebase was successful. Hopefully we can get this PR passing all tests by the end of the week.

[sbc100]

It looks like pretty much all the wasm2c tests are failing.

I would start by looking at ./test/run-tests.py test/wasm2c/address-overflow.txt and then move onto the rest of the failures which are spec test failures (e.g. ./test/run-tests.py test/wasm2c/spec/nop.txt).

(We will obviously be able to discuss this in more depth of Friday but if you feel like getting a head start that is where I would start).

[sbc100]

Can you revert this file and submit this as a separate PR if you think there is a need for it. I for one am not a fan of .gitignore files that match a broad set of files like this, but we can have that discussion outside of this PR.

[shravanrn]

Will fix

[sbc100]

Can you explain the utility of this helper? Why compile the the wasm2c code to a dll and then load it dyamically? Why not just compile it with a main.c and run its like that? Maybe there is some use case there its useful to have the wasm2c output be built as a dll that I'm not getting.

[shravanrn]

This is a pattern that is common in many sandboxing toolchains. Lucet is probably the easiest to look at https://github.com/bytecodealliance/lucet/blob/main/lucet-wasi/src/main.rs

The goal of the runner is to give developers an easy way to test simple applications without changing their code. The runner uses a default Wasm configuration (instantiates a heap and the runtime etc) and would execute any wasm2c compiled module that includes a main. The expected workflow is that developers can simply compile their existing application to a wasm module, generate C with wasm2c, compile the result to a dll and run it with no modification.

[sbc100]

I guess the separation of runtime from compiled module is the key benefit here.. since it allows the compilation of the module and runtime to be separated. But it seems that benefit is largely proportional to size of the runtime itself and how likely it is that you would want to iterate on the runtime without recompiling the wasm2c code.

In practice, have you found this useful with wasm2c?

[shravanrn]

Yup! I think there are couple of positives here

• You are absolutely right in that the main use case is to avoid compiling wasm2c multiple times for each app. In particular, I was compiling SPEC from wasm at some point and this was a useful way to not have to compile wasm2c repeatedly for each of the SPEC benchmarks.
• One additional (minor) benefit is that having a runner is nice as it simplifies tutorials --- hello_world need not explain what files are needed to compile the wasm2c runtime etc.

That said, compiling wasm2c doesn't take that long and is not that complicated, so its not the end of the world to have to compile it multiple times. Other Wasm compilers like Lucet really needed a runner as recompiling Lucet (which is written in rust) takes a while.

Given this, I think a runner would be nice to have but not a strict necessity.

[kripken]

@keithw Thanks for offering to do that rebase! I think that would be very useful to help decide on the path forward here.

Overall, I think that getting all tests passing here + at least some amount of testing of any new features added would be a hard requirement for landing this. I had not considered that it might take nontrivial work to get to that point, so that would be good to estimate. If it looks like that can't be done in a reasonable amount of time, then perhaps this PR can wait until there are more resources available to help out. If such a delay is a concern for the Firefox people then perhaps we should discuss with them directly (@shravanrn perhaps you can cc them here?).

If we are considering such a delay then I think it might be a reasonable path forward to land #1814 meanwhile - instancing by itself is quite useful and the approach there seems right to me.

But again, this would all depend on the state of testing here from my perspective. @sbc100 sounds like you are more optimistic?

[binji]

I just want to add that although I'm not working on wabt for my job anymore, I'd like to help here where I can (e.g. reviewing some of @sbc100's PRs). My latency won't be great though since I have to do it off work hours.

[sbc100]

I met with @shravanrn today and proposed a way forward that allows for more flexibility around how we land this change. The plan is to create a medium term branch upstream here in the wabt repo (I already created it actually: https://github.com/WebAssembly/wabt/tree/rlbox). This branch contains the exact code the FF is using today. The next commit on that branch will be the contents of this "rebase/merge" PR.. followed by changes that get all the tests passing on that branch.

In the mean time I'm planning on cherry-picking/integrating parts of the branch over onto main. For example, I think we can land the win32 really quickly (I have a WIP PR here: #1843).

Changes from main will continue to be merged in the rlbox branch up until the point that all changes have been integrated and the branch is not longer needed. FF can pull new revisions from the rlbox branch over time and we will try to add more tests to wabt to ensure we don't regress the FF use case. The exact nature of these new tests is still TBD.

The upshot is that I think this PR can be closed for now.

[sbc100]

@keithw, your offer to rebase/unify your instancing work with this PR would be be much appreciated, but not that we now have the rlbox branch on which you can work.

IIUC @shravanrn is going to take care of updating the rlbox branch to match current contents of the this PR (unless you want me to do that @shravanrn?)

[sbc100]

Next steps:

1. Update rlbox branch to match the contents of this PR
2. Close this PR
3. Have firefox start working off of a pinned revision from rlbox branch
4. Work on rlbox branch to get all tests passing and also continue to submit merge PRs to keep it up-to-date with main

[keithw]

@keithw, your offer to rebase/unify your instancing work with this PR would be be much appreciated, but not that we now have the rlbox branch on which you can work.

Acked. @shravanrn, what would be most helpful here -- do you want us to wait until the wasm2c tests are passing and then we'll attempt a rebase that preserves that, or if not, do you have a test that demonstrates intended/successful use of the RLBox API that we can make sure to keep passing post-rebase?

[sbc100]

@keithw, your offer to rebase/unify your instancing work with this PR would be be much appreciated, but not that we now have the rlbox branch on which you can work.

Acked. @shravanrn, what would be most helpful here -- do you want us to wait until the wasm2c tests are passing and then we'll attempt a rebase that preserves that, or if not, do you have a test that demonstrates intended/successful use of the RLBox API that we can make sure to keep passing post-rebase?

I think it would be good to get all tests passing on the rlbox branch, and have it be completely up-to-date with the current main branch before trying to do any integration of the two instancing approaches.

I like the PR that @keithw just posted (#1853) that increases wasm2c test coverage.. once that lands we can have move confidence while iterating.

[shravanrn]

Sorry for the delay here. Responses below

what would be most helpful here -- do you want us to wait until the wasm2c tests are passing and then we'll attempt a rebase that preserves that.

So i did a quick test fixing session last weekend that got about half the tests working. I'll check that in to the rlbox tree today. I will need to just a little more work to get the remaining tests working and check that in this weekend.

Rebasing on this branch after later today (or this weekend if you prefer to wait till the bulk of the tests pass) is probably reasonable a good place to start

Do you have a test that demonstrates intended/successful use of the RLBox API that we can make sure to keep passing post-rebase?

I will include a quick way to run the RLBox test suite as part of the "rlbox" branch this weekend. For now, this may be a quick hacky test to ensure functionality is preserved. We can figure out the best way to integrate this into the CI going forward.

[shravanrn]

Closing this PR per discussion above

[sbc100]

Sorry for the delay here. Responses below

what would be most helpful here -- do you want us to wait until the wasm2c tests are passing and then we'll attempt a rebase that preserves that.

So i did a quick test fixing session last weekend that got about half the tests working. I'll check that in to the rlbox tree today. I will need to just a little more work to get the remaining tests working and check that in this weekend.

Rebasing on this branch after later today (or this weekend if you prefer to wait till the bulk of the tests pass) is probably reasonable a good place to start

Do you have a test that demonstrates intended/successful use of the RLBox API that we can make sure to keep passing post-rebase?

I will include a quick way to run the RLBox test suite as part of the "rlbox" branch this weekend. For now, this may be a quick hacky test to ensure functionality is preserved. We can figure out the best way to integrate this into the CI going forward.

BTW, I got the full spec tests passing on win32 yesterday: #1843 .. the tricky part of handling of NaN in MSVC's math library functions such as truncf. Feel free to grab what ever bits from that PR that you need. I'm hoping to land it soon on main.