-
Notifications
You must be signed in to change notification settings - Fork 532
propagate stack protector to Linux C compilers #1550
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
Conversation
While Ubuntu's compilers have -fstack-protector-strong enabled by default, some other compilers don't. Propagate `-Zstack-protector` (soon to be `-Cstack-protector`) forward.
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.
Thanks, just a minor nit
// FIXME: Drop the -Z variant and update the doc link once the option is stabilized | ||
"-Zstack-protector" | "-Cstack-protector" => { | ||
self.stack_protector = | ||
Some(flag_ok_or(value, "-Zstack-protector must have a value")?); |
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.
Some(flag_ok_or(value, "-Zstack-protector must have a value")?); | |
Some(flag_ok_or(value, format!("{flag} must have a value"))?); |
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 will do a refactor myself alone with other similar nits
I propose stabilizing `-Cstack-protector` as `-Zstack-protector`. This PR adds a new `-Cstack-protector` flag, leaving the unstable `-Z` flag as is to ease the transition period. The `-Z` flag will be removed in the future. No RFC/MCP, this flag was added in rust-lang#84197 and was not deemed large enough to require additional process. The tracking issue for this feature is rust-lang#114903. The `-Cstack-protector=strong` mode uses the same underlying heuristics as Clang's `-fstack-protector-strong`. These heuristics weren't designed for Rust, and may be over-conservative in some cases - for example, if Rust stores a field's data in an alloca using an LLVM array type, LLVM regard the alloca as meaning that the function has a C array, and enable stack overflow canaries even if the function accesses the alloca in a safe way. Some people thought we should wait on stabilization until there are better heuristics, but I didn't hear about any concrete case where this unduly harms performance, and I think that when a need comes, we can improve the heuristics in LLVM after stabilization. The heuristics do seem to not be under-conservative, so this should not be a security risk. The `-Cstack-protector=basic` mode (`-fstack-protector`) uses heuristics that are specifically designed to catch old-C-style string manipulation. This is not a good fit to Rust, which does not perform much unsafe C-style string manipulation. As far as I can tell, nobody has been asking for it, and few people are using it even in today's C - modern distros (e.g. [Debian]) tend to use `-fstack-protector-strong`. Therefore, `-Cstack-protector=basic` has been **removed**. If anyone is interested in it, they are welcome to add it back as an unstable option. [Debian]: https://wiki.debian.org/Hardening#DEB_BUILD_HARDENING_STACKPROTECTOR_.28gcc.2Fg.2B-.2B-_-fstack-protector-strong.29 Most implementation was done in <rust-lang#84197>. The command-line attribute enables the relevant LLVM attribute on all functions in <https://github.com/rust-lang/rust/blob/68baa87ba6f03f8b6af2a368690161f1601e4040/compiler/rustc_codegen_llvm/src/attributes.rs#L267-L276>. Each target can indicate that it does not support stack canaries - currently, the GPU platforms `nvptx64-nvidia-cuda` and `amdgcn-amd-amdhsa`. On these platforms, use of `-Cstack-protector` causes an error. The feature has tests that make sure that the LLVM heuristic gives reasonable results for several functions, by checking for `__security_check_cookie` (on Windows) or `__stack_chk_fail` (on Linux). See <https://github.com/rust-lang/rust/tree/68baa87ba6f03f8b6af2a368690161f1601e4040/tests/assembly-llvm/stack-protector> No call-for-testing has been conducted, but the feature seems to be in use. No reported bugs seem to exist. - @bbjornse was the original implementor at rust-lang#84197 - @mrcnski documented it at rust-lang#111722 - @wesleywiser added tests for Windows at rust-lang#116037 - @davidtwco worked on the feature at rust-lang#121742 - @nikic provided support from the LLVM side (on Zulip on <https://rust-lang.zulipchat.com/#narrow/channel/233931-t-compiler.2Fmajor-changes/topic/Proposal.20for.20Adapt.20Stack.20Protector.20for.20Ru.E2.80.A6.20compiler-team.23841> and elsewhere), thanks @nikic! No FIXMEs related to this feature. This feature cannot cause undefined behavior. No changes to reference/spec, docs added to the codegen docs as part of the stabilization PR. No. None. No support needed for rustdoc, clippy, rust-analyzer, rustfmt or rustup. Cargo could expose this as an option in build profiles but I would expect the decision as to what version should be used would be made for the entire crate graph at build time rather than by individual package authors. `-C stack-protector` is propagated to C compilers using cc-rs via rust-lang/cc-rs#1550
While Ubuntu's compilers have -fstack-protector-strong enabled by default, some other compilers don't. Propagate
-Zstack-protector
(soon to be-Cstack-protector
) forward.