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.

Sign up for GitHub

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

Jump to bottom

Revert "CFI: Skip non-passed arguments" #123205

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Revert "CFI: Skip non-passed arguments" #123205

wants to merge 1 commit into from
+39 −47

Conversation

rcvalle
Copy link
Member

@rcvalle rcvalle commented Mar 29, 2024 •
edited
Loading

Since we're now using an approach which is a variant of #123082 (that transforms closures into dynamic Fn traits but isolating it to the Fn call methods only) instead of #121962 or #122573, skipping non-passed arguments shouldn't be necessary for KCFI anymore and we can claim back the reduced granularity.

This should be tested with and merged after the remaining KCFI-related patches are merged.

This reverts commit f2f0d25 (#122456).

This was split off from #116404.

cc @compiler-errors @workingjubilee @maurer

@rustbot
Copy link
Collaborator

rustbot commented Mar 29, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 29, 2024
@rcvalle
Copy link
Member Author

rcvalle commented Mar 29, 2024

r? @compiler-errors

@rcvalle
Revert "CFI: Skip non-passed arguments"
8067604 
Since we're now using an approach which is a variant of rust-lang#123082 (that
transforms closures into dynamic Fn traits but isolating it to the Fn
call methods only) instead of rust-lang#121962 or rust-lang#122573, skipping non-passed
arguments shouldn't be necessary for KCFI anymore and we can claim back
the reduced granularity.

This reverts commit f2f0d25.
@rcvalle rcvalle force-pushed the rust-cfi-revert-skip-non-passed-arguments branch from 9ce1899 to 8067604 Compare March 29, 2024 19:43
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
#16 exporting to docker image format
#16 sending tarball 30.5s done
#16 DONE 34.6s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
failures:

---- [ui] tests/ui/sanitizer/cfi-closure-fn-ptr-cast.rs#cfi stdout ----

error in revision `cfi`: test run failed!
status: signal: 4 (SIGILL) (core dumped)
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitizer/cfi-closure-fn-ptr-cast.cfi" && RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitizer/cfi-closure-fn-ptr-cast.cfi/a"
stderr: none

@workingjubilee workingjubilee added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2024
@workingjubilee
Copy link
Member

We can choose to ship this eventually, since as we've discussed, while I find eliding () to be a fine implementation, it's not quite as "Rust-incorrect" per se as other problems we are managing with CFI... but it does need to actually build and run code, yeah, so it's gonna wait until enough things land that it can work first. ^^;

@rcvalle
Copy link
Member Author

rcvalle commented Mar 29, 2024 •
edited
Loading

It doesn't just elide (), but also function items, and more (e.g., the extern function and the constructor that had to be wrapped in a reference in the tests so they aren't gone). For example, this made functions that had only one parameter that was a function item and otherwise were each one placed in its own group to be grouped together with all functions that were just fn func(), which was what happened with these examples in the tests. And this is just an example, any function that was disambiguated by a ZST had a similar grouping (or ungrouping).

But yes, we should wait the remaining patches to be merged and test this with them before merging. If we still find an issue, we should look into it and fix it, instead of work around it with this.

@bors
Copy link
Contributor

bors commented Apr 4, 2024

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

@rcvalle rcvalle mentioned this pull request Aug 13, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@compiler-errors compiler-errors

Labels
PG-exploit-mitigations Project group: Exploit mitigations S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rcvalle @rustbot @rust-log-analyzer @workingjubilee @bors @wesleywiser @compiler-errors