Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compartmentalizing unsafe from managed code #709

Closed
dcodeIO opened this issue Jul 6, 2019 · 7 comments
Closed

Compartmentalizing unsafe from managed code #709

dcodeIO opened this issue Jul 6, 2019 · 7 comments

Comments

@dcodeIO
Copy link
Member

dcodeIO commented Jul 6, 2019

With current master it is possible to mix unsafe and managed code at will, which can lead to all sorts of issues where unsafe code corrupts managed state. To account for this, it has been suggested to add a mechanism to either allow or disallow the mixing.

As ever so often, there are two perspectives to this.

  1. Once WebAssembly GC lands it can be expected that if everything becomes a managed object, unsafe code will not be able to interfere with its internals, and if it is possible to model the entire runtime on top of safe WebAssembly features that'll make the restrictions unnecessary.

  2. At this point, though, with the runtime living in linear memory alongside user code, there'll be use cases where an engine wants to restrict a user in a way that he cannot accidentally break it. It can also be expected that the runtime will stick around even if WebAssembly GC lands, to support environments that don't provide GC.

Hence I suggest to add a new compiler flag --allow-unsafe that is equivalent to the current behavior, a new compiler flag --disallow-unsafe that disallows the use of anything potentially unsafe in user code and a new default that acts like --allow-unsafe but emits warnings on unsafe code, essentially hinting a user into either using the --allow-unsafe or the --disallow-unsafe flag.

Standard library code would not have this restriction since it cannot implement anything without resorting to unsafe code. One detail here is that, currently, the only files being considered as stdlib files are the top-level files in lib directories, so I suggest to make the top-level files essentially contagious, so that every file imported from a stdlib file becomes a stdlib file as well.

For an implementer, allowing unsafe code in lib files means that, under the --disallow-unsafe condition, it is still possible to add unsafe code as library files using the --lib option, for example to more efficiently implement custom collections etc.

Open questions:

  • What should the compiler do if an external module, for instance a JSON parser, that is being imported relies on unsafe code? Should this always be disallowed or allowed depending on the flag, should it exclude any modules imports from the check, or should there be an option to define this on a per module basis?
  • Do we need a mechanism to annotate unsafe code, like being able to declare an unsafe block of statements? How would this integrate with the proposed compiler options?

As of the respective meta issue, this should probably become our first RFC.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 15, 2019

Looks that this change is less controversial than I initially thought, perhaps because it is backwards compatible anyway. On this basis, it seems that merging and delaying the overhead of doing an exemplary RFC is fine. Thoughts?

@MaxGraey
Copy link
Member

  1. I guess it should be per-module basis
  2. Do we need a mechanism to annotate unsafe code, like being able to declare an unsafe block of statements?
    Probably gust @unsafe for whole function is enough for first time.

Also I propose allow unsafe code without decorators for --runtime stub/none because it's only one way how you could write code without managed runtime. wdyt? Also people who prefer write simple examples will require add two flags --runtime none and --allow-unsafe which looks little bit verbose for me.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 15, 2019

  1. I guess it should be per-module basis

What it currently does is allowing unsafe code everywhere, except in user-provided code reached from the main entry file. Can't see the full picture yet where an embedder, like in blockchain, wants to seriously limit unsafe code. That might be done by either accepting contract code only and compiling it on a trusted machine, possibly requiring white listing external dependencies anyway (making per-module restrictions redundant), while allowing a user to provide a .wasm blob can never be safe in this sense anyway (making per-module restrictions unnecessary). Suggesting to wait with that until there's a concrete use case indicating where to go.

  1. [...] Probably gust @unsafe for whole function is enough for first time.

At this point it seems to me that @unsafe would undermine the checking in that it would allow anyone to use as much unsafe code as they like, essentially rendering the command line restrictions useless (to force safe code). I'd recommend anyone requiring unsafe code to put it into its own file and include it with --lib instead - so an embedder has some sort of handle on it by disallowing --lib for example. Again, I don't have the full picture yet and suggest to watch for real world requirements as these pop up.

Also I propose allow unsafe code without decorators for --runtime stub/none because it's only one way how you could write code without managed runtime.

Not sure that I understand this correctly. The none/stub runtime is a complete runtime with managed headers and whatnot, just no sophisticated MM/GC. It should be pretty much interchangeable with the full/stub runtime and requires no extra unsafe work per-se.

Also people who prefer write simple examples will require add two flags --runtime none and --allow-unsafe which looks little bit verbose for me.

I'd say that both the none + unsafe and none - unsafe scenarios are valid, so hardcoding none + unsafe seems wrong. Perhaps we should make configuration a bit more convenient by adding an assembly section to tsconfig.json that can hold all these options, instead of having to provide them on the command line?

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 15, 2019

Another option for the bottom concern might be to allow unsafe code by default, without even emitting warnings, so one must explicitly opt-in to unsafe restrictions via --disallow-unsafe. Not sure on this one, though, since proactively hinting on unsafe code now can have value if we ever decide to disable unsafe code by default, since most users will already know about the difference.

That's a relatively fundamental question related to what we talked about in the meta repo actually, because, depending on the outcome of future Wasm specs, we might either have to keep compartmentalizing the runtime or not. If we have to, emitting warnings makes a good guide, but if we don't have to, emitting warnings is not necessary. The fundamental part is whether we favor 1) giving access to low-level Wasm instructions or 2) providing a safe managed environment like that of other languages by default.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 17, 2019

Thinking about this a little more, it is likely that a user currently expects that they can just use load etc. because this is done frequently throughout the documentation and examples. Also, limiting unsafe code is specific to special scenarios, so allowing unsafe code by default (without an extra case of emitting warnings) and solely providing an option to restrict unsafe code where necessary might actually make sense at this point.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 21, 2019

Continuing the thread on the PR: #710

@dcodeIO dcodeIO unpinned this issue Jul 22, 2019
@dcodeIO
Copy link
Member Author

dcodeIO commented May 27, 2020

Closing this issue as part of 2020 vacuum because the --noUnsafe flag has landed meanwhile, enabling the use case.

@dcodeIO dcodeIO closed this as completed May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants