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

RFC First cut support for sandboxing procmacros with wasm #7297

Closed
wants to merge 2 commits into from

Conversation

jsgf
Copy link
Contributor

@jsgf jsgf commented Aug 25, 2019

This is a first cut experiment at building proc-macros with wasm in order to sandbox them.

It adds a new Kind: HostSandbox, in addition to Host and Target. This is treated as a host build, but its always wasm32-unknown-unknown.

In Cargo.toml, you can set wasm_sandbox = true in addition to proc-macro = true, which enables the use of HostSandbox.

When the compilation actually happens, it's treated as a cross-build to wasm32-unknown-unknown.

Lightly tested on serde, which worked until it couldn't find proc_macro from rustc (well, proc-macro2's build.rs assumes it can't exist and so fakes it out).

Assuming this basic approach is sound, I'm thinking that the wasm_sandbox flag will become a marker that the procmacro could be sandboxed, and then a command-line or cargo config option would request that procmacros be built sandboxed (with further config to forbid non-sandboxed).

TODO:

  • the rustc side
  • what happens if we're cross-compiling to wasm32? Does it conflict or is it ok?

cc @alexcrichton, @dtolnay

@rust-highfive
Copy link

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2019
@jsgf jsgf changed the title RFC First cut support for sandboxing procmacros with WASM RFC First cut support for sandboxing procmacros with wasm Aug 25, 2019
@jsgf
Copy link
Contributor Author

jsgf commented Aug 25, 2019

r? @alexcrichton

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

In the medium term, I would like for crates.io to distribute proc macros as precompiled wasm binaries -- which immediately eliminates all complaints about proc macros taking long to compile.

Is the implementation here forward compatible with a world where serde_derive = { version = "1.0", features = ["some", "features"] } serves me a precompiled wasm with the requested feature set? or do we need to ask that the macro author make some additional guarantees about the possible behavior of the macro when they opt in to wasm-sandbox = true.

@jsgf
Copy link
Contributor Author

jsgf commented Aug 25, 2019

If you assume features are additive then you could just set them all for prebuilt. Or if some are rare and bloaty then prebuilt could exclude them and you'd need to build locally if you want them.

IOW the prebuilt would have associated features and resolution would work out whether it's usable.

Of course it breaks if features are used wrongly.

Copy link
Member

@killercup killercup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool!

In the medium term, I would like for crates.io to distribute proc macros as precompiled wasm binaries

I'm looking forward to that!

src/cargo/util/toml/mod.rs Show resolved Hide resolved
@jsgf jsgf force-pushed the wasm-procmacro branch 2 times, most recently from d843872 to f5e49fe Compare August 26, 2019 04:24
@alexcrichton
Copy link
Member

Thanks for getting started on this @jsgf! I think this has the rough shape of what will be necessary, but I think that we'll probably want to pursue the rustc changes first since they're likely the much more meaty ones for now.

Some updates we'll need on the Cargo side eventually are:

  • The wasm-sandbox flag in the manifest will need a feature gate
  • This'll need to still work when the wasm32-unknown-unknown target isn't installed and no crates use wasm-sandbox
  • We'll either want to auto-fix or improve the error with wasm32-unknown-unknown missing and a crate using wasm-sandbox
  • I think we'll want to refactor Kind to just be InternedString instead of a Host or Target variant where the InternedString is just the target of compilation. Ideally Cargo could build all sorts of targets all at once but this "only kind or only host" limitation has been around for a bit too long at this point

I suspect we'll have some sort of different --crate-type feature in rustc itself to indicate this as well, and that'd be keyed off proc macros compiled for wasm. I think we've still got aways to go with this, but it's hopefully not that far away!


In terms of overall feature design I personally feel that precompiled wasm files distributed on crates.io is one of the biggest wins here and is one we'd want to flesh out before stabilizing everything. That doesn't necessarily needed to happen right here on this PR, but would likely be accompanied in the eventual RFC to flesh all this out.

For example I do have concerns about what @dtolnay was mentioning with features but I also have concerns about transitive dependencies. For example the precompiled wasm file might use syn 1.0.3 but if syn 1.0.4 is released later what would happen to the wasm file?

@jsgf
Copy link
Contributor Author

jsgf commented Aug 26, 2019

@alexcrichton I'm thinking that wasm-sandbox will become advisory - ie, this procmacro supports being sandboxed. Then something like cargo build --sandbox-procmacros would enable it (I guess feature gated, or just unstable), along with --require-sandboxed-procmacros to prevent unsandboxed procmacros.

I think we'll want to refactor Kind to just be InternedString instead of a Host or Target variant where the InternedString is just the target of compilation.

Maybe, but is some logic being applied to Kind in this diff - for example, the build script for a Kind::HostSandbox target will be Host. I'm not how that would look if it were just strings.

Now that this seems to have minimal correct functionality, I've moved into rustc. I'll put up a WIP RFC PR once I've made some headway. I expect it require further changes here.

The rationale for this is that rustc doesn't support proc-macro as
a crate type for wasm32, so use cdylib since its functioanlly
equivalent to the crate type we'd want. But it doesn't work because
a proc-macro crate also has language features that we still need
to make available.
@joshtriplett
Copy link
Member

Based on discussion in the Cargo meeting, note that we don't want to consider this in any way a security feature, only an isolation feature to make it easier to treat macros as functions from inputs to outputs.

@jsgf
Copy link
Contributor Author

jsgf commented Aug 29, 2019

@joshtriplett Totally on board with that characterization.

@bors
Copy link
Contributor

bors commented Sep 3, 2019

☔ The latest upstream changes (presumably #7216) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Sep 26, 2019
This commit is an internal refactoring of Cargo's compilation backend to
eventually support compiling multiple target simultaneously. The
original motivation for this came up in discussion of rust-lang#7297 and this has
long been something I've intended to update Cargo for. Nothing in the
backend currently exposes the ability to actually build multiple target
simultaneously, but this should have no function change with respect to
all current consumers. Eventually we'll need to refactor APIs of how you
enter the compilation backend to compile for multiple targets.
bors added a commit that referenced this pull request Sep 26, 2019
Refactor `Kind` to carry target name in `Target`

This commit is an internal refactoring of Cargo's compilation backend to
eventually support compiling multiple target simultaneously. The
original motivation for this came up in discussion of #7297 and this has
long been something I've intended to update Cargo for. Nothing in the
backend currently exposes the ability to actually build multiple target
simultaneously, but this should have no function change with respect to
all current consumers. Eventually we'll need to refactor APIs of how you
enter the compilation backend to compile for multiple targets.
@alexcrichton
Copy link
Member

alexcrichton commented Oct 23, 2019

Ok this has been quiet for some time now and in the meantime we've had watt watt emerge as well as a way of testing this. Naturally this is going to be a pretty major feature and I think we'll probably want to start with an RFC before the implementation here as well.

In any case we're certainly interested in continuing to investigate this as always, and we're more than willing to help discuss this in the future!

@auscompgeek
Copy link

@alexcrichton that links to rfc 2474, which doesn't seem right given the context. Did you mean to link to something else?

@dtolnay
Copy link
Member

dtolnay commented Oct 24, 2019

Here is the link: https://github.com/dtolnay/watt

@alexcrichton
Copy link
Member

Er yes sorry I apologize, @dtolnay has the right link.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants