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

add an option to track raw pointer tags in Stacked Borrows #1603

Merged
merged 5 commits into from
Oct 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -232,13 +232,20 @@ environment variable:
* `-Zmiri-track-alloc-id=<id>` shows a backtrace when the given allocation is
being allocated or freed. This helps in debugging memory leaks and
use after free bugs.
* `-Zmiri-track-call-id=<id>` shows a backtrace when the given call id is
assigned to a stack frame. This helps in debugging UB related to Stacked
Borrows "protectors".
* `-Zmiri-track-pointer-tag=<tag>` shows a backtrace when the given pointer tag
is popped from a borrow stack (which is where the tag becomes invalid and any
future use of it will error). This helps you in finding out why UB is
happening and where in your code would be a good place to look for it.
* `-Zmiri-track-call-id=<id>` shows a backtrace when the given call id is
assigned to a stack frame. This helps in debugging UB related to Stacked
Borrows "protectors".
* `-Zmiri-track-raw-pointers` makes Stacked Borrows track a pointer tag even for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you construct a test for both the false positive case as well as an example where we now detect latent aliasing where we didn't before? (I'm guessing under certain circumstances they could be one and the same test? we'd just detect the aliasing earlier now)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example with a false positive caused by this flag is run-pass/box.rs, since it casts a ptr to an int and back.

I can add a test for a bug that would otherwise be missed (rust-lang/rust#78477 is the example that motivated this change).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added such a test.

raw pointers. This can make valid code fail to pass the checks, but also can
help identify latent aliasing issues in code that Miri accepts by default. You
can recognize false positives by "<untagged>" occurring in the message -- this
indicates a pointer that was cast from an integer, so Miri was unable to track
this pointer. Make sure to use a non-Windows target with this flag, as the
Windows runtime makes use of integer-pointer casts.

Some native rustc `-Z` flags are also very relevant for Miri:

Expand Down
3 changes: 3 additions & 0 deletions src/bin/miri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,9 @@ fn main() {
"-Zmiri-ignore-leaks" => {
miri_config.ignore_leaks = true;
}
"-Zmiri-track-raw-pointers" => {
miri_config.track_raw = true;
}
"--" => {
after_dashdash = true;
}
Expand Down
14 changes: 4 additions & 10 deletions src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
use std::convert::TryFrom;
use std::ffi::OsStr;

use rand::rngs::StdRng;
use rand::SeedableRng;
use log::info;

use rustc_hir::def_id::DefId;
Expand Down Expand Up @@ -48,6 +46,8 @@ pub struct MiriConfig {
pub tracked_call_id: Option<CallId>,
/// The allocation id to report about.
pub tracked_alloc_id: Option<AllocId>,
/// Whether to track raw pointers in stacked borrows.
pub track_raw: bool,
}

impl Default for MiriConfig {
Expand All @@ -64,6 +64,7 @@ impl Default for MiriConfig {
tracked_pointer_tag: None,
tracked_call_id: None,
tracked_alloc_id: None,
track_raw: false,
}
}
}
Expand All @@ -84,14 +85,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
rustc_span::source_map::DUMMY_SP,
param_env,
Evaluator::new(config.communicate, config.validate, layout_cx),
MemoryExtra::new(
StdRng::seed_from_u64(config.seed.unwrap_or(0)),
config.stacked_borrows,
config.tracked_pointer_tag,
config.tracked_call_id,
config.tracked_alloc_id,
config.check_alignment,
),
MemoryExtra::new(&config),
);
// Complete initialization.
EnvVars::init(&mut ecx, config.excluded_env_vars)?;
Expand Down
23 changes: 11 additions & 12 deletions src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use std::fmt;

use log::trace;
use rand::rngs::StdRng;
use rand::SeedableRng;

use rustc_data_structures::fx::FxHashMap;
use rustc_middle::{
Expand Down Expand Up @@ -132,16 +133,14 @@ pub struct MemoryExtra {
}

impl MemoryExtra {
pub fn new(
rng: StdRng,
stacked_borrows: bool,
tracked_pointer_tag: Option<PtrId>,
tracked_call_id: Option<CallId>,
tracked_alloc_id: Option<AllocId>,
check_alignment: AlignmentCheck,
) -> Self {
let stacked_borrows = if stacked_borrows {
Some(Rc::new(RefCell::new(stacked_borrows::GlobalState::new(tracked_pointer_tag, tracked_call_id))))
pub fn new(config: &MiriConfig) -> Self {
let rng = StdRng::seed_from_u64(config.seed.unwrap_or(0));
let stacked_borrows = if config.stacked_borrows {
Some(Rc::new(RefCell::new(stacked_borrows::GlobalState::new(
config.tracked_pointer_tag,
config.tracked_call_id,
config.track_raw,
))))
} else {
None
};
Expand All @@ -150,8 +149,8 @@ impl MemoryExtra {
intptrcast: Default::default(),
extern_statics: FxHashMap::default(),
rng: RefCell::new(rng),
tracked_alloc_id,
check_alignment,
tracked_alloc_id: config.tracked_alloc_id,
check_alignment: config.check_alignment,
}
}

Expand Down
30 changes: 17 additions & 13 deletions src/range_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ impl<T> RangeMap<T> {
/// Provides read-only iteration over everything in the given range. This does
/// *not* split items if they overlap with the edges. Do not use this to mutate
/// through interior mutability.
pub fn iter<'a>(&'a self, offset: Size, len: Size) -> impl Iterator<Item = &'a T> + 'a {
///
/// The iterator also provides the offset of the given element.
pub fn iter<'a>(&'a self, offset: Size, len: Size) -> impl Iterator<Item = (Size, &'a T)> + 'a {
let offset = offset.bytes();
let len = len.bytes();
// Compute a slice starting with the elements we care about.
Expand All @@ -75,7 +77,7 @@ impl<T> RangeMap<T> {
};
// The first offset that is not included any more.
let end = offset + len;
slice.iter().take_while(move |elem| elem.range.start < end).map(|elem| &elem.data)
slice.iter().take_while(move |elem| elem.range.start < end).map(|elem| (Size::from_bytes(elem.range.start), &elem.data))
}

pub fn iter_mut_all<'a>(&'a mut self) -> impl Iterator<Item = &'a mut T> + 'a {
Expand Down Expand Up @@ -112,11 +114,13 @@ impl<T> RangeMap<T> {
/// this will split entries in the map that are only partially hit by the given range,
/// to make sure that when they are mutated, the effect is constrained to the given range.
/// Moreover, this will opportunistically merge neighbouring equal blocks.
///
/// The iterator also provides the offset of the given element.
pub fn iter_mut<'a>(
&'a mut self,
offset: Size,
len: Size,
) -> impl Iterator<Item = &'a mut T> + 'a
) -> impl Iterator<Item = (Size, &'a mut T)> + 'a
where
T: Clone + PartialEq,
{
Expand Down Expand Up @@ -197,7 +201,7 @@ impl<T> RangeMap<T> {
// Now we yield the slice. `end` is inclusive.
&mut self.v[first_idx..=end_idx]
};
slice.iter_mut().map(|elem| &mut elem.data)
slice.iter_mut().map(|elem| (Size::from_bytes(elem.range.start), &mut elem.data))
}
}

Expand All @@ -209,26 +213,26 @@ mod tests {
fn to_vec<T: Copy>(map: &RangeMap<T>, offset: u64, len: u64) -> Vec<T> {
(offset..offset + len)
.into_iter()
.map(|i| map.iter(Size::from_bytes(i), Size::from_bytes(1)).next().map(|&t| t).unwrap())
.map(|i| map.iter(Size::from_bytes(i), Size::from_bytes(1)).next().map(|(_, &t)| t).unwrap())
.collect()
}

#[test]
fn basic_insert() {
let mut map = RangeMap::<i32>::new(Size::from_bytes(20), -1);
// Insert.
for x in map.iter_mut(Size::from_bytes(10), Size::from_bytes(1)) {
for (_, x) in map.iter_mut(Size::from_bytes(10), Size::from_bytes(1)) {
*x = 42;
}
// Check.
assert_eq!(to_vec(&map, 10, 1), vec![42]);
assert_eq!(map.v.len(), 3);

// Insert with size 0.
for x in map.iter_mut(Size::from_bytes(10), Size::from_bytes(0)) {
for (_, x) in map.iter_mut(Size::from_bytes(10), Size::from_bytes(0)) {
*x = 19;
}
for x in map.iter_mut(Size::from_bytes(11), Size::from_bytes(0)) {
for (_, x) in map.iter_mut(Size::from_bytes(11), Size::from_bytes(0)) {
*x = 19;
}
assert_eq!(to_vec(&map, 10, 2), vec![42, -1]);
Expand All @@ -238,16 +242,16 @@ mod tests {
#[test]
fn gaps() {
let mut map = RangeMap::<i32>::new(Size::from_bytes(20), -1);
for x in map.iter_mut(Size::from_bytes(11), Size::from_bytes(1)) {
for (_, x) in map.iter_mut(Size::from_bytes(11), Size::from_bytes(1)) {
*x = 42;
}
for x in map.iter_mut(Size::from_bytes(15), Size::from_bytes(1)) {
for (_, x) in map.iter_mut(Size::from_bytes(15), Size::from_bytes(1)) {
*x = 43;
}
assert_eq!(map.v.len(), 5);
assert_eq!(to_vec(&map, 10, 10), vec![-1, 42, -1, -1, -1, 43, -1, -1, -1, -1]);

for x in map.iter_mut(Size::from_bytes(10), Size::from_bytes(10)) {
for (_, x) in map.iter_mut(Size::from_bytes(10), Size::from_bytes(10)) {
if *x < 42 {
*x = 23;
}
Expand All @@ -256,14 +260,14 @@ mod tests {
assert_eq!(to_vec(&map, 10, 10), vec![23, 42, 23, 23, 23, 43, 23, 23, 23, 23]);
assert_eq!(to_vec(&map, 13, 5), vec![23, 23, 43, 23, 23]);

for x in map.iter_mut(Size::from_bytes(15), Size::from_bytes(5)) {
for (_, x) in map.iter_mut(Size::from_bytes(15), Size::from_bytes(5)) {
*x = 19;
}
assert_eq!(map.v.len(), 6);
assert_eq!(to_vec(&map, 10, 10), vec![23, 42, 23, 23, 23, 19, 19, 19, 19, 19]);
// Should be seeing two blocks with 19.
assert_eq!(
map.iter(Size::from_bytes(15), Size::from_bytes(2)).map(|&t| t).collect::<Vec<_>>(),
map.iter(Size::from_bytes(15), Size::from_bytes(2)).map(|(_, &t)| t).collect::<Vec<_>>(),
vec![19, 19]
);

Expand Down
Loading