Add relro-level tests by kyrias · Pull Request #48388 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation33 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The relro-level debugging flag was added in #43170 which was merged in July 2017. This PR moves this flag to be a proper codegen flag.
(rust_highfive has picked a reviewer for you, use r? to override)
This is basically a stabilization. Is there a tracking issue for this feature?
Relevant to the compiler team, which will review and decide on the PR/issue.
label
I know nothing about this, so it's hard for me to judge if we are happy enough with its implementation to be ready to stabilize. =)
cc @rust-lang/compiler
No tracking issue. This is essentially just a flag to be able to disable RELRO, or use only partial RELRO. Full RELRO has been on by default as of #43170 being merged, but @alexcrichton wanted the flag to disable it to stay unstable for a while at the time.
Ok. Usually for a stabilization I like to see a little report that shows the test cases that show that this feature is indeed working as expected -- based on the the diffs in this PR, I take it that there are no tests at all for this (i.e., nothing had to be changed from -Z to -C). Do you think you'd be up for adding one?
Not really sure how to test this from rust honestly. To actually check that it is functioning properly we'd essentially need to actually do a compile and then parse the ELF binary and check for the GNU_RELRO header and BIND_NOW sections. Not sure if there's any nice way to do that though?
@kyrias usually we would make a run-make test for that sort of thing
Maybe it's not worth the trouble, I don't know, but I am a big believer in "if it ain't tested, it's broken".
Interesting, didn't know about run-make tests, I'll give it a shot. (Is there any good docs for getting more details about the different parts of the compiler tests, and how to write the different types of tests.)
Is there any specific list of dependencies that are guaranteed to exist when they're run?
So I figured that when you specify relro-level=off you probably want to tell the linker to explicitly not use RELRO rather than just have the compiler not pass on the flag at all, so I added that as well, and then a test for this case.
Since this is a very small change I just added it in this PR, but if you would prefer I can split it out and file a PR for it separately.
☔ The latest upstream changes (presumably #48531) made this pull request unmergeable. Please resolve the merge conflicts.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Thanks @kyrias! For stabilizatin purposes could you also detail a bit what your plans are with the stable flag?
I'm realizing now that we don't have a ton of specific paltform-specific options stabilized in the sense that -C relro doesn't mean much on Windows, but that's not necessarily a problem.
Hey, sorry for the delay, been away for a week. Now that I think about it I don't really have any specific plans for it, I just thought that it can be useful to be able to disable it in some rare circumstances. Since the executables end up being slightly bigger, disabling RELRO can be useful for extremely low memory targets. (And BIND_NOW additionally causes some performance impact as it has to resolve all symbols at load time, even if there is a large number of symbols that end up not being used.) But other than that I guess there aren't anything in specific I can think off.
Ah ok thanks for the info @kyrias! In that case I wonder if it's best left unstable for now? That way it's there to tinker with if necessary but otherwise we may want to hold out as it may inform how we stabilize this.
Yeah, might make sense until the problems become more real. I think I'm mostly just bothered by the fact that -Z is documented as debug options everywhere, which bothers me since it's not actually a debug option, heh.
Anyway, I could drop the move of the option and just keep the tests and the relro-level=off fix.
Move relro-level flag from debugging to codegen Add relro-level tests
Previously relro-level=off would just not tell the linker to use RELRO, but when you want to disable RELRO you most likely want to entirely prevent.
Signed-off-by: Johannes Löthberg johannes@kyriasis.com
Signed-off-by: Johannes Löthberg johannes@kyriasis.com
Signed-off-by: Johannes Löthberg johannes@kyriasis.com
📌 Commit 1dbce4b has been approved by alexcrichton
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
⌛ Testing commit 1dbce4b with merge 7f6687d357f136197ea8285137bfddb7cd99c231...
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
⌛ Testing commit 1dbce4b with merge 211c155e8eb32ad9e147edf35471e6a2c73c30b5...
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
@bors retry
Some OpenSSL installation error, should be already fixed by #48901.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit that referenced this pull request
Add relro-level tests
The relro-level debugging flag was added in #43170 which was merged in July 2017. This PR moves this flag to be a proper codegen flag.
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.