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.

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

Randomize the ordering of struct fields when compiling in debug mode #38550

Open
bstrie opened this issue Dec 22, 2016 · 10 comments
Open

Randomize the ordering of struct fields when compiling in debug mode #38550

bstrie opened this issue Dec 22, 2016 · 10 comments
Labels
A-reproducibility Area: Reproducible / deterministic builds C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bstrie
Copy link
Contributor

bstrie commented Dec 22, 2016

Now that it looks like #28951 will be addressed via #37429 , we may have a further obligation to ensure to do what we can to ensure that this doesn't bite people. One way that we could help out would be to randomize the order of struct fields by default when compiling in debug mode. This would only apply to structs that are not tagged with a #repr, and would effectively act as the #[repr(random)] proposed by @petrochenkov at #37429 (comment) (actually adding explicit #[repr(random)] would probably require an RFC, and so is not a prereq for this issue).

@abonander
Copy link
Contributor

I know there's code out there in the wild that will be broken by this, especially if it affects the stdlib. Perhaps a Crater run and an early warning for the affected crates is in order.

@bstrie
Copy link
Contributor Author

bstrie commented Dec 23, 2016

@abonander We ran Crater for #37249 and saw no instances of legitimate breakage, though of course what I'm proposing here is more radical and also Crater only catches a subset of errors. This is definitely a conversation that we need to have with the broader community so that people are aware of what Rust actually guarantees (i.e. "less than you think if you're not using repr(C)"), which is part of the reason why I've proposed this. It's true that maybe we could just have this as a dev option for the moment solely for the purpose of teasing out how much would break on Crater.

@bstrie
Copy link
Contributor Author

bstrie commented Dec 23, 2016

Unanswered question: if a user does reveal a bug thanks to Rust reordering struct fields randomly (which, in correct code, should never trigger bugs), how do we help the user reproduce that bug deterministically?

@abonander
Copy link
Contributor

Unanswered question: if a user does reveal a bug thanks to Rust reordering struct fields randomly (which, in correct code, should never trigger bugs), how do we help the user reproduce that bug deterministically?

Probably some compiler flag that prints the seed for randomization for each struct, and/or prints the final ordering of the fields for a given struct given the seed.

@ahicks92
Copy link
Contributor

This will probably end up being a flag that does it both in debug and release. Se this thread.

I wish the three related discussions were centralized.

@ahicks92
Copy link
Contributor

Just thought of something else. The seed for the reordering can be a hash of the path to the struct, thus producing a stable ordering until such time as the struct is moved or renamed. This should be really easy to implement.

@steveklabnik steveklabnik added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed A-compiler labels Mar 24, 2017
@diwic
Copy link
Contributor

diwic commented Apr 21, 2017

@camlorn Read your article and came to think of something. For this one:

struct RcBox<T: ?Sized> {
    strong: Cell<usize>,
    refcount: Cell<usize>,
    value: T,
}

Could this optimisation change the order of strong, refcount and value so that it is different for RcBox<MyStruct> and RcBox<MyTrait>, so that a currently safe operation rc_mystruct as Rc<MyTrait> becomes a very bad thing?

@ahicks92
Copy link
Contributor

Nope. I didn't go into it because the article was already pretty lengthy, but @eddyb provided magic code that I am not going to even pretend to understand which can tell us whether or not we can move the last field.

@eddyb
Copy link
Member

eddyb commented Apr 24, 2017

It's not just the last field but you can't reorder the prefix differenly between those types, the cast is safe.

@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 26, 2017
@jonas-schievink jonas-schievink added the A-reproducibility Area: Reproducible / deterministic builds label Aug 19, 2019
@eddyb
Copy link
Member

eddyb commented Sep 23, 2020

We could at least randomize the order of the fields that share an alignment, using the "def path hash" of the ty::Adt.
That way we could release this into the wild and see what breaks.

Similarly, I just realized we could do something similar for function calls, though it's harder to know what to key on, there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-reproducibility Area: Reproducible / deterministic builds C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants