-
Notifications
You must be signed in to change notification settings - Fork 13.5k
rustc_const_eval: Expose APIs for signalling foreign accesses to memory #141391
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
Open
nia-e
wants to merge
2
commits into
rust-lang:master
Choose a base branch
from
nia-e:exposed-ffi
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -799,7 +799,7 @@ impl<Prov: Provenance, Extra, Bytes: AllocBytes> Allocation<Prov, Extra, Bytes> | |
/// Initialize all previously uninitialized bytes in the entire allocation, and set | ||
/// provenance of everything to `Wildcard`. Before calling this, make sure all | ||
/// provenance in this allocation is exposed! | ||
pub fn prepare_for_native_write(&mut self) -> AllocResult { | ||
pub fn prepare_for_native_write(&mut self, paranoid: bool) { | ||
let full_range = AllocRange { start: Size::ZERO, size: Size::from_bytes(self.len()) }; | ||
// Overwrite uninitialized bytes with 0, to ensure we don't leak whatever their value happens to be. | ||
for chunk in self.init_mask.range_as_init_chunks(full_range) { | ||
|
@@ -809,18 +809,25 @@ impl<Prov: Provenance, Extra, Bytes: AllocBytes> Allocation<Prov, Extra, Bytes> | |
uninit_bytes.fill(0); | ||
} | ||
} | ||
if paranoid { | ||
self.mark_foreign_write(full_range); | ||
} | ||
} | ||
|
||
/// Initialise previously uninitialised bytes in the given range, and set provenance of | ||
/// everything in it to `Wildcard`. Before calling this, make sure all provenance in this | ||
/// range is exposed! | ||
pub fn mark_foreign_write(&mut self, range: AllocRange) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to skip resetting unit memory to 0. That step must not be skipped! We shouldn't need two operations here anyway. Just one operation, |
||
// Mark everything as initialized now. | ||
self.mark_init(full_range, true); | ||
self.mark_init(range, true); | ||
|
||
// Set provenance of all bytes to wildcard. | ||
self.provenance.write_wildcards(self.len()); | ||
// Set provenance of affected bytes to wildcard. | ||
self.provenance.write_wildcards(range); | ||
|
||
// Also expose the provenance of the interpreter-level allocation, so it can | ||
// be written by FFI. The `black_box` is defensive programming as LLVM likes | ||
// to (incorrectly) optimize away ptr2int casts whose result is unused. | ||
std::hint::black_box(self.get_bytes_unchecked_raw_mut().expose_provenance()); | ||
|
||
Ok(()) | ||
} | ||
|
||
/// Remove all provenance in the given memory range. | ||
|
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
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
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So if
paranoid
isfalse
, then this function does what exactly? It seems to do just absolutely nothing.^^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 zeroes out the bytes of uninitialised memory without actually marking it as init. I initially didn't do this, but it resulted in
mark_foreign_write
overwriting the data we cared about with zeroes. That's also why the latter doesn't zero out anything. We first zero the memory, then call the foreign code, then without re-zeroing mark it as init if it was written to after being zeroedThere 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.
Where does it do that zeroing?
prepare_for_native_write
only gets called ifparanoid
is true. So what you say and what the code does do not seem to line up, or I am misunderstanding something.But also, I think we shouldn't have such a 2-stage approach. This seems easier to reason about if we just fully delay everything until the memory gets accessed the first time.
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 I explained myself poorly, sorry. Calling
get_alloc_raw()
has a side effect, namely (by callingget_global_alloc()
which callsadjust_global_allocation()
which callsinit_allocation()
) it, well, actually initialises that allocation. The point is that if we skip this, callingget_global_alloc()
post-FFI might "initialise" allocations that were actually written to by the foreign code that Miri doesn't know about. I confirmed this with testing; if we allocate a pointer and pass it across the FFI boundary but skip callingprepare_for_native_call(false)
, the data written will be replaced with zeroes as soon asget_global_alloc()
is called after the FFI call has completed. So, as far as I can tellinit_allocation()
is responsible for said zeroing the first time it's called for a specific allocationThere 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.
There's no zeroing anywhere outside
prepare_for_native_write
so I don't know what you are talking about.adjust_global_allocation
will set up the memory of the static with whatever the initial value of the static is. Is that what you mean? That's not "zeroing" though unless the initial value of the static happens to be zero.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.
It's a good point that we need to actually initialize the globals at some point. Why can't we do this lazily on first access, like the other FFI adjustments are done with your approach?
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.
Ah, that would explain why only tests that had to do with statics had trouble here. Every time I ran into that problem the memory was all zeroes, so I incorrectly assumed that's what it was doing I guess. I should probably update this then, apologies for my confusion
I'm not sure how to do that I guess, since if the op was a read instead of a write we need it to already have been initialised or else the foreign code will read uninitialised data, no? We only know an access happened 1 instruction after it did happen so I think we still have to be cautious here and initialise the globals. I might be missing something though