MSVC: Add support for linking against the "spectre-mitigated" CRT

[dpaoliello]

Issue Details:
Since VS 2017, MSVC has shipped a set of "spectre-mitigated" CRT static libs: https://devblogs.microsoft.com/cppblog/spectre-mitigations-in-msvc/

Typically these are used by opening a VS Command Prompt in "spectre mode" (https://docs.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-170#vcvarsall-syntax) which then sets the LIB environment variable to point to the directory with the spectre-mitigated libs. However, since cc prepends to the LIB environment variable, it uses the non-spectre-mitigated libs even when invoked from a VS Command Prompt in "spectre mode". This causes issues when trying to build a spectre-mitigated binary using Rust, as rustc uses cc for linking.

Fix Details:
When cc detects that the VSCMD_ARG_VCVARS_SPECTRE environment variable is set to spectre (either by being run from a VS Command Prompt in "spectre mode", or users may explicitly set this themselves), it will use the spectre-mitigated lib directory when building its LIB environment variable.

[dot-asm]

However, since cc builds its own LIB environment variable,

This doesn't seem to be totally correct. More specifically it doesn't appear to be building LIB from scratch, but prefixes existing one.

Fix Details: When cc detects that the VSCMD_ARG_VCVARS_SPECTRE environment variable is set to spectre (either by being run from a VS Command Prompt in "spectre mode", or users may explicitly set this themselves), it will use the spectre-mitigated lib directory when building its LIB environment variable.

I for one would argue for existing LIB simply taking precedence. I mean having cc-rs building own LIB variable only if it's not set already.

[Just in case, I'm just a cc-rs user weathering #663.]

[dpaoliello]

However, since cc builds its own LIB environment variable,

This doesn't seem to be totally correct. More specifically it doesn't appear to be building LIB from scratch, but prefixes existing one.

Ah, you're right, looks like I misread the code.

Fix Details: When cc detects that the VSCMD_ARG_VCVARS_SPECTRE environment variable is set to spectre (either by being run from a VS Command Prompt in "spectre mode", or users may explicitly set this themselves), it will use the spectre-mitigated lib directory when building its LIB environment variable.

I for one would argue for existing LIB simply taking precedence. I mean having cc-rs building own LIB variable only if it's not set already.

Unfortunately, appending to LIB would be a breaking change. There could be an option to opt-in to that behavior, but that option would also have to be exposed via rustc/cargo, so you'd still get the wrong behavior by default (and I'm not sure when you could switch that default).

Additionally, we'd still need something like this for folks who don't want to run cargo/rustc in a VS Command Prompt (i.e., some way for the auto-discovery to opt-in to spectre-mitigated libs).

[dot-asm]

Unfortunately, appending to LIB

The suggestion was not to append, but do nothing. Rationale basically is since one trusts %PATH% with cl.exe location, why wouldn't one trust %LIB% [and %INCLUDE%]. Yes, my position seems to be in need of refinement. If cl.exe is located through %PATH% and %LIB%/%INCLUDE% are defined, then do nothing. However...

would be a breaking change.

Well, the question is what exactly breaks? It should be noted that cc-rs merely compiles a library with cl.exe, which is then linked by rustc. Which has its own opinion about how to compose the %LIB% environment variable! In other words no matter what we make cc-rs do, as it stands now, rustc will override it. So that the issue actually belongs with rust-lang itself, not with cc-rs. I suppose we can only flesh out things here. For example...

It should be noted that Rust by itself doesn't seem to have dependencies on the spectre-mitigated libraries that Microsoft provides. llvm backend effectively adds dependency to vcruntime through the exception handling mechanism, but how valuable are the mitigations in this context? Assuming there are relevant compiled in exception handlers and helpers. But in either case there is a pros side to it. The limited dependencies means that as far as Rust itself is concerned there is not much one can break by messing up the %LIB%.

[ChrisDenton]

This PR seems reasonable to me. All it does is prefix one lib directory instead of prefixing another lib directory if a special environment variable is found. And this does indeed appear to reflect the intent of that environment variable.

IMHO, changing the behaviour of how cc-rs adds to LIB is beyond the scope of this PR and should probably be a separate issue.

[dot-asm]

This PR seems reasonable to me.

Once again. As it stands now, manipulating %LIB% in cc-rs achieves nothing. Because rustc, as it calls link.exe, effectively overrides the added library paths by prefixing %LIB% with otherwise equivalent paths that don't have spectre-mitigated libraries.

Now, it is possible to override %LIB% with /LIBPATH linker command-line options. And cargo even has a preferred way to generate those for you, by having the build script print cargo::rustc-link-search=native={path}. This is the way to achieve the intended goal, not manipulating %LIB%. [Just in case, I for one would still advocate for honouring %LIB%, but in rustc. And since rustc makes no use of cl.exe, it could honour %LIB% if link.exe was found on the %PATH%. But it's just me:-)]

As for opting in through an environment variable in general. Not that I'm one, but would you expect Visual Studio Code users to appreciate it? Wouldn't it be more appropriate if opt-in was more "Rust-y"? Through Cargo.toml, .cargo/config.toml or build.rs? For example, cc-rs could take the steps to ensure that spectre-mitigated libs are used if build.rs passed the /Qspectre option to the compiler...

[ChrisDenton]

I'm not sure I follow you. link.exe will not link anything unless you tell it to (this is unlike cl.exe). Both directories in the LIB environment variable and the /LIBPATH command line option are searched. /LIBPATH is searched before LIB which allows overriding any defaults.

If you're right that setting LIB achieves nothing then I really do think you should open an issue about that. Maybe with some test cases that show it's useless.

[dot-asm]

Keyword is that cc-rs doesn't call link.exe. It simply creates a library and tells rustc to link with it. [Or multiple libraries.] When rustc is about to call link.exe, it prefixes %LIB% with a couple of paths. And these additional paths have the same libraries that you intended to be spectre-mitigated. Thus they simply get shadowed by rustc. In other words setting %LIB%, be it here in cc-rs or through vcvarsall, doesn't get you where you want to go, rustc is in the way...

[ChrisDenton]

Again, I think you should open an issue about that. That code path will still exist whether or not this PR is merged.

[dot-asm]

you should open an issue about that.

Where? On rust-lang? Maybe I will, but I feel that it would be only appropriate if "we can [only] flesh out things here" and achieve some common ground. Maybe I'm missing something which would make filing an issue inappropriate... For example it might be deemed more than sufficient to adhere to /LIBPATH in cc-rs... I can see merit to it... While my suggestion is arguably quite radical and not [necessarily] without cons...

[ChrisDenton]

Open an issue here, if you like. I don't think this PR is the appropriate place to flesh out things as the issue exists regardless.

[dot-asm]

Open an issue here

??? Issue with what? With rustc? What would it achieve if rustc people are not here? Or issue with this PR? Is it inappropriate to discuss PR in PR thread? I don't follow, both options appear counter-productive to me. Well, never mind...

[ChrisDenton]

Fair enough. Opening an issue on the main rust repo be great.

[dot-asm]

I sense that there might be disagreement about what the issue actually is. I apologize that I failed to communicate it, but my understanding is that it would be [more than] appropriate to link with spectre-mitigated libraries specifically when you bring in C/C++ modules [presumably also compiled with /Qspectre, right?] into a Rust application. While the implied idea might be that there should be an option to link any Rust application with spectre-mitigated libraries. If the latter is the case, then I'd say that the merge request doesn't belong here. Because cc-rs has no bearing on how arbitrary Rust applications are linked. It's used and has a chance to affect linking only when the application interfaces with C/C++ code. But if considered in rust-lang context in isolation I would argue that linking with spectre-mitiaged libraries is hardly justified. Question is how much of the spectre-mitigated static[!] library code is linked into a pure Rust application? It appears to me that there is none. As already mentioned, a pure Rust application has only one dependency(*) on VC runtime, exception handling subroutines, but those are linked dynamically. This effectively means that the spectre-mitigated libraries are not actually used in a pure Rust application(*). In other words, it only makes sense to link with spectre-mitigated libraries when you bring non-Rust code into application. Hence my understanding from above. Does it make sense?

(*) Formally speaking there also is CRT initialization code brought into application. But I suppose that we can agree that spectre mitigations are not essential in the CRT initialization context. Because it's fully predictable and not dependent on any secret values. Nor does a pure Rust application make it call any sensitive constructors. And for completeness, Rust also takes memcpy/move/set/cmp from CRT, but even if not linked dynamically, I don't see any difference between spetre-mitigated and default object modules.

[dpaoliello]

Because cc-rs has no bearing on how arbitrary Rust applications are linked. It's used and has a chance to affect linking only when the application interfaces with C/C++ code.

That's not true: under the covers rustc uses cc-rs to find the linker to use and setup the linking environment, thus any change to how cc-rs handles linking changes how every Rust program linked by rustc is built.

The motivation for this change is that linking against the Spectre-mitigated CRT is compliance requirement enforced by BinSkim (https://github.com/Microsoft/binskim) - even if Rust at the moment doesn't rely on any API hardened for Spectre, that's no guarantee that it won't in the future or that any "pure" Rust application won't eventually link in non-Rust code that does.

[dot-asm]

That's not true:

Ah! I was looking for it, but apparently not hard enough. Live and learn. Thanks!

any change to how cc-rs handles linking changes how every Rust program linked by rustc is built.

Specifically in msvc environment.

Rust at the moment doesn't rely on any API hardened for Spectre, that's no guarantee that it won't in the future

Given the current state of affairs, we can be relatively confident that it won't be the result of an oversight but a conscious choice. In other words, even if there is no guarantee, it's very unlikely.

[ChrisDenton]

@dot-asm I think perhaps people have been talking past each other and misunderstanding each other. Could you clarify your current stance on this PR? I have maybe gotten confused somewhere along the line.

[dot-asm]

Even though I originally commented in ignorance of the fact that MSVC rustc itself has direct dependency on this crate (I thought that rustc had an equivalent way to invoke the vendor linker, kind of a trimmed down copy of this crate if you wish, not a direct dependency), I would still advocate against the suggestion in this specific form. Now bear with me. As already mentioned, it doesn't make any real difference if "pure" Rust application is linked with spectre-mitigated MSVC libraries. (Nor do I see this change in the foreseeable future.) And for this reason I would argue that it makes way more sense if the said libraries were linked only when it actually matters, specifically when the application actually interacts with external non-Rust code. And especially because one can arrange it in a way that is independent of rustc. So that users who care can start using the spectre-mitigated libraries at their discretion without having to wait for a future Rust release that would have to update its dependency on this crate. How? By letting cc-rs take a note if user passes /Qspectre to the MSVC compiler, and have it pass down paths to the spectre-mitigated libraries to rustc through /LIBPATH options (which take precedence over %LIB% as already established). As for users who don't need to actually compile anything but to link a pre-compiled library that is dependent on spectre-mitigated libraries, one can expose the method that passes /LIBPATH options for user to invoke from the build.rs. Letting users solve it in their build.rs is more reliable and transparent.

[dot-asm]

I would still advocate against the suggestion in this specific form.

Though formally speaking what I suggest is not in a direct conflict with the suggestion at hand. In sense that cc-rs could do what I suggest, and rustc could start doing what Daniel suggests, and things will work out either in the corresponding intended ways. So it's not like I'm literally "against" the suggestion, I just reckon there is a more sensible way to solve the problem :-)

[ChrisDenton]

I admit I do not understand the objection. We already use a fair bit of magic to either use the environment or to go out of our way to find the right libraries ourselves in a way that mimics the Developer console. For example, the SDK libraries used depends on various factors, either the environment variable is used or else it searches the registry for the latest (using the sort order).

I think it unlikely rustc will gain options to make all the msvc specific moving parts configurable but if it does, I see no problem in doing the right thing by default.

Besides, it's really weird that currently cc sometimes uses spectre libraries but sometimes doesn't. E.g. when the developer command prompt is set up for x86 and compiling for x64 (or vice versa) then cc will have to go searching for the right libraries to use (and therefore are never spectre-mitigated). Whereas if it's compiling x64 in the x64 prompt then the environment is used (and therefore libs are spectre-mitigated).

[dot-asm]

I do not understand the objection.

As mentioned in the second remark, it's not really an objection per se, but an argument for a possibly better solution. Where "better" means self-contained in cc-rs, more reliable and more transparent for the user.

I think it unlikely rustc will gain options to make all the msvc specific moving parts configurable but if it does,

Note that my suggestion doesn't require anything of rustc that it doesn't do today.

I see no problem in doing the right thing by default.

This naturally depends on definition of "the right thing" :-) Well, not that I claim an exclusive knowledge about what it is :-)

As for platform/target "crisscrossing." This ought to mean that there are situations when rustc simply does what I originally suggest, respect the %LIB% environment set by vcvarsall...

[ChrisDenton]

As for platform/target "crisscrossing." This ought to mean that there are situations when rustc simply does what I originally suggest, respect the %LIB% environment set by vcvarsall...

It can't. You can't use x86 libraries to build x64 applications.

[arlosi]

Using VSCMD_ARG_VCVARS_SPECTRE (as this PR does) seems like the safest way to get rustc to compile with the spectre mitigated libs. If the user is building within a spectre vcvars prompt, they'll get the correct behavior (even when cross-compiling).

Changing how cc-rs modifies LIB could also work, but may require more design work and testing to make sure we don't break existing uses.

[dot-asm]

If the user is building within a spectre vcvars prompt,

And the question is how big is the "if". That is how many Rust users are actually using vcvars prompt. That's the thing. Rust users don't have to use it, and suspicion is that very few would do. And I can imagine that they would appreciate the opportunity to control this through the build script more [than maintaining the discipline of invoking vcvars with right parameters]. This way there won't be ambiguity, did I compile this under vcvars or not? Or worse, did target user of my application run this under vcvars or not?

Changing how cc-rs modifies LIB could also work, but may require more design work and testing to make sure we don't break existing uses.

Formally speaking there is a contradiction. This merge request is all about changing how cc-rs modifies LIB, while the formulation suggests that it's not... Yeah, I'm just messing with you:-) I understand what you mean... But just in case, note that my final suggestion is to not change anything that would affect how rustc itself operates.

[ChrisDenton]

And I can imagine that they would appreciate the opportunity to control this through the build script more

Again, you're asking for cc-rs to add a feature which it doesn't currently have. That would be a new issue. This PR improves a feature cc-rs already has (using Visual Studio environment variables, if set).

[dpaoliello]

If the user is building within a spectre vcvars prompt,

And the question is how big is the "if". That is how many Rust users are actually using vcvars prompt. That's the thing. Rust users don't have to use it, and suspicion is that very few would do.

To be clear: this change doesn't require a vcvars prompt, a developer can set VSCMD_ARG_VCVARS_SPECTRE completely independently of vcvars and this will still work. But IF they use a vcvars prompt then cc-rs will now respect their choice to enable spectre mitigated libs.

And I can imagine that they would appreciate the opportunity to control this through the build script more [than maintaining the discipline of invoking vcvars with right parameters].

Since this is controlled via an environment variable, developers can control it via a build script: https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-env

[dot-asm]

developers can control it via a build script: https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-env

The keyword on the referred page is "when compiling the package." If you provide a library, setting the environment variable in question won't have any effect on final application link stage. Or in other words, you would have to convince your library users to set the environment variable in question in their build scripts. (And this is after you've convinced them to upgrade to a future Rust version ;-)

Anyway, I've opened #687.

[NobodyXu]

Seems fine by me, though I'm not very famili with windows, so I'd also like review from @thomcc

[thomcc]

Chris, can you take a look here? You've already been active, so maybe you have opinions.

[ChrisDenton]

Sorry it took me a minute to get back up to speed here.

Yes, I agree with these changes. We should attempt to respect the environment as much as possible. The code look good to me.

[NobodyXu]

Hello @dpaoliello could you resolve the merge conflicts please?

I will merge your PR once it is resolved

[dpaoliello]

@NobodyXu Done