Skip to content

Commit cb1b881

Browse files
RalfJungRalfJung
committed
add a weak form of protection that justifies Box noalias
1 parent 94ddca1 commit cb1b881

20 files changed

+146
-65
lines changed

‎src/stacked_borrows/diagnostics.rs

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use rustc_span::{Span, SpanData};
66
use rustc_target::abi::Size;
77

88
use crate::helpers::CurrentSpan;
9-
use crate::stacked_borrows::{err_sb_ub, AccessKind, GlobalStateInner, Permission};
9+
use crate::stacked_borrows::{err_sb_ub, AccessKind, GlobalStateInner, Permission, ProtectorKind};
1010
use crate::*;
1111

1212
use rustc_middle::mir::interpret::InterpError;
@@ -288,7 +288,11 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir
288288
}
289289
Operation::Access(AccessOp { kind, range, .. }) =>
290290
(*range, InvalidationCause::Access(*kind)),
291-
_ => unreachable!("Tags can only be invalidated during a retag or access"),
291+
_ => {
292+
// This can be reached, but never be relevant later since the entire allocation is
293+
// gone now.
294+
return;
295+
}
292296
};
293297
self.history.invalidations.push(Invalidation { tag, range, span, cause });
294298
}
@@ -369,7 +373,7 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir
369373

370374
/// Report a descriptive error when `new` could not be granted from `derived_from`.
371375
#[inline(never)] // This is only called on fatal code paths
372-
pub fn grant_error(&self, perm: Permission, stack: &Stack) -> InterpError<'tcx> {
376+
pub(super) fn grant_error(&self, perm: Permission, stack: &Stack) -> InterpError<'tcx> {
373377
let Operation::Retag(op) = &self.operation else {
374378
unreachable!("grant_error should only be called during a retag")
375379
};
@@ -389,7 +393,7 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir
389393

390394
/// Report a descriptive error when `access` is not permitted based on `tag`.
391395
#[inline(never)] // This is only called on fatal code paths
392-
pub fn access_error(&self, stack: &Stack) -> InterpError<'tcx> {
396+
pub(super) fn access_error(&self, stack: &Stack) -> InterpError<'tcx> {
393397
let Operation::Access(op) = &self.operation else {
394398
unreachable!("access_error should only be called during an access")
395399
};
@@ -408,7 +412,11 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir
408412
}
409413

410414
#[inline(never)] // This is only called on fatal code paths
411-
pub fn protector_error(&self, item: &Item) -> InterpError<'tcx> {
415+
pub(super) fn protector_error(&self, item: &Item, kind: ProtectorKind) -> InterpError<'tcx> {
416+
let protected = match kind {
417+
ProtectorKind::WeakProtector => "weakly protected",
418+
ProtectorKind::StrongProtector => "strongly protected",
419+
};
412420
let call_id = self
413421
.threads
414422
.all_stacks()
@@ -422,19 +430,15 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir
422430
match self.operation {
423431
Operation::Dealloc(_) =>
424432
err_sb_ub(
425-
format!(
426-
"deallocating while item {:?} is protected by call {:?}",
427-
item, call_id
428-
),
433+
format!("deallocating while item {item:?} is {protected} by call {call_id:?}",),
429434
None,
430435
None,
431436
),
432437
Operation::Retag(RetagOp { orig_tag: tag, .. })
433438
| Operation::Access(AccessOp { tag, .. }) =>
434439
err_sb_ub(
435440
format!(
436-
"not granting access to tag {:?} because that would remove {:?} which is protected because it is an argument of call {:?}",
437-
tag, item, call_id
441+
"not granting access to tag {tag:?} because that would remove {item:?} which is {protected} because it is an argument of call {call_id:?}",
438442
),
439443
None,
440444
tag.and_then(|tag| self.get_logs_relevant_to(tag, Some(item.tag()))),

‎src/stacked_borrows/mod.rs

Lines changed: 60 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,26 @@ pub struct Stacks {
9191
modified_since_last_gc: bool,
9292
}
9393

94+
/// The flavor of the protector.
95+
#[derive(Copy, Clone, Debug)]
96+
enum ProtectorKind {
97+
/// Protected against aliasing violations from other pointers.
98+
///
99+
/// Items protected like this cause UB when they are invalidated, *but* the pointer itself may
100+
/// still be used to issue a deallocation.
101+
///
102+
/// This is required for LLVM IR pointers that are `noalias` but *not* `dereferenceable`.
103+
WeakProtector,
104+
105+
/// Protected against any kind of invalidation.
106+
///
107+
/// Items protected like this cause UB when they are invalidated or the memory is deallocated.
108+
/// This is strictly stronger protection than `WeakProtector`.
109+
///
110+
/// This is required for LLVM IR pointers that are `dereferenceable` (and also allows `noalias`).
111+
StrongProtector,
112+
}
113+
94114
/// Extra global state, available to the memory access hooks.
95115
#[derive(Debug)]
96116
pub struct GlobalStateInner {
@@ -102,12 +122,12 @@ pub struct GlobalStateInner {
102122
base_ptr_tags: FxHashMap<AllocId, SbTag>,
103123
/// Next unused call ID (for protectors).
104124
next_call_id: CallId,
105-
/// All currently protected tags.
125+
/// All currently protected tags, and the status of their protection.
106126
/// An item is protected if its tag is in this set, *and* it has the "protected" bit set.
107127
/// We add tags to this when they are created with a protector in `reborrow`, and
108128
/// we remove tags from this when the call which is protecting them returns, in
109129
/// `GlobalStateInner::end_call`. See `Stack::item_popped` for more details.
110-
protected_tags: FxHashSet<SbTag>,
130+
protected_tags: FxHashMap<SbTag, ProtectorKind>,
111131
/// The pointer ids to trace
112132
tracked_pointer_tags: FxHashSet<SbTag>,
113133
/// The call ids to trace
@@ -189,7 +209,7 @@ impl GlobalStateInner {
189209
next_ptr_tag: SbTag(NonZeroU64::new(1).unwrap()),
190210
base_ptr_tags: FxHashMap::default(),
191211
next_call_id: NonZeroU64::new(1).unwrap(),
192-
protected_tags: FxHashSet::default(),
212+
protected_tags: FxHashMap::default(),
193213
tracked_pointer_tags,
194214
tracked_call_ids,
195215
retag_fields,
@@ -314,6 +334,7 @@ impl<'tcx> Stack {
314334
item: &Item,
315335
global: &GlobalStateInner,
316336
dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>,
337+
deallocation: bool,
317338
) -> InterpResult<'tcx> {
318339
if !global.tracked_pointer_tags.is_empty() {
319340
dcx.check_tracked_tag_popped(item, global);
@@ -336,8 +357,11 @@ impl<'tcx> Stack {
336357
// 2. Most frames protect only one or two tags. So this duplicative global turns a search
337358
// which ends up about linear in the number of protected tags in the program into a
338359
// constant time check (and a slow linear, because the tags in the frames aren't contiguous).
339-
if global.protected_tags.contains(&item.tag()) {
340-
return Err(dcx.protector_error(item).into());
360+
if let Some(&protector_kind) = global.protected_tags.get(&item.tag()) {
361+
let allowed = deallocation && matches!(protector_kind, ProtectorKind::WeakProtector);
362+
if !allowed {
363+
return Err(dcx.protector_error(item, protector_kind).into());
364+
}
341365
}
342366
Ok(())
343367
}
@@ -350,7 +374,7 @@ impl<'tcx> Stack {
350374
&mut self,
351375
access: AccessKind,
352376
tag: ProvenanceExtra,
353-
global: &mut GlobalStateInner,
377+
global: &GlobalStateInner,
354378
dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>,
355379
exposed_tags: &FxHashSet<SbTag>,
356380
) -> InterpResult<'tcx> {
@@ -377,7 +401,7 @@ impl<'tcx> Stack {
377401
0
378402
};
379403
self.pop_items_after(first_incompatible_idx, |item| {
380-
Stack::item_popped(&item, global, dcx)?;
404+
Stack::item_popped(&item, global, dcx, /* deallocation */ false)?;
381405
dcx.log_invalidation(item.tag());
382406
Ok(())
383407
})?;
@@ -398,7 +422,7 @@ impl<'tcx> Stack {
398422
0
399423
};
400424
self.disable_uniques_starting_at(first_incompatible_idx, |item| {
401-
Stack::item_popped(&item, global, dcx)?;
425+
Stack::item_popped(&item, global, dcx, /* deallocation */ false)?;
402426
dcx.log_invalidation(item.tag());
403427
Ok(())
404428
})?;
@@ -440,14 +464,15 @@ impl<'tcx> Stack {
440464
dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>,
441465
exposed_tags: &FxHashSet<SbTag>,
442466
) -> InterpResult<'tcx> {
443-
// Step 1: Make sure there is a granting item.
444-
self.find_granting(AccessKind::Write, tag, exposed_tags)
467+
// Step 1: Make a write access.
468+
// As part of this we do regular protector checking, i.e. even weakly protected items cause UB when popped.
469+
self.access(AccessKind::Write, tag, global, dcx, exposed_tags)
445470
.map_err(|_| dcx.dealloc_error())?;
446471

447-
// Step 2: Consider all items removed. This checks for protectors.
472+
// Step 2: Pretend we remove the remaining items, checking if any are strongly protected.
448473
for idx in (0..self.len()).rev() {
449474
let item = self.get(idx).unwrap();
450-
Stack::item_popped(&item, global, dcx)?;
475+
Stack::item_popped(&item, global, dcx, /* deallocation */ true)?;
451476
}
452477

453478
Ok(())
@@ -698,7 +723,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
698723
kind: RefKind,
699724
retag_cause: RetagCause, // What caused this retag, for diagnostics only
700725
new_tag: SbTag,
701-
protect: bool,
726+
protect: Option<ProtectorKind>,
702727
) -> InterpResult<'tcx, Option<AllocId>> {
703728
let this = self.eval_context_mut();
704729

@@ -761,7 +786,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
761786
);
762787
let mut dcx = dcx.build(&mut stacked_borrows.history, base_offset);
763788
dcx.log_creation();
764-
if protect {
789+
if protect.is_some() {
765790
dcx.log_protector();
766791
}
767792
}
@@ -821,10 +846,16 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
821846
size.bytes()
822847
);
823848

824-
if protect {
849+
if let Some(protect) = protect {
825850
// See comment in `Stack::item_popped` for why we store the tag twice.
826851
this.frame_mut().extra.stacked_borrows.as_mut().unwrap().protected_tags.push(new_tag);
827-
this.machine.stacked_borrows.as_mut().unwrap().get_mut().protected_tags.insert(new_tag);
852+
this.machine
853+
.stacked_borrows
854+
.as_mut()
855+
.unwrap()
856+
.get_mut()
857+
.protected_tags
858+
.insert(new_tag, protect);
828859
}
829860

830861
// Update the stacks.
@@ -866,7 +897,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
866897
Permission::SharedReadWrite
867898
};
868899
let protected = if frozen {
869-
protect
900+
protect.is_some()
870901
} else {
871902
// We do not protect inside UnsafeCell.
872903
// This fixes https://github.com/rust-lang/rust/issues/55005.
@@ -899,7 +930,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
899930
.as_mut()
900931
.expect("we should have Stacked Borrows data")
901932
.borrow_mut();
902-
let item = Item::new(new_tag, perm, protect);
933+
let item = Item::new(new_tag, perm, protect.is_some());
903934
let range = alloc_range(base_offset, size);
904935
let mut global = machine.stacked_borrows.as_ref().unwrap().borrow_mut();
905936
// FIXME: can't share this with the current_span inside log_creation
@@ -926,7 +957,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
926957
val: &ImmTy<'tcx, Provenance>,
927958
kind: RefKind,
928959
retag_cause: RetagCause, // What caused this retag, for diagnostics only
929-
protect: bool,
960+
protect: Option<ProtectorKind>,
930961
) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
931962
let this = self.eval_context_mut();
932963
// We want a place for where the ptr *points to*, so we get one.
@@ -996,7 +1027,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
9961027
place: &PlaceTy<'tcx, Provenance>,
9971028
ref_kind: RefKind,
9981029
retag_cause: RetagCause,
999-
protector: bool,
1030+
protector: Option<ProtectorKind>,
10001031
) -> InterpResult<'tcx> {
10011032
let val = self.ecx.read_immediate(&self.ecx.place_to_op(place)?)?;
10021033
let val = self.ecx.retag_reference(&val, ref_kind, retag_cause, protector)?;
@@ -1015,13 +1046,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
10151046
}
10161047

10171048
fn visit_box(&mut self, place: &PlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
1018-
// Boxes do not get a protector: protectors reflect that references outlive the call
1019-
// they were passed in to; that's just not the case for boxes.
1049+
// Boxes get a weak protectors, since they may be deallocated.
10201050
self.retag_place(
10211051
place,
10221052
RefKind::Unique { two_phase: false },
10231053
self.retag_cause,
1024-
/*protector*/ false,
1054+
/*protector*/
1055+
(self.kind == RetagKind::FnEntry).then_some(ProtectorKind::WeakProtector),
10251056
)
10261057
}
10271058

@@ -1046,7 +1077,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
10461077
place,
10471078
ref_kind,
10481079
self.retag_cause,
1049-
/*protector*/ self.kind == RetagKind::FnEntry,
1080+
/*protector*/
1081+
(self.kind == RetagKind::FnEntry)
1082+
.then_some(ProtectorKind::StrongProtector),
10501083
)?;
10511084
}
10521085
ty::RawPtr(tym) => {
@@ -1059,7 +1092,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
10591092
place,
10601093
RefKind::Raw { mutable: tym.mutbl == Mutability::Mut },
10611094
self.retag_cause,
1062-
/*protector*/ false,
1095+
/*protector*/ None,
10631096
)?;
10641097
}
10651098
}
@@ -1110,12 +1143,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
11101143
// (The pointer type does not matter, so we use a raw pointer.)
11111144
let ptr_layout = this.layout_of(this.tcx.mk_mut_ptr(return_place.layout.ty))?;
11121145
let val = ImmTy::from_immediate(return_place.to_ref(this), ptr_layout);
1113-
// Reborrow it.
1146+
// Reborrow it. With protection! That is part of the point.
11141147
let val = this.retag_reference(
11151148
&val,
11161149
RefKind::Unique { two_phase: false },
11171150
RetagCause::FnReturn,
1118-
/*protector*/ true,
1151+
/*protector*/ Some(ProtectorKind::StrongProtector),
11191152
)?;
11201153
// And use reborrowed pointer for return place.
11211154
let return_place = this.ref_to_mplace(&val)?;

‎tests/fail/stacked_borrows/aliasing_mut1.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is protected because it is an argument of call ID
1+
error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected because it is an argument of call ID
22
--> $DIR/aliasing_mut1.rs:LL:CC
33
|
44
LL | pub fn safe(_x: &mut i32, _y: &mut i32) {}
5-
| ^^ not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is protected because it is an argument of call ID
5+
| ^^ not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected because it is an argument of call ID
66
|
77
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
88
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

‎tests/fail/stacked_borrows/aliasing_mut2.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
error: Undefined Behavior: not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is protected because it is an argument of call ID
1+
error: Undefined Behavior: not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is strongly protected because it is an argument of call ID
22
--> $DIR/aliasing_mut2.rs:LL:CC
33
|
44
LL | pub fn safe(_x: &i32, _y: &mut i32) {}
5-
| ^^ not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is protected because it is an argument of call ID
5+
| ^^ not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is strongly protected because it is an argument of call ID
66
|
77
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
88
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

‎tests/fail/stacked_borrows/aliasing_mut4.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
error: Undefined Behavior: not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is protected because it is an argument of call ID
1+
error: Undefined Behavior: not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is strongly protected because it is an argument of call ID
22
--> $DIR/aliasing_mut4.rs:LL:CC
33
|
44
LL | pub fn safe(_x: &i32, _y: &mut Cell<i32>) {}
5-
| ^^ not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is protected because it is an argument of call ID
5+
| ^^ not granting access to tag <TAG> because that would remove [SharedReadOnly for <TAG>] which is strongly protected because it is an argument of call ID
66
|
77
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
88
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

‎tests/fail/stacked_borrows/box_noalias_violation.rs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
unsafe fn test(mut x: Box<i32>, y: *const i32) -> i32 {
2+
// We will call this in a way that x and y alias.
3+
*x = 5;
4+
std::mem::forget(x);
5+
*y //~ERROR: weakly protected
6+
}
7+
8+
fn main() {
9+
unsafe {
10+
let mut v = 42;
11+
let ptr = &mut v as *mut i32;
12+
test(Box::from_raw(ptr), ptr);
13+
}
14+
}

0 commit comments

Comments
 (0)