Skip to content

Commit ff3e990

Browse files
borsbors
committed
Auto merge of rust-lang#3106 - RalfJung:tree-borrows-initial, r=RalfJung
Tree Borrows: do not create new tags as 'Active' Cc `@Vanille-N`
2 parents 6ed502d + 5178ae6 commit ff3e990

File tree

9 files changed

+106
-61
lines changed

9 files changed

+106
-61
lines changed

‎src/tools/miri/src/borrow_tracker/mod.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
305305
}
306306
}
307307

308-
fn protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
308+
fn protect_place(
309+
&mut self,
310+
place: &MPlaceTy<'tcx, Provenance>,
311+
) -> InterpResult<'tcx, MPlaceTy<'tcx, Provenance>> {
309312
let this = self.eval_context_mut();
310313
let method = this.machine.borrow_tracker.as_ref().unwrap().borrow().borrow_tracker_method;
311314
match method {

‎src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs

Lines changed: 28 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -810,36 +810,43 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
810810
Ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }))
811811
}
812812

813-
/// Retags an individual pointer, returning the retagged version.
814-
/// `kind` indicates what kind of reference is being created.
815-
fn sb_retag_reference(
813+
fn sb_retag_place(
816814
&mut self,
817-
val: &ImmTy<'tcx, Provenance>,
815+
place: &MPlaceTy<'tcx, Provenance>,
818816
new_perm: NewPermission,
819817
info: RetagInfo, // diagnostics info about this retag
820-
) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
818+
) -> InterpResult<'tcx, MPlaceTy<'tcx, Provenance>> {
821819
let this = self.eval_context_mut();
822-
// We want a place for where the ptr *points to*, so we get one.
823-
let place = this.ref_to_mplace(val)?;
824-
let size = this.size_and_align_of_mplace(&place)?.map(|(size, _)| size);
820+
let size = this.size_and_align_of_mplace(place)?.map(|(size, _)| size);
825821
// FIXME: If we cannot determine the size (because the unsized tail is an `extern type`),
826822
// bail out -- we cannot reasonably figure out which memory range to reborrow.
827823
// See https://github.com/rust-lang/unsafe-code-guidelines/issues/276.
828824
let size = match size {
829825
Some(size) => size,
830-
None => return Ok(val.clone()),
826+
None => return Ok(place.clone()),
831827
};
832828

833829
// Compute new borrow.
834830
let new_tag = this.machine.borrow_tracker.as_mut().unwrap().get_mut().new_ptr();
835831

836832
// Reborrow.
837-
let new_prov = this.sb_reborrow(&place, size, new_perm, new_tag, info)?;
833+
let new_prov = this.sb_reborrow(place, size, new_perm, new_tag, info)?;
838834

839-
// Adjust pointer.
840-
let new_place = place.map_provenance(|_| new_prov);
835+
// Adjust place.
836+
Ok(place.clone().map_provenance(|_| new_prov))
837+
}
841838

842-
// Return new pointer.
839+
/// Retags an individual pointer, returning the retagged version.
840+
/// `kind` indicates what kind of reference is being created.
841+
fn sb_retag_reference(
842+
&mut self,
843+
val: &ImmTy<'tcx, Provenance>,
844+
new_perm: NewPermission,
845+
info: RetagInfo, // diagnostics info about this retag
846+
) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
847+
let this = self.eval_context_mut();
848+
let place = this.ref_to_mplace(val)?;
849+
let new_place = this.sb_retag_place(&place, new_perm, info)?;
843850
Ok(ImmTy::from_immediate(new_place.to_ref(this), val.layout))
844851
}
845852
}
@@ -972,26 +979,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
972979
/// call.
973980
///
974981
/// This is used to ensure soundness of in-place function argument/return passing.
975-
fn sb_protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
982+
fn sb_protect_place(
983+
&mut self,
984+
place: &MPlaceTy<'tcx, Provenance>,
985+
) -> InterpResult<'tcx, MPlaceTy<'tcx, Provenance>> {
976986
let this = self.eval_context_mut();
977987

978-
// We have to turn the place into a pointer to use the usual retagging logic.
979-
// (The pointer type does not matter, so we use a raw pointer.)
980-
let ptr = this.mplace_to_ref(place)?;
981-
// Reborrow it. With protection! That is the entire point.
988+
// Retag it. With protection! That is the entire point.
982989
let new_perm = NewPermission::Uniform {
983990
perm: Permission::Unique,
984991
access: Some(AccessKind::Write),
985992
protector: Some(ProtectorKind::StrongProtector),
986993
};
987-
let _new_ptr = this.sb_retag_reference(
988-
&ptr,
994+
this.sb_retag_place(
995+
place,
989996
new_perm,
990997
RetagInfo { cause: RetagCause::InPlaceFnPassing, in_field: false },
991-
)?;
992-
// We just throw away `new_ptr`, so nobody can access this memory while it is protected.
993-
994-
Ok(())
998+
)
995999
}
9961000

9971001
/// Mark the given tag as exposed. It was found on a pointer with the given AllocId.

‎src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@ use rustc_target::abi::{Abi, Align, Size};
55
use crate::borrow_tracker::{AccessKind, GlobalStateInner, ProtectorKind, RetagFields};
66
use rustc_middle::{
77
mir::{Mutability, RetagKind},
8-
ty::{self, layout::HasParamEnv, Ty},
8+
ty::{
9+
self,
10+
layout::{HasParamEnv, HasTyCtxt},
11+
Ty,
12+
},
913
};
1014
use rustc_span::def_id::DefId;
1115

@@ -174,6 +178,8 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
174178
new_tag: BorTag,
175179
) -> InterpResult<'tcx, Option<Provenance>> {
176180
let this = self.eval_context_mut();
181+
// Make sure the new permission makes sense as the initial permission of a fresh tag.
182+
assert!(new_perm.initial_state.is_initial());
177183
// Ensure we bail out if the pointer goes out-of-bounds (see miri#1050).
178184
this.check_ptr_access_align(
179185
place.ptr(),
@@ -275,10 +281,6 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
275281
diagnostics::AccessCause::Reborrow,
276282
)?;
277283
// Record the parent-child pair in the tree.
278-
// FIXME: We should eventually ensure that the following `assert` holds, because
279-
// some "exhaustive" tests consider only the initial configurations that satisfy it.
280-
// The culprit is `Permission::new_active` in `tb_protect_place`.
281-
//assert!(new_perm.initial_state.is_initial());
282284
tree_borrows.new_child(orig_tag, new_tag, new_perm.initial_state, range, span)?;
283285
drop(tree_borrows);
284286

@@ -306,15 +308,12 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
306308
Ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }))
307309
}
308310

309-
/// Retags an individual pointer, returning the retagged version.
310-
fn tb_retag_reference(
311+
fn tb_retag_place(
311312
&mut self,
312-
val: &ImmTy<'tcx, Provenance>,
313+
place: &MPlaceTy<'tcx, Provenance>,
313314
new_perm: NewPermission,
314-
) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
315+
) -> InterpResult<'tcx, MPlaceTy<'tcx, Provenance>> {
315316
let this = self.eval_context_mut();
316-
// We want a place for where the ptr *points to*, so we get one.
317-
let place = this.ref_to_mplace(val)?;
318317

319318
// Determine the size of the reborrow.
320319
// For most types this is the entire size of the place, however
@@ -323,7 +322,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
323322
// then we override the size to do a zero-length reborrow.
324323
let reborrow_size = match new_perm {
325324
NewPermission { zero_size: false, .. } =>
326-
this.size_and_align_of_mplace(&place)?
325+
this.size_and_align_of_mplace(place)?
327326
.map(|(size, _)| size)
328327
.unwrap_or(place.layout.size),
329328
_ => Size::from_bytes(0),
@@ -339,12 +338,21 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
339338
let new_tag = this.machine.borrow_tracker.as_mut().unwrap().get_mut().new_ptr();
340339

341340
// Compute the actual reborrow.
342-
let new_prov = this.tb_reborrow(&place, reborrow_size, new_perm, new_tag)?;
341+
let new_prov = this.tb_reborrow(place, reborrow_size, new_perm, new_tag)?;
343342

344-
// Adjust pointer.
345-
let new_place = place.map_provenance(|_| new_prov);
343+
// Adjust place.
344+
Ok(place.clone().map_provenance(|_| new_prov))
345+
}
346346

347-
// Return new pointer.
347+
/// Retags an individual pointer, returning the retagged version.
348+
fn tb_retag_reference(
349+
&mut self,
350+
val: &ImmTy<'tcx, Provenance>,
351+
new_perm: NewPermission,
352+
) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
353+
let this = self.eval_context_mut();
354+
let place = this.ref_to_mplace(val)?;
355+
let new_place = this.tb_retag_place(&place, new_perm)?;
348356
Ok(ImmTy::from_immediate(new_place.to_ref(this), val.layout))
349357
}
350358
}
@@ -493,22 +501,21 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
493501
/// call.
494502
///
495503
/// This is used to ensure soundness of in-place function argument/return passing.
496-
fn tb_protect_place(&mut self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
504+
fn tb_protect_place(
505+
&mut self,
506+
place: &MPlaceTy<'tcx, Provenance>,
507+
) -> InterpResult<'tcx, MPlaceTy<'tcx, Provenance>> {
497508
let this = self.eval_context_mut();
498509

499-
// We have to turn the place into a pointer to use the usual retagging logic.
500-
// (The pointer type does not matter, so we use a raw pointer.)
501-
let ptr = this.mplace_to_ref(place)?;
502-
// Reborrow it. With protection! That is the entire point.
510+
// Retag it. With protection! That is the entire point.
503511
let new_perm = NewPermission {
504-
initial_state: Permission::new_active(),
512+
initial_state: Permission::new_reserved(
513+
place.layout.ty.is_freeze(this.tcx(), this.param_env()),
514+
),
505515
zero_size: false,
506516
protector: Some(ProtectorKind::StrongProtector),
507517
};
508-
let _new_ptr = this.tb_retag_reference(&ptr, new_perm)?;
509-
// We just throw away `new_ptr`, so nobody can access this memory while it is protected.
510-
511-
Ok(())
518+
this.tb_retag_place(place, new_perm)
512519
}
513520

514521
/// Mark the given tag as exposed. It was found on a pointer with the given AllocId.

‎src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ impl Permission {
159159
}
160160

161161
/// Default initial permission of the root of a new tree.
162+
/// Must *only* be used for the root, this is not in general an "initial" permission!
162163
pub fn new_active() -> Self {
163164
Self { inner: Active }
164165
}

‎src/tools/miri/src/machine.rs

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,19 +1275,25 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
12751275
ecx: &mut InterpCx<'mir, 'tcx, Self>,
12761276
place: &PlaceTy<'tcx, Provenance>,
12771277
) -> InterpResult<'tcx> {
1278-
// We do need to write `uninit` so that even after the call ends, the former contents of
1279-
// this place cannot be observed any more.
1280-
ecx.write_uninit(place)?;
12811278
// If we have a borrow tracker, we also have it set up protection so that all reads *and
12821279
// writes* during this call are insta-UB.
1283-
if ecx.machine.borrow_tracker.is_some() {
1280+
let protected_place = if ecx.machine.borrow_tracker.is_some() {
12841281
// Have to do `to_op` first because a `Place::Local` doesn't imply the local doesn't have an address.
12851282
if let Either::Left(place) = ecx.place_to_op(place)?.as_mplace_or_imm() {
1286-
ecx.protect_place(&place)?;
1283+
ecx.protect_place(&place)?.into()
12871284
} else {
12881285
// Locals that don't have their address taken are as protected as they can ever be.
1286+
place.clone()
12891287
}
1290-
}
1288+
} else {
1289+
// No borrow tracker.
1290+
place.clone()
1291+
};
1292+
// We do need to write `uninit` so that even after the call ends, the former contents of
1293+
// this place cannot be observed any more. We do the write after retagging so that for
1294+
// Tree Borrows, this is considered to activate the new tag.
1295+
ecx.write_uninit(&protected_place)?;
1296+
// Now we throw away the protected place, ensuring its tag is never used again.
12911297
Ok(())
12921298
}
12931299

‎src/tools/miri/tests/fail/function_calls/arg_inplace_mutate.tree.stderr

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,17 @@ LL | | let non_copy = S(42);
1919
LL | |
2020
LL | | }
2121
| |_____^
22-
help: the protected tag <TAG> was created here, in the initial state Active
22+
help: the protected tag <TAG> was created here, in the initial state Reserved
2323
--> $DIR/arg_inplace_mutate.rs:LL:CC
2424
|
2525
LL | unsafe { ptr.write(S(0)) };
2626
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
27+
help: the protected tag <TAG> later transitioned to Active due to a child write access at offsets [0x0..0x4]
28+
--> $DIR/arg_inplace_mutate.rs:LL:CC
29+
|
30+
LL | unsafe { ptr.write(S(0)) };
31+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
32+
= help: this transition corresponds to the first write to a 2-phase borrowed mutable reference
2733
= note: BACKTRACE (of the first span):
2834
= note: inside `callee` at $DIR/arg_inplace_mutate.rs:LL:CC
2935
note: inside `main`

‎src/tools/miri/tests/fail/function_calls/arg_inplace_observe_during.tree.stderr

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,17 @@ LL | | let non_copy = S(42);
1919
LL | |
2020
LL | | }
2121
| |_____^
22-
help: the protected tag <TAG> was created here, in the initial state Active
22+
help: the protected tag <TAG> was created here, in the initial state Reserved
2323
--> $DIR/arg_inplace_observe_during.rs:LL:CC
2424
|
2525
LL | x.0 = 0;
2626
| ^^^^^^^
27+
help: the protected tag <TAG> later transitioned to Active due to a child write access at offsets [0x0..0x4]
28+
--> $DIR/arg_inplace_observe_during.rs:LL:CC
29+
|
30+
LL | x.0 = 0;
31+
| ^^^^^^^
32+
= help: this transition corresponds to the first write to a 2-phase borrowed mutable reference
2733
= note: BACKTRACE (of the first span):
2834
= note: inside `change_arg` at $DIR/arg_inplace_observe_during.rs:LL:CC
2935
note: inside `main`

‎src/tools/miri/tests/fail/function_calls/return_pointer_aliasing.tree.stderr

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,17 @@ LL | | let ptr = &raw mut x;
1919
LL | | }
2020
LL | | }
2121
| |_____^
22-
help: the protected tag <TAG> was created here, in the initial state Active
22+
help: the protected tag <TAG> was created here, in the initial state Reserved
2323
--> $DIR/return_pointer_aliasing.rs:LL:CC
2424
|
2525
LL | unsafe { ptr.read() };
2626
| ^^^^^^^^^^^^^^^^^^^^^
27+
help: the protected tag <TAG> later transitioned to Active due to a child write access at offsets [0x0..0x4]
28+
--> $DIR/return_pointer_aliasing.rs:LL:CC
29+
|
30+
LL | unsafe { ptr.read() };
31+
| ^^^^^^^^^^^^^^^^^^^^^
32+
= help: this transition corresponds to the first write to a 2-phase borrowed mutable reference
2733
= note: BACKTRACE (of the first span):
2834
= note: inside `myfun` at $DIR/return_pointer_aliasing.rs:LL:CC
2935
note: inside `main`

‎src/tools/miri/tests/fail/function_calls/return_pointer_aliasing2.stderr

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,17 @@ LL | | let ptr = &raw mut _x;
1919
LL | | }
2020
LL | | }
2121
| |_____^
22-
help: the protected tag <TAG> was created here, in the initial state Active
22+
help: the protected tag <TAG> was created here, in the initial state Reserved
2323
--> $DIR/return_pointer_aliasing2.rs:LL:CC
2424
|
2525
LL | unsafe { ptr.write(0) };
2626
| ^^^^^^^^^^^^^^^^^^^^^^^
27+
help: the protected tag <TAG> later transitioned to Active due to a child write access at offsets [0x0..0x4]
28+
--> $DIR/return_pointer_aliasing2.rs:LL:CC
29+
|
30+
LL | unsafe { ptr.write(0) };
31+
| ^^^^^^^^^^^^^^^^^^^^^^^
32+
= help: this transition corresponds to the first write to a 2-phase borrowed mutable reference
2733
= note: BACKTRACE (of the first span):
2834
= note: inside `myfun` at $DIR/return_pointer_aliasing2.rs:LL:CC
2935
note: inside `main`

0 commit comments

Comments
 (0)