-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Detect when the initial binary launch may be slow on macOS #15908
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
base: master
Are you sure you want to change the base?
Conversation
r? @weihanglo rustbot has assigned @weihanglo. Use |
FYI we tend to prefer small but atomic commits. Feel free to edit your commits through the review process. |
@@ -69,6 +70,7 @@ libgit2-sys = "0.18.2" | |||
libloading = "0.8.8" | |||
memchr = "2.7.5" | |||
miow = "0.6.0" | |||
objc2 = "0.6.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a dependency on the objc2 ecosystem (objc2, block2 and objc2-encode), which I maintain. They are fairly widely used, and Mozilla gfx-rs/wgpu#5641, though I acknowledge that this could still be seen as a supply-chain attack. Implementing the execution policy check without these crates is doable, though a lot more code.
# Disable warning, e.g. if using a workplace-issued Mac that | ||
# doesn't allow granting Developer Tool permissions. | ||
[build] | ||
detect-antivirus = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should the config option be named? I went with build.detect-antivirus, an alternative would be to put it under [term]. Or maybe build.detect-slow-bin-first-run, to focus on the intention instead of what it does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is we move forward with an [advice]
table, mirroring git's. It would be ideal if we could suggest a command to run to turn off the advice (rather than saying how to edit a file)
I know [advice]
has at least come up at #15887 (comment), unsure where else.
I can try to split it up further, maybe as:
Or would you prefer a different order / more splitting? |
To clarify, I feel the current commits aren't too atomic
You could either
|
4b6f790
to
14df569
Compare
I've tried to reorder and split things in a way that maybe makes a bit more sense:
|
gtcx.shell().note( | ||
"detected that XProtect is enabled in this session, which may \ | ||
slow down builds as it scans build scripts and test binaries \ | ||
before they are run.\ | ||
\n\ | ||
If you trust your packages and their dependencies, then this \ | ||
overhead can be avoided by giving your terminal more permissions \ | ||
under `System Preferences > Security & Privacy > Developer Tools`. \ | ||
(Cargo has made an entry for your current terminal appear there \ | ||
already, though you will need to go and manually enable it).\ | ||
\n\ | ||
Alternatively, you can disable this note by adding \ | ||
`build.detect-antivirus = false` to your ~/.cargo/config.toml.\ | ||
\n\ | ||
See <https://support.apple.com/en-gb/guide/security/sec469d47bd8/web> \ | ||
for more information.", | ||
)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs further work on the wording!
Previously discussed in #15908 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with "session" over "terminal", since this term also applies if you're connecting via ssh (or running Cargo directly as a launchd agent). WDYT?
(Technically, I think it's something like "process context" or "TCC entitlement inheritance state", but the other terms are probably clearer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, never mind, "terminal" is probably clearer for the user, so instead I've used "session" at the beginning of the diagnostic, and "terminal" later.
Current wording:
note: detected that XProtect is enabled in this session, which may slow down builds as it scans build scripts and test binaries before they are run.
If you trust the software that you run in your terminal, then this overhead can be avoided by giving it more permissions under `System Preferences > Security & Privacy > Developer Tools`. (Cargo has made an entry for the current terminal appear there, though you will need to go and manually enable it).
Alternatively, you can disable this note by adding `build.detect-antivirus = false` to your ~/.cargo/config.toml.
See <https://doc.rust-lang.org/cargo/appendix/antivirus.html#xprotect> for more information.
b565d43
to
b922792
Compare
Thanks for your work on this! At a high level, its looking good! Note that I've not dug into the unsafe code and the cargo team needs to discuss this further. |
673e865
to
b9c00b7
Compare
if let Err(err) = detect_antivirus::detect_and_report(gctx) { | ||
// Errors in this detection are not fatal. | ||
tracing::error!("failed detecting whether binaries may be slow to run: {err}"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit unsure whether the end of create_bcx
is the right place for this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason for the concern?
Note that we have another type of check in here, the rustc compatibility one which you put this right after so that works from a consolidating environment checks perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's the right place, but I just wanted to call to attention that I'm not familiar with Cargo's internals, and that there might be a more suited place? But if you say it's correct, then it's correct.
src/cargo/ops/cargo_compile/mod.rs
Outdated
// Heuristic: Only do the check if we have to run more than a specific | ||
// number of binaries. This makes it so that small beginner projects | ||
// don't hit this. | ||
// | ||
// TODO(madsmtm): What should the threshold be? | ||
// | ||
// We also don't want to do this check when installing, since there | ||
// might be `cargo install` users who are not necessarily developers | ||
// (and so the note will be irrelevant to them). | ||
if (10 < num_binaries && build_config.intent != UserIntent::Install) | ||
|| build_config.detect_antivirus == DetectAntivirus::Always | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want this heuristic, what should the threshold be?
As a data point, a simple binary using reqwest
(in blocking mode) currently requires 10 build scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One challenge with this heuristic is that tests, doctests, and build scripts are very different
For tests (in cargo test
s model), the overhead of the test itself is likely to dwarf XProtect. We don't have parallel test running yet (working on the foundation to allow it) which defers that problem.
For doctests, 2024 edition at least reduces the number of binaries. However, there is no caching of the binaries that are built (yet) so there are no binaries on every run which is a problem if there is any memory to XProtect.
Both of the above may not be relevant if the user is doing a cargo check
or even a cargo build
which is what this location is for.
Regarding build scripts, I wouldn't expect those in a beginner project except through dependencies which can really blow up the numbers with deps like reqwest
atm. After #14948, beginnings should hopefully only deal with build scripts for -sys
packages and I have a likely unrealistic dream that we can consolidate those down to 1. Larger projects may also have build scripts for gathering commit information and adding a windows manifest.
For build scripts, the single threaded nature of XProtect becomes a problem for build scripts that are likely to run in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My counter proposals
- no binaries check
- only check build scripts for now and note in
cargo test --all
should run tests in parallel #5609 that we should add a check like this tocargo test
, and either- check if any build scripts (
0<num_binaries
) - check a small number (e.g.
4<num_binaries
)
- check if any build scripts (
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One challenge with this heuristic is that tests, doctests, and build scripts are very different
Just throwing an idea out there, we could make it a weighted sum instead?
let binary_weight = unit_graph.keys().filter(|unit| match unit.mode {
UnitMode::RunCustomBuild => 1.0,
UnitMode::Test => 0.5, // Or something
UnitMode::Doctest => 0.7, // Or something
_ => 0.0,
}).sum();
For tests (in
cargo test
s model), the overhead of the test itself is likely to dwarf XProtect. We don't have parallel test running yet (working on the foundation to allow it) which defers that problem.
Not sure I agree with this assessment? If you have a lot of integration tests, it's going to take a long time to run all those if XProtect wants to scan them all first. Sure, there's also build and link time of those, but that can be done in parallel.
A quick test with 100 integration tests revealed ~2 seconds build+link time and ~15 seconds test run time on my machine. With XProtect disabled, the test run time was down to ~0.4 seconds (even though they were run serially by Cargo).
For doctests, 2024 edition at least reduces the number of binaries. However, there is no caching of the binaries that are built (yet) so there are no binaries on every run which is a problem if there is any memory to XProtect.
I'm not sure I understand what you mean? If doctest's binaries aren't cached, then there's even more reason why XProtect is going to be problematic there?
Both of the above may not be relevant if the user is doing a
cargo check
or even acargo build
which is what this location is for.
True. Not that a plain cargo check
/cargo build
will produce UnitMode::Test
, but cargo test --no-run
and cargo build --tests
both do, and in those cases the heuristic is definitely wrong.
Maybe there's a way to know how many binaries we're actually going to run? Or maybe we should do the check later?
Regarding build scripts, I wouldn't expect those in a beginner project except through dependencies which can really blow up the numbers with deps like
reqwest
atm.
Agreed, and yes, we should also work on reducing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick test with 100 integration tests revealed ~2 seconds build+link time and ~15 seconds test run time on my machine. With XProtect disabled, the test run time was down to ~0.4 seconds (even though they were run serially by Cargo).
Uh, wow. I had assumed that the biggest problem was that it runs serially so the overrhead for an already serial operation would be minimal. This is strange.
I'm not sure I understand what you mean? If doctest's binaries aren't cached, then there's even more reason why XProtect is going to be problematic there?
Yes, thats what I'm calling out. On the surface level, Edition 2024 reduced the impact of this but the fact that they are rebuilt every run is a problem.
True. Not that a plain cargo check/cargo build will produce UnitMode::Test, but cargo test --no-run and cargo build --tests both do, and in those cases the heuristic is definitely wrong.
That builds them but still doesn't run them and so therefore isn't running into the issue. I feel weird providing the message speculatively like that which is why I was suggesting the test check be in cargo test
. Granted, I thought that wouldn't be a problem until #5609 but maybe it would be needed now.
if (10 < num_binaries && build_config.intent != UserIntent::Install) | ||
|| build_config.detect_antivirus == DetectAntivirus::Always | ||
{ | ||
if let Err(err) = detect_antivirus::detect_and_report(gctx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I wanted to note is that there is a performance cost to this. At least on my system, an error result took over 60ms, and a passing result took 14ms. We try to keep cargo's overhead relatively small (I like to keep the budget under 500ms total). 14ms is probably ok, but it's inching towards the territory where it is significant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, you're right.
On my machine (to have comparable numbers to those I've given elsewhere), I get:
Action | Time |
---|---|
dlopen -ing ExecutionPolicy.framework |
0.2-0.5ms |
Create EPDeveloperTool |
~0.3ms |
Get authorization status (fail) | 5-7ms |
Get authorization status (success) | 4-6ms |
Check SIP status (fail) | 2-4ms |
Request access (fail) | 8-22ms |
This is of course unfortunate, and is more reason to not do the check unless the heuristic triggers (whatever the heuristic ends up being).
If you trust your packages and their dependencies, then this \ | ||
overhead can be avoided by giving your session more permissions \ | ||
under `System Preferences > Security & Privacy > Developer Tools`. \ | ||
(Cargo has made an entry for your current session appear there \ | ||
already, though you will need to go and manually enable it).\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Messaging-wise, this seems much more widespread than just trusting cargo packages and dependencies, since it is authorizing the terminal/editor/tool that cargo is running under. That means, for example, you have to trust everything that happens in the terminal or editor.
One question that came up is whether or not it is possible to trust just cargo/rustup, or does it have to be the App that it is running under? It wasn't really clear to us what the rules were here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question that came up is whether or not it is possible to trust just cargo/rustup, or does it have to be the App that it is running under? It wasn't really clear to us what the rules were here.
If there is, I haven't found a way to do so. Adding just the cargo
/ rustup
binary as a Developer Tool does not work.
I recall someone suggesting something about making a launchd daemon (or agent). I think that would allow cargo
to XPC to cargod
and request it to build and run build scripts, and only have the daemon have the elevated privileges - but there's wayyy too many issues with that idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Messaging-wise, this seems much more widespread than just trusting cargo packages and dependencies, since it is authorizing the terminal/editor/tool that cargo is running under. That means, for example, you have to trust everything that happens in the terminal or editor.
I have tried to make the text a bit clearer in this regard, the current wording is:
note: detected that XProtect is enabled in this session, which may slow down builds as it scans build scripts and test binaries before they are run.
If you trust the software that you run in your terminal, then this overhead can be avoided by giving it more permissions under `System Preferences > Security & Privacy > Developer Tools`. (Cargo has made an entry for the current terminal appear there, though you will need to go and manually enable it).
Alternatively, you can disable this note by adding `build.detect-antivirus = false` to your ~/.cargo/config.toml.
See <https://doc.rust-lang.org/cargo/appendix/antivirus.html#xprotect> for more information.
See also appendix/antivirus.md
that I just added to the docs.
already, though you will need to go and manually enable it).\ | ||
\n\ | ||
Alternatively, you can disable this note by adding \ | ||
`build.detect-antivirus = false` to your ~/.cargo/config.toml.\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there may be some discussion about the exact configuration value, but I wanted to note that this currently isn't correct since false
isn't a valid value. Just want to make sure that all gets updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intended for:
-Zdetect-antivirus=auto
,-Zdetect-antivirus=always
and-Zdetect-antivirus=never
, because I thought that would be useful while testing.[build] detect-antivirus = false
or[build] detect-antivirus = true
in configuration file, I don't actually want users to be able to tweak the heuristic of when this fires.
What do you mean by false
not being a valid value?
27e6d86
to
56ffb5b
Compare
src/doc/src/appendix/antivirus.md
Outdated
# Antivirus Software | ||
|
||
Many operating systems include antivirus software programs that scan the system for malware. These sometimes interfere with development workflows, see below. | ||
|
||
## XProtect | ||
|
||
macOS has a layered system for protecting against malware, see [Apple's documentation](https://support.apple.com/en-gb/guide/security/sec469d47bd8/web) for details. One of the components here is XProtect, which intercepts all binary launches and scans new binaries for malware before executing them. | ||
|
||
Unfortunately, scanning binaries is done on a single thread and can be fairly slow (100-300ms). This affects projects developed with Cargo, since Cargo creates many binaries that are often only executed once (examples include build scripts and test binaries). | ||
|
||
You can avoid this overhead by doing the following: | ||
- Open `System Settings` and navigate to the `Privacy & Security` item. | ||
- On older macOS versions, click the `Privacy` tab. | ||
- Navicate to the `Developer Tools` item. | ||
- Add your terminal to the list, and enable it. | ||
- You can run `spctl developer-mode enable-terminal` to add `Terminal.app` to this list. | ||
- If you use a third-party terminal application, you might need to add that here as well. | ||
- Restart your terminal. | ||
|
||
See the screenshot below for what this looks like on macOS 26 Tahoe. | ||
|
||
 | ||
|
||
<!-- TODO: Talk about Cargo's `build.detect-antivirus = false` once stable --> | ||
|
||
### Security considerations | ||
|
||
Unfortunately, there doesn't seem to be a way to scope this to select binaries, e.g. adding Cargo directly as a developer tool has no effect, it has to be the "top-level" process. | ||
|
||
As such, **this disables your antivirus for all software that is launched via your terminal**. This is only one part of macOS' security protections that specifically checks for known malware signatures, and it is nowhere near as unsafe as disabling SIP or giving Full Disk Access would be - but depending on your security constraints, you might still consider leaving it enabled. | ||
|
||
Changing this setting has not been tested on systems in enterprise environments, it might affect more things there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this appendix to the documentation, so that the note can link to it. Needs iteration on the wording.
The screenshot was taken in a macOS 26 Tahoe RC VM, with exif data stripped using `exiftool` and size-optimized using `image_optim`.
To allow disabling the warning.
32bde1b
to
750b2e8
Compare
Having this enabled will make every initial binary launch slower. We check this by seeing if we have the "Developer Tool" execution policy grant, or if SIP Filesystem Protections are disabled.
This is very much a heuristic to avoid outputting the note to users where it might not be relevant.
Useful when testing the feature.
750b2e8
to
ca66bfd
Compare
if (10 < num_binaries && build_config.intent != UserIntent::Install) | ||
|| build_config.detect_antivirus == DetectAntivirus::Always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative, or additional thing, we could consider only doing the check once every X days, a bit similar to the automatic garbage collection.
This has the problem that it might take a long time for the user to notice the message, and worse, it makes Cargo's output non-deterministic, so I'm inclined not to do it.
Motivation
macOS has an antivirus system called XProtect, which does a signature-based check in applications and binaries upon first launch to see if they contain known malicious code. See Apple's documentation for details.
This takes a relatively long time though, see the following table for the approximate process run time of the simplest
fn main() {}
on my M2 Macbook (gauged withtime target/debug/foo
):First launch
Subsequent launch
First launch
Subsequent launch
This is amplified if the binary links other libraries or frameworks, and even more so by the fact that the
XprotectService
daemon runs in a single thread, so if you try to launch 10 new binaries at once, the slowdown will be more than a second.This PR tries to resolve this by querying the OS to try to detect whether XprotectService is likely to spend a lot of time verifying our binaries, and if we think it will, emit a warning to the user instructing them how they can disable it in their terminal.
This check and associated warning is gated behind the
-Zdetect-antivirus
flag, and it can be controlled by the configuration optionbuild.detect-antivirus
.Implementing a check such as this one really only makes sense in Cargo (in contrast to e.g. Rustup), since it may change if the user switches terminal (thus we cannot do this as a one-time check). We could also do this in the compiler, but it's not really relevant there, the compiler produces binaries, it doesn't care about how they are invoked.
See this Zulip thread for further discussion, and this blog post.
Possible concerns and considerations
objc2
ecosystem (objc2
,block2
andobjc2-encode
), which I maintain. They are fairly widely used, and Mozilla plans on vetting them soon-ish, though I acknowledge that this could still be seen as a supply-chain attack. Implementing the execution policy check without these crates is doable, though a lot more code.build.detect-antivirus
, an alternative would be to put it under[term]
. Or maybebuild.detect-slow-bin-first-run
, to focus on the intention instead of what it does?Testing
I guess there's two parts to this testing; testing that the code works as written, and testing that the detection logic itself is correct.
To test the code itself with SIP disabled, do:
System Preferences > Security & Privacy > Developer Tools
.cargo check -Zdetect-antivirus=always
on any project.System Preferences > Security & Privacy > Developer Tools
.cargo check -Zdetect-antivirus=always
on any project.Testing with SIP enabled:
System Preferences > Security & Privacy > Developer Tools
.cargo check -Zdetect-antivirus=always
on any project.System Preferences > Security & Privacy > Developer Tools
.If you want to change the SIP parts, use
csrutil disable
and/orcsrutil enable --without fs
. This is possible in a VM with UTM, btw, if you don't want to touch your main system.To check that the output of
cargo check -Zdetect-antivirus
is correct, I've been doing the following:Sample output when XProtect is enabled
Sample output when XProtect is disabled
Note that it seems like disabling Developer Tool permissions takes a little while to propagate to XProtect, so you might not see the slow timings immediately. A reboot solves this, closing your terminal and waiting a few minutes also does.
See also this repository I made for testing how this interacts with GitHub Actions (they disable SIP).