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

Instrumentation for delayed UB stemming from uninitialized memory #3374

Merged
merged 64 commits into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
b2e6d8f
Instrumentation for delayed UB stemming from uninitialized memory
artemagvanian Jul 23, 2024
9d3783e
Merge branch 'main' into delayed-ub
artemagvanian Jul 23, 2024
40c8047
Add minimal statics support, clean up atomics
artemagvanian Jul 24, 2024
6c9989d
Merge branch 'main' into delayed-ub
artemagvanian Jul 24, 2024
9df425a
Add tests for different ways to trigger delayed UB
artemagvanian Jul 24, 2024
ee0f863
Merge branch 'main' into delayed-ub
artemagvanian Jul 24, 2024
268885f
Make sure statics can also be analysis targets
artemagvanian Jul 24, 2024
2d99ffa
Add support for closures, fix function call handling
artemagvanian Jul 24, 2024
3a0c190
Merge branch 'main' into delayed-ub
artemagvanian Jul 24, 2024
7d69b02
Formatting nit
artemagvanian Jul 24, 2024
6810535
Fix bugs with inter-function analysis, add more tests
artemagvanian Jul 25, 2024
cf13e0f
Merge branch 'main' into delayed-ub
artemagvanian Jul 25, 2024
a508e62
Fix methods changed by merge
artemagvanian Jul 25, 2024
bf1ffc6
Mitigate issues with non-deterministic memory initialization instrume…
artemagvanian Jul 26, 2024
85a550a
Correctness fixes and performance improvements
artemagvanian Jul 26, 2024
6d4bf1c
Merge branch 'main' into delayed-ub
artemagvanian Jul 26, 2024
e597689
Add more tests, remove unnecessary nodes from the graph
artemagvanian Jul 26, 2024
afb9df7
Apply clippy suggestion
artemagvanian Jul 26, 2024
08494ed
Fix an issue with terminator handling in `InstrumentationVisitor`
artemagvanian Jul 26, 2024
0f11c95
Fix expected test output files
artemagvanian Jul 29, 2024
ba56f12
Merge branch 'main' into delayed-ub
artemagvanian Jul 29, 2024
c5d25ed
More efficient intersection handling
artemagvanian Jul 29, 2024
e7bb5b8
Add a better explanation for running reachability analysis twice
artemagvanian Jul 29, 2024
1792a68
Move points-to analysis into a separate module, organize imports
artemagvanian Jul 29, 2024
f4601c1
Use StaticDef as analysis target
artemagvanian Jul 29, 2024
1186426
Add a comment in `initial_target_visitor.rs`
artemagvanian Jul 29, 2024
cf877b1
Fix small bug in delayed UB instrumentation visitor
artemagvanian Jul 29, 2024
0a6be41
Systematic ptr/ref transmute handling
artemagvanian Jul 29, 2024
f527f2e
Changes from code review
artemagvanian Jul 29, 2024
5d6beee
Merge branch 'main' into delayed-ub
artemagvanian Jul 29, 2024
50ef746
Formatting change
artemagvanian Jul 29, 2024
bd7e960
Link to the closed StableMIR PR
artemagvanian Jul 29, 2024
5fe54f2
Merge branch 'main' into delayed-ub
artemagvanian Jul 30, 2024
076f5ad
Handle spurious failures caused by delayed UB instrumentation
artemagvanian Jul 30, 2024
8014b42
Merge branch 'main' into delayed-ub
artemagvanian Jul 31, 2024
b5b7660
Add more comments to the points_to_analysis
artemagvanian Jul 31, 2024
6812fce
Merge branch 'main' into delayed-ub
artemagvanian Jul 31, 2024
ac6591d
Add more comments to the analysis
artemagvanian Jul 31, 2024
c6187b3
Merge branch 'main' into delayed-ub
artemagvanian Aug 1, 2024
dcdc019
Merge branch 'main' into delayed-ub
artemagvanian Aug 1, 2024
bedaa46
Wrap `PointsToAnalysis::run` into a separate function
artemagvanian Aug 1, 2024
421b136
Remove unnecessary body copy
artemagvanian Aug 1, 2024
3b3fdda
Update the documentation for `PointsToGraph`
artemagvanian Aug 1, 2024
a2e03c6
Rename `GlobalMemLoc -> MemLoc`
artemagvanian Aug 1, 2024
186abe3
Rename `MemLoc::Global` -> `MemLoc::Static`
artemagvanian Aug 1, 2024
6de9bfd
Expand on the doc comment of `LocalMemLoc::Alloc`
artemagvanian Aug 1, 2024
a0ef351
More changes to `PointsToGraph`
artemagvanian Aug 1, 2024
5114525
Remove confusing references to "scalar places"
artemagvanian Aug 1, 2024
759a540
Accept suggestion from code review
artemagvanian Aug 1, 2024
0354c4e
Merge branch 'main' into delayed-ub
artemagvanian Aug 1, 2024
f1172d6
Merge branch 'main' into delayed-ub
artemagvanian Aug 2, 2024
41bcfb0
Merge branch 'main' into delayed-ub
artemagvanian Aug 2, 2024
c7eed14
Formatting change
artemagvanian Aug 2, 2024
136d5a2
Remove `merge` method duplicate
artemagvanian Aug 2, 2024
7cd2f80
Accept suggestion from clippy
artemagvanian Aug 2, 2024
078d38a
Merge branch 'main' into delayed-ub
artemagvanian Aug 2, 2024
54bea65
Documentation changes / renaming
artemagvanian Aug 2, 2024
3d43def
Accept suggestions for error messages
artemagvanian Aug 2, 2024
9deb5a3
Move `successors_for_rvalue` into a separate function
artemagvanian Aug 2, 2024
700033e
Add a comment about intrinsics
artemagvanian Aug 2, 2024
0197e2a
Merge branch 'main' into delayed-ub
artemagvanian Aug 2, 2024
4cba01e
Restrict use of all internal MIR types to `points_to` module
artemagvanian Aug 2, 2024
8671a38
Small comment update
artemagvanian Aug 5, 2024
52daa97
Merge branch 'main' into delayed-ub
artemagvanian Aug 5, 2024
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
18 changes: 16 additions & 2 deletions kani-compiler/src/codegen_cprover_gotoc/compiler_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,13 @@ impl GotocCodegenBackend {
check_contract: Option<InternalDefId>,
mut transformer: BodyTransformation,
) -> (GotocCtx<'tcx>, Vec<MonoItem>, Option<AssignsContract>) {
// This runs reachability analysis before global passes are applied.
//
// Alternatively, we could run reachability only once after the global passes are applied
// and resolve the necessary dependencies inside the passes on the fly. This, however, has a
// disadvantage of not having a precomputed call graph for the global passes to use. The
// call graph could be used, for example, in resolving function pointer or vtable calls for
// global passes that need this.
let (items, call_graph) = with_timer(
|| collect_reachable_items(tcx, &mut transformer, starting_items),
"codegen reachability analysis",
Expand Down Expand Up @@ -115,6 +122,13 @@ impl GotocCodegenBackend {
call_graph,
);

// Re-collect reachable items after global transformations were applied. This is necessary
artemagvanian marked this conversation as resolved.
Show resolved Hide resolved
// since global pass could add extra calls to instrumentation.
celinval marked this conversation as resolved.
Show resolved Hide resolved
let (items, _) = with_timer(
|| collect_reachable_items(tcx, &mut transformer, starting_items),
"codegen reachability analysis (second pass)",
);

// Follow rustc naming convention (cx is abbrev for context).
// https://rustc-dev-guide.rust-lang.org/conventions.html#naming-conventions
let mut gcx =
Expand Down Expand Up @@ -260,8 +274,8 @@ impl CodegenBackend for GotocCodegenBackend {
for unit in units.iter() {
// We reset the body cache for now because each codegen unit has different
// configurations that affect how we transform the instance body.
let mut transformer = BodyTransformation::new(&queries, tcx, &unit);
for harness in &unit.harnesses {
let transformer = BodyTransformation::new(&queries, tcx, &unit);
zhassan-aws marked this conversation as resolved.
Show resolved Hide resolved
let model_path = units.harness_model_path(*harness).unwrap();
let contract_metadata =
contract_metadata_for_harness(tcx, harness.def.def_id()).unwrap();
Expand All @@ -273,7 +287,7 @@ impl CodegenBackend for GotocCodegenBackend {
contract_metadata,
transformer,
);
transformer = results.extend(gcx, items, None);
results.extend(gcx, items, None);
if let Some(assigns_contract) = contract_info {
modifies_instances.push((*harness, assigns_contract));
}
Expand Down
6 changes: 6 additions & 0 deletions kani-compiler/src/kani_middle/transform/body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,12 @@ impl SourceInstruction {
SourceInstruction::Terminator { bb } => blocks[bb].terminator.span,
}
}

pub fn bb(&self) -> usize {
match self {
SourceInstruction::Statement { bb, .. } | SourceInstruction::Terminator { bb } => *bb,
}
}
}

fn find_instance(tcx: TyCtxt, diagnostic: &str) -> Option<Instance> {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
// Copyright Kani Contributors
// SPDX-License-Identifier: Apache-2.0 OR MIT
//
//! This module contains the visitor responsible for collecting initial analysis targets for delayed
artemagvanian marked this conversation as resolved.
Show resolved Hide resolved
//! UB instrumentation.

use stable_mir::{
mir::{
alloc::GlobalAlloc,
mono::{Instance, InstanceKind},
visit::Location,
Body, CastKind, LocalDecl, MirVisitor, Mutability, NonDivergingIntrinsic, Operand, Place,
Rvalue, Statement, StatementKind, Terminator, TerminatorKind,
},
ty::{ConstantKind, RigidTy, TyKind},
CrateDef, DefId,
};

use crate::kani_middle::transform::check_uninit::ty_layout::tys_layout_compatible;

/// Pointer, write through which might trigger delayed UB.
pub enum AnalysisTarget {
Place(Place),
Static(DefId),
artemagvanian marked this conversation as resolved.
Show resolved Hide resolved
}

/// Visitor that finds initial analysis targets for delayed UB instrumentation. For our purposes,
/// analysis targets are *pointers* to places reading and writing from which should be tracked.
pub struct InitialTargetVisitor {
body: Body,
targets: Vec<AnalysisTarget>,
}

impl InitialTargetVisitor {
pub fn new(body: Body) -> Self {
Self { body, targets: vec![] }
}

pub fn into_targets(self) -> Vec<AnalysisTarget> {
self.targets
}

pub fn push_operand(&mut self, operand: &Operand) {
match operand {
Operand::Copy(place) | Operand::Move(place) => {
self.targets.push(AnalysisTarget::Place(place.clone()));
}
Operand::Constant(constant) => {
// Extract the static from the constant.
if let ConstantKind::Allocated(allocation) = constant.const_.kind() {
for (_, prov) in &allocation.provenance.ptrs {
if let GlobalAlloc::Static(static_def) = GlobalAlloc::from(prov.0) {
self.targets.push(AnalysisTarget::Static(static_def.def_id()));
artemagvanian marked this conversation as resolved.
Show resolved Hide resolved
};
}
}
}
}
}
}

impl MirVisitor for InitialTargetVisitor {
fn visit_rvalue(&mut self, rvalue: &Rvalue, location: Location) {
artemagvanian marked this conversation as resolved.
Show resolved Hide resolved
if let Rvalue::Cast(kind, operand, ty) = rvalue {
let operand_ty = operand.ty(self.body.locals()).unwrap();
match kind {
CastKind::Transmute | CastKind::PtrToPtr => {
if let (
RigidTy::RawPtr(from_ty, Mutability::Mut),
celinval marked this conversation as resolved.
Show resolved Hide resolved
RigidTy::RawPtr(to_ty, Mutability::Mut),
) = (operand_ty.kind().rigid().unwrap(), ty.kind().rigid().unwrap())
{
if !tys_layout_compatible(from_ty, to_ty) {
self.push_operand(operand);
}
}
}
_ => {}
};
}
self.super_rvalue(rvalue, location);
}

fn visit_statement(&mut self, stmt: &Statement, location: Location) {
if let StatementKind::Intrinsic(NonDivergingIntrinsic::CopyNonOverlapping(copy)) =
&stmt.kind
{
self.push_operand(&copy.dst);
}
self.super_statement(stmt, location);
}

fn visit_terminator(&mut self, term: &Terminator, location: Location) {
if let TerminatorKind::Call { func, args, .. } = &term.kind {
let instance = match try_resolve_instance(self.body.locals(), func) {
Ok(instance) => instance,
Err(reason) => {
panic!("{reason}");
}
artemagvanian marked this conversation as resolved.
Show resolved Hide resolved
};
if instance.kind == InstanceKind::Intrinsic {
match instance.intrinsic_name().unwrap().as_str() {
"copy" => {
assert_eq!(args.len(), 3, "Unexpected number of arguments for `copy`");
assert!(matches!(
args[0].ty(self.body.locals()).unwrap().kind(),
TyKind::RigidTy(RigidTy::RawPtr(_, Mutability::Not))
));
assert!(matches!(
args[1].ty(self.body.locals()).unwrap().kind(),
TyKind::RigidTy(RigidTy::RawPtr(_, Mutability::Mut))
));
// Here, `dst` is the second argument.
self.push_operand(&args[1]);
}
"volatile_copy_memory" | "volatile_copy_nonoverlapping_memory" => {
assert_eq!(
args.len(),
3,
"Unexpected number of arguments for `volatile_copy`"
);
assert!(matches!(
args[0].ty(self.body.locals()).unwrap().kind(),
TyKind::RigidTy(RigidTy::RawPtr(_, Mutability::Mut))
));
assert!(matches!(
args[1].ty(self.body.locals()).unwrap().kind(),
TyKind::RigidTy(RigidTy::RawPtr(_, Mutability::Not))
));
// Here, `dst` is the first argument.
self.push_operand(&args[0]);
}
_ => {}
}
}
}
self.super_terminator(term, location);
}
}

/// Try retrieving instance for the given function operand.
fn try_resolve_instance(locals: &[LocalDecl], func: &Operand) -> Result<Instance, String> {
let ty = func.ty(locals).unwrap();
match ty.kind() {
TyKind::RigidTy(RigidTy::FnDef(def, args)) => Ok(Instance::resolve(def, &args).unwrap()),
_ => Err(format!("Kani does not support reasoning about arguments to `{ty:?}`.")),
artemagvanian marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
// Copyright Kani Contributors
// SPDX-License-Identifier: Apache-2.0 OR MIT
//
//! Visitor that collects all instructions relevant to uninitialized memory access caused by delayed
//! UB. In practice, that means collecting all instructions where the place is featured.

use crate::kani_middle::transform::body::{InsertPosition, MutableBody, SourceInstruction};
use crate::kani_middle::transform::check_uninit::delayed_ub::points_to_graph::{
GlobalMemLoc, PointsToGraph,
};
use crate::kani_middle::transform::check_uninit::relevant_instruction::{
InitRelevantInstruction, MemoryInitOp,
};
use crate::kani_middle::transform::check_uninit::TargetFinder;
use rustc_hir::def_id::DefId as InternalDefId;
use rustc_middle::ty::TyCtxt;
use rustc_smir::rustc_internal;
use stable_mir::mir::visit::{Location, PlaceContext};
use stable_mir::mir::{BasicBlockIdx, MirVisitor, Operand, Place, Statement, Terminator};
use std::collections::HashSet;

pub struct InstrumentationVisitor<'a, 'tcx> {
/// Whether we should skip the next instruction, since it might've been instrumented already.
/// When we instrument an instruction, we partition the basic block, and the instruction that
/// may trigger UB becomes the first instruction of the basic block, which we need to skip
/// later.
skip_next: bool,
/// The instruction being visited at a given point.
current: SourceInstruction,
/// The target instruction that should be verified.
pub target: Option<InitRelevantInstruction>,
/// Aliasing analysis data.
points_to: &'a PointsToGraph<'tcx>,
/// The list of places we should be looking for, ignoring others
analysis_targets: &'a HashSet<GlobalMemLoc<'tcx>>,
current_def_id: InternalDefId,
tcx: TyCtxt<'tcx>,
}

impl<'a, 'tcx> TargetFinder for InstrumentationVisitor<'a, 'tcx> {
fn find_next(
&mut self,
body: &MutableBody,
bb: BasicBlockIdx,
skip_first: bool,
) -> Option<InitRelevantInstruction> {
self.skip_next = skip_first;
self.current = SourceInstruction::Statement { idx: 0, bb };
self.target = None;
self.visit_basic_block(&body.blocks()[bb]);
self.target.clone()
}
}

impl<'a, 'tcx> InstrumentationVisitor<'a, 'tcx> {
pub fn new(
points_to: &'a PointsToGraph<'tcx>,
analysis_targets: &'a HashSet<GlobalMemLoc<'tcx>>,
current_def_id: InternalDefId,
tcx: TyCtxt<'tcx>,
) -> Self {
Self {
skip_next: false,
current: SourceInstruction::Statement { idx: 0, bb: 0 },
target: None,
points_to,
analysis_targets,
current_def_id,
tcx,
}
}
fn push_target(&mut self, source_op: MemoryInitOp) {
let target = self.target.get_or_insert_with(|| InitRelevantInstruction {
source: self.current,
after_instruction: vec![],
before_instruction: vec![],
});
target.push_operation(source_op);
}
}

impl<'a, 'tcx> MirVisitor for InstrumentationVisitor<'a, 'tcx> {
fn visit_statement(&mut self, stmt: &Statement, location: Location) {
if self.skip_next {
self.skip_next = false;
} else if self.target.is_none() {
// Check all inner places.
self.super_statement(stmt, location);
// Switch to the next statement.
let SourceInstruction::Statement { idx, bb } = self.current else { unreachable!() };
self.current = SourceInstruction::Statement { idx: idx + 1, bb };
}
}

fn visit_terminator(&mut self, term: &Terminator, location: Location) {
if !(self.skip_next || self.target.is_some()) {
self.current = SourceInstruction::Terminator { bb: self.current.bb() };
self.super_terminator(term, location);
}
}

fn visit_place(&mut self, place: &Place, ptx: PlaceContext, location: Location) {
// Match the place by whatever it is pointing to and find an intersection with the targets.
if self
.points_to
.follow_from_place(rustc_internal::internal(self.tcx, place), self.current_def_id)
.intersection(&self.analysis_targets)
.next()
.is_some()
{
// If we are mutating the place, initialize it.
if ptx.is_mutating() {
self.push_target(MemoryInitOp::SetRef {
operand: Operand::Copy(place.clone()),
value: true,
position: InsertPosition::After,
});
} else {
// Otherwise, check its initialization.
self.push_target(MemoryInitOp::CheckRef { operand: Operand::Copy(place.clone()) });
}
}
self.super_place(place, ptx, location)
}
}
Loading
Loading