From d97cc2b222bc19b733178f95bc0658875c9ff044 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Mon, 5 Aug 2024 11:18:47 -0700 Subject: [PATCH 01/11] analyze: borrowck: pre-assign origins to cast target types --- c2rust-analyze/src/borrowck/mod.rs | 21 +++++++++++++++++++++ c2rust-analyze/src/borrowck/type_check.rs | 10 +++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/c2rust-analyze/src/borrowck/mod.rs b/c2rust-analyze/src/borrowck/mod.rs index 2330a62d1..a1f2eb72d 100644 --- a/c2rust-analyze/src/borrowck/mod.rs +++ b/c2rust-analyze/src/borrowck/mod.rs @@ -326,6 +326,26 @@ fn run_polonius<'tcx>( local_ltys.push(lty); } + // Assign origins to `rvalue_tys`. We sort the keys first to ensure that we assign origins in + // a deterministic order. + let mut keys = acx.rvalue_tys.keys().collect::>(); + keys.sort(); + let rvalue_ltys = keys + .into_iter() + .map(|&loc| { + let lty = acx.rvalue_tys[&loc]; + let new_lty = assign_origins( + ltcx, + hypothesis, + &mut facts, + &mut maps, + &acx.gacx.adt_metadata, + lty, + ); + (loc, new_lty) + }) + .collect::>(); + // Gather field permissions let field_permissions = field_ltys .iter() @@ -348,6 +368,7 @@ fn run_polonius<'tcx>( &mut maps, &mut loans, &local_ltys, + &rvalue_ltys, &field_permissions, hypothesis, mir, diff --git a/c2rust-analyze/src/borrowck/type_check.rs b/c2rust-analyze/src/borrowck/type_check.rs index fc098acde..c0f323b9f 100644 --- a/c2rust-analyze/src/borrowck/type_check.rs +++ b/c2rust-analyze/src/borrowck/type_check.rs @@ -26,6 +26,7 @@ struct TypeChecker<'tcx, 'a> { maps: &'a mut AtomMaps<'tcx>, loans: &'a mut HashMap>, local_ltys: &'a [LTy<'tcx>], + rvalue_ltys: &'a HashMap>, field_permissions: &'a HashMap, hypothesis: &'a PointerTableMut<'a, PermissionSet>, local_decls: &'a IndexVec>, @@ -389,7 +390,7 @@ impl<'tcx> TypeChecker<'tcx, '_> { self.do_assign(result_lty, op_lty); result_lty } - Rvalue::Cast(_, _, ty) => self.ltcx.label(ty, &mut |_ty| { + Rvalue::Cast(_, _, _ty) => { // TODO: handle Unsize casts at minimum /* assert!( @@ -397,8 +398,9 @@ impl<'tcx> TypeChecker<'tcx, '_> { "pointer Cast NYI" ); */ - Label::default() - }), + // TODO: connect this type to the type of the operand + self.rvalue_ltys[&self.current_location] + } Rvalue::Aggregate(ref kind, ref ops) => match **kind { AggregateKind::Array(..) => { let ty = rv.ty(self.local_decls, *self.ltcx); @@ -623,6 +625,7 @@ pub fn visit_body<'tcx>( maps: &mut AtomMaps<'tcx>, loans: &mut HashMap>, local_ltys: &[LTy<'tcx>], + rvalue_ltys: &HashMap>, field_permissions: &HashMap, hypothesis: &PointerTableMut, mir: &Body<'tcx>, @@ -635,6 +638,7 @@ pub fn visit_body<'tcx>( maps, loans, local_ltys, + rvalue_ltys, field_permissions, hypothesis, local_decls: &mir.local_decls, From beac2b9bc9e21a84ef203e35ba1c173d27deaf6f Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Wed, 10 Jul 2024 13:47:56 -0700 Subject: [PATCH 02/11] analyze: rewrite: use RewriteKind::Ref instead of implicit &mut in RewriteKind::DynOwnedTake --- c2rust-analyze/src/rewrite/expr/convert.rs | 5 +---- c2rust-analyze/src/rewrite/expr/mir_op.rs | 5 ++++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/c2rust-analyze/src/rewrite/expr/convert.rs b/c2rust-analyze/src/rewrite/expr/convert.rs index 3815eb4cf..7a8d4b71c 100644 --- a/c2rust-analyze/src/rewrite/expr/convert.rs +++ b/c2rust-analyze/src/rewrite/expr/convert.rs @@ -802,10 +802,7 @@ pub fn convert_cast_rewrite(kind: &mir_op::RewriteKind, hir_rw: Rewrite) -> Rewr // `p` -> `mem::replace(&mut p, Err(()))` Rewrite::Call( "std::mem::replace".to_string(), - vec![ - Rewrite::Ref(Box::new(hir_rw), hir::Mutability::Mut), - Rewrite::Text("Err(())".into()), - ], + vec![hir_rw, Rewrite::Text("Err(())".into())], ) } mir_op::RewriteKind::DynOwnedWrap => { diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index 968e9528f..2c3ba7e4e 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -135,7 +135,7 @@ pub enum RewriteKind { /// Extract the `T` from `DynOwned`. DynOwnedUnwrap, - /// Move out of a `DynOwned` and set the original location to empty / non-owned. + /// Move out of `&mut DynOwned` and set the original location to empty / non-owned. DynOwnedTake, /// Wrap `T` in `Ok` to produce `DynOwned`. DynOwnedWrap, @@ -448,6 +448,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { v.visit_place(rv_pl, PlaceAccess::Mut); }); }); + // Obtain a reference to the place containing the `DynOwned` pointer. + v.emit(RewriteKind::Ref { mutbl: true }); + // Take the pointer out of that place. v.emit(RewriteKind::DynOwnedTake); v.emit_cast_lty_lty(rv_lty, pl_lty); return; From b75755b5ccfaeda36993ec58bceb80895982abd8 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Wed, 10 Jul 2024 13:57:42 -0700 Subject: [PATCH 03/11] analyze: mir_op: fix handling of DynOwnedTake on nullable ptrs --- c2rust-analyze/src/rewrite/expr/mir_op.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index 2c3ba7e4e..f15a9fec2 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -449,9 +449,20 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { }); }); // Obtain a reference to the place containing the `DynOwned` pointer. - v.emit(RewriteKind::Ref { mutbl: true }); + if v.is_nullable(rv_lty.label) { + v.emit(RewriteKind::OptionDowngrade { + mutbl: true, + deref: false, + }); + v.emit(RewriteKind::OptionMapBegin); + } else { + v.emit(RewriteKind::Ref { mutbl: true }); + } // Take the pointer out of that place. v.emit(RewriteKind::DynOwnedTake); + if v.is_nullable(rv_lty.label) { + v.emit(RewriteKind::OptionMapEnd); + } v.emit_cast_lty_lty(rv_lty, pl_lty); return; } From ad5d46d617aa7b24549cfa7de29ff3d1f8ad361b Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Wed, 10 Jul 2024 15:03:14 -0700 Subject: [PATCH 04/11] analyze: rewrite::apply: fix precedence of MethodCall --- c2rust-analyze/src/rewrite/apply.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c2rust-analyze/src/rewrite/apply.rs b/c2rust-analyze/src/rewrite/apply.rs index 2f9370a28..d9e6b53e6 100644 --- a/c2rust-analyze/src/rewrite/apply.rs +++ b/c2rust-analyze/src/rewrite/apply.rs @@ -361,7 +361,7 @@ impl Emitter<'_, S> { }) } Rewrite::MethodCall(ref method, ref receiver_rw, ref arg_rws) => { - self.emit(receiver_rw, 0)?; + self.emit(receiver_rw, 3)?; self.emit_str(".")?; self.emit_str(method)?; self.emit_parenthesized(true, |slf| { From 9b958adc4f9913a42da88b88a2730ca8fe32afe9 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Wed, 10 Jul 2024 15:04:04 -0700 Subject: [PATCH 05/11] analyze: rewrite: add mir_op::RewriteKind::Deref --- c2rust-analyze/src/rewrite/expr/convert.rs | 5 +++++ c2rust-analyze/src/rewrite/expr/mir_op.rs | 3 +++ 2 files changed, 8 insertions(+) diff --git a/c2rust-analyze/src/rewrite/expr/convert.rs b/c2rust-analyze/src/rewrite/expr/convert.rs index 7a8d4b71c..77354b3d2 100644 --- a/c2rust-analyze/src/rewrite/expr/convert.rs +++ b/c2rust-analyze/src/rewrite/expr/convert.rs @@ -758,6 +758,11 @@ pub fn convert_cast_rewrite(kind: &mir_op::RewriteKind, hir_rw: Rewrite) -> Rewr Rewrite::Ref(Box::new(place), mutbl_from_bool(mutbl)) } + mir_op::RewriteKind::Deref => { + // `p` -> `*p` + Rewrite::Deref(Box::new(hir_rw)) + } + mir_op::RewriteKind::OptionUnwrap => { // `p` -> `p.unwrap()` Rewrite::MethodCall("unwrap".to_string(), Box::new(hir_rw), vec![]) diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index f15a9fec2..37c1e36ad 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -66,6 +66,9 @@ pub enum RewriteKind { /// Replace &raw with & or &raw mut with &mut RawToRef { mutbl: bool }, + /// Replace `ptr` with `*ptr`. + Deref, + /// Replace `ptr.is_null()` with `ptr.is_none()`. IsNullToIsNone, /// Replace `ptr.is_null()` with the constant `false`. We use this in cases where the rewritten From 7e4f1c8916a7e93d8882e1d89bc745dc62fab5d9 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Wed, 10 Jul 2024 15:07:31 -0700 Subject: [PATCH 06/11] analyze: rewrite: use RewriteKind::Deref and implicit &mut in RewriteKind::DynOwnedTake --- c2rust-analyze/src/rewrite/expr/convert.rs | 5 ++++- c2rust-analyze/src/rewrite/expr/mir_op.rs | 5 +++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/c2rust-analyze/src/rewrite/expr/convert.rs b/c2rust-analyze/src/rewrite/expr/convert.rs index 77354b3d2..ff5fec82c 100644 --- a/c2rust-analyze/src/rewrite/expr/convert.rs +++ b/c2rust-analyze/src/rewrite/expr/convert.rs @@ -807,7 +807,10 @@ pub fn convert_cast_rewrite(kind: &mir_op::RewriteKind, hir_rw: Rewrite) -> Rewr // `p` -> `mem::replace(&mut p, Err(()))` Rewrite::Call( "std::mem::replace".to_string(), - vec![hir_rw, Rewrite::Text("Err(())".into())], + vec![ + Rewrite::Ref(Box::new(hir_rw), hir::Mutability::Mut), + Rewrite::Text("Err(())".into()), + ], ) } mir_op::RewriteKind::DynOwnedWrap => { diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index 37c1e36ad..c6e1e7837 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -448,7 +448,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { // `visit_place` directly with the desired access. v.enter_rvalue_operand(0, |v| { v.enter_operand_place(|v| { + eprintln!("BEGIN visit_place for ownership transfer"); v.visit_place(rv_pl, PlaceAccess::Mut); + eprintln!("END visit_place for ownership transfer"); }); }); // Obtain a reference to the place containing the `DynOwned` pointer. @@ -458,8 +460,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { deref: false, }); v.emit(RewriteKind::OptionMapBegin); - } else { - v.emit(RewriteKind::Ref { mutbl: true }); + v.emit(RewriteKind::Deref); } // Take the pointer out of that place. v.emit(RewriteKind::DynOwnedTake); From 92d4f7c750cdbdf9404cc08fb4dd7f1a4802ba9f Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Wed, 10 Jul 2024 15:08:29 -0700 Subject: [PATCH 07/11] analyze: mir_op: use as_ref/as_mut + Deref for dyn_owned option downgrades --- c2rust-analyze/src/rewrite/expr/mir_op.rs | 26 +++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index c6e1e7837..fda25cc68 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -1043,6 +1043,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { PlaceElem::Deref => { self.enter_place_deref_pointer(|v| { v.visit_place_ref(base_pl, proj_ltys, access); + let dyn_owned = v.is_dyn_owned(base_lty); if v.is_nullable(base_lty.label) { // If the pointer type is non-copy, downgrade (borrow) before calling // `unwrap()`. @@ -1054,12 +1055,15 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { if !desc.own.is_copy() { v.emit(RewriteKind::OptionDowngrade { mutbl: access == PlaceAccess::Mut, - deref: true, + deref: !dyn_owned, }); } v.emit(RewriteKind::OptionUnwrap); + if dyn_owned { + v.emit(RewriteKind::Deref); + } } - if v.is_dyn_owned(base_lty) { + if dyn_owned { v.emit(RewriteKind::DynOwnedDowngrade { mutbl: access == PlaceAccess::Mut, }); @@ -1303,6 +1307,12 @@ where return Ok(()); } + // To downgrade and unwrap a non-`dyn_owned` pointer, we call `p.as_deref().unwrap()`, + // which goes from `Option>` to `&T`. To downgrade a `dyn_owned` pointer, we + // instead do `*p.as_ref().unwrap()`, which goes from `Option>` to + // `DynOwned`, with the latter being a read-only `Place` that can be further downgraded + // to eventually reach `&T`. + let mut deref_after_unwrap = false; if from.option && from.own != to.own { // Downgrade ownership before unwrapping the `Option` when possible. This can avoid // moving/consuming the input. For example, if the `from` type is `Option>` and @@ -1315,15 +1325,17 @@ where Ownership::Raw | Ownership::Imm => { (self.emit)(RewriteKind::OptionDowngrade { mutbl: false, - deref: true, + deref: !from.dyn_owned, }); + deref_after_unwrap = from.dyn_owned; from.own = Ownership::Imm; } Ownership::RawMut | Ownership::Cell | Ownership::Mut => { (self.emit)(RewriteKind::OptionDowngrade { mutbl: true, - deref: true, + deref: !from.dyn_owned, }); + deref_after_unwrap = from.dyn_owned; from.own = Ownership::Mut; } Ownership::Rc if from.own == Ownership::Rc => { @@ -1346,6 +1358,9 @@ where if from.option && !to.option { // Unwrap first, then perform remaining casts. (self.emit)(RewriteKind::OptionUnwrap); + if deref_after_unwrap { + (self.emit)(RewriteKind::Deref); + } from.option = false; } else if from.option && to.option { trace!("try_build_cast_desc_desc: emit OptionMapBegin"); @@ -1363,6 +1378,9 @@ where ); } (self.emit)(RewriteKind::OptionMapBegin); + if deref_after_unwrap { + (self.emit)(RewriteKind::Deref); + } from.option = false; in_option_map = true; } From 323eef4760f4df5146f8c28ac9f569af5460b1f1 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Wed, 10 Jul 2024 15:37:17 -0700 Subject: [PATCH 08/11] analyze: add tests/filecheck/rewrite_nullable_box.rs --- c2rust-analyze/tests/filecheck.rs | 1 + .../tests/filecheck/rewrite_nullable_box.rs | 35 +++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 c2rust-analyze/tests/filecheck/rewrite_nullable_box.rs diff --git a/c2rust-analyze/tests/filecheck.rs b/c2rust-analyze/tests/filecheck.rs index 94672a386..bba56350b 100644 --- a/c2rust-analyze/tests/filecheck.rs +++ b/c2rust-analyze/tests/filecheck.rs @@ -63,6 +63,7 @@ define_tests! { pointee, ptrptr1, regions_fixed, + rewrite_nullable_box, rewrite_paths, rewrite_paths_manual_shim, statics, diff --git a/c2rust-analyze/tests/filecheck/rewrite_nullable_box.rs b/c2rust-analyze/tests/filecheck/rewrite_nullable_box.rs new file mode 100644 index 000000000..8c8ec09d8 --- /dev/null +++ b/c2rust-analyze/tests/filecheck/rewrite_nullable_box.rs @@ -0,0 +1,35 @@ +//! Test cases for rewriting of nullable owned pointers. +#![feature(rustc_private)] +#![feature(register_tool)] +#![register_tool(c2rust_analyze_test)] + +use std::mem; +extern crate libc; + +extern "C" { + fn malloc(_: libc::c_ulong) -> *mut libc::c_void; + fn free(__ptr: *mut libc::c_void); +} + +#[c2rust_analyze_test::skip_borrowck] +// CHECK-LABEL: fn test_is_null( +pub unsafe fn test_is_null(cond: bool) { + // CHECK: let mut ptr: core::option::Option,()>> = + let mut ptr: *mut i32 = malloc(4) as *mut i32; + // This assignment exercises the option + dyn_owned case in `visit_place_ref`. + // CHECK: *(*(ptr).as_mut().unwrap()).as_deref_mut().unwrap() = 1; + *ptr = 1; + if cond { + ptr = 0 as *mut i32; + } + // This condition exercises the option + dyn_owned case in `try_build_cast_desc_desc`. + // CHECK: if !(ptr).as_ref().map(|__ptr| (*__ptr).as_deref().unwrap()).is_none() { + if !ptr.is_null() { + // CHECK: *(*(ptr).as_mut().unwrap()).as_deref_mut().unwrap() = 2; + *ptr = 2; + } + // This condition exercises the option + dyn_owned case in the `assignment_transfers_ownership` + // case of `visit_statement`. + // CHECK: std::mem::drop(((ptr).as_mut().map(|__ptr| std::mem::replace(&mut *__ptr,Err(()))).map(|__ptr| __ptr.unwrap()))); + free(ptr as *mut libc::c_void); +} From ac3c95b465ed2318894cb3cbb542bf79c5bc737c Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Wed, 10 Jul 2024 15:50:48 -0700 Subject: [PATCH 09/11] analyze: mir_op: trace! reasons for not rewriting malloc/calloc --- c2rust-analyze/src/rewrite/expr/mir_op.rs | 31 +++++++++++++++++++---- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index fda25cc68..9d9569edd 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -671,7 +671,10 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { let pointee_lty = match dest_pointee { Some(x) => x, // TODO: emit void* cast before bailing out - None => return, + None => { + trace!("{callee:?}: no pointee type for dest"); + return; + } }; let orig_pointee_ty = pointee_lty.ty; @@ -687,7 +690,13 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { let zero_ty = match ZeroizeType::from_ty(tcx, orig_pointee_ty) { Some(x) => x, // TODO: emit void* cast before bailing out - None => return, + None => { + trace!( + "{callee:?}: failed to compute ZeroizeType \ + for {orig_pointee_ty:?}" + ); + return; + } }; let rw = match *callee { @@ -757,7 +766,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { }); } - Callee::Realloc => { + ref callee @ Callee::Realloc => { self.enter_rvalue(|v| { let src_lty = v.acx.type_of(&args[0]); let src_pointee = v.pointee_lty(src_lty); @@ -767,7 +776,13 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { let pointee_lty = match common_pointee { Some(x) => x, // TODO: emit void* cast before bailing out - None => return, + None => { + trace!( + "{callee:?}: no common pointee type \ + between {src_pointee:?} and {dest_pointee:?}" + ); + return; + } }; let orig_pointee_ty = pointee_lty.ty; @@ -785,7 +800,13 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { let zero_ty = match ZeroizeType::from_ty(tcx, orig_pointee_ty) { Some(x) => x, // TODO: emit void* cast before bailing out - None => return, + None => { + trace!( + "{callee:?}: failed to compute ZeroizeType \ + for {orig_pointee_ty:?}" + ); + return; + } }; // Cast input to either `Box` or `Box<[T]>`, as in `free`. From 904ae1e2b0ceb3d4bcc41348941fa1a49082eaf3 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Wed, 10 Jul 2024 16:20:28 -0700 Subject: [PATCH 10/11] analyze: rewrite: handle realloc(NULL, ...) --- c2rust-analyze/src/rewrite/expr/convert.rs | 31 ++++++++++--- c2rust-analyze/src/rewrite/expr/mir_op.rs | 53 ++++++++++++++-------- 2 files changed, 58 insertions(+), 26 deletions(-) diff --git a/c2rust-analyze/src/rewrite/expr/convert.rs b/c2rust-analyze/src/rewrite/expr/convert.rs index ff5fec82c..e2857e97d 100644 --- a/c2rust-analyze/src/rewrite/expr/convert.rs +++ b/c2rust-analyze/src/rewrite/expr/convert.rs @@ -345,6 +345,7 @@ impl<'tcx> ConvertVisitor<'tcx> { elem_size, src_single, dest_single, + option, } => { // `realloc(p, n)` -> `Box::new(...)` assert!(matches!(hir_rw, Rewrite::Identity)); @@ -364,9 +365,14 @@ impl<'tcx> ConvertVisitor<'tcx> { } let expr = match (src_single, dest_single) { (false, false) => { + let src = if option { + "src_ptr.unwrap_or(Box::new([]))" + } else { + "src_ptr" + }; stmts.push(Rewrite::Let1( "mut dest_ptr".into(), - Box::new(Rewrite::Text("Vec::from(src_ptr)".into())), + Box::new(format_rewrite!("Vec::from({src})")), )); stmts.push(format_rewrite!( "dest_ptr.resize_with(dest_n, || {})", @@ -375,8 +381,9 @@ impl<'tcx> ConvertVisitor<'tcx> { Rewrite::Text("dest_ptr.into_boxed_slice()".into()) } (false, true) => { + let opt_flatten = if option { ".flatten()" } else { "" }; format_rewrite!( - "src_ptr.into_iter().next().unwrap_or_else(|| {})", + "src_ptr.into_iter(){opt_flatten}.next().unwrap_or_else(|| {})", zeroize_expr ) } @@ -385,16 +392,28 @@ impl<'tcx> ConvertVisitor<'tcx> { "mut dest_ptr".into(), Box::new(Rewrite::Text("Vec::with_capacity(dest_n)".into())), )); - stmts.push(Rewrite::Text( - "if dest_n >= 1 { dest_ptr.push(*src_ptr); }".into(), - )); + if option { + stmts.push(Rewrite::Text( + "if dest_n >= 1 { if let Some(src) = src_ptr { dest_ptr.push(*src); } }".into(), + )); + } else { + stmts.push(Rewrite::Text( + "if dest_n >= 1 { dest_ptr.push(*src_ptr); }".into(), + )); + } stmts.push(format_rewrite!( "dest_ptr.resize_with(dest_n, || {})", zeroize_expr, )); Rewrite::Text("dest_ptr.into_boxed_slice()".into()) } - (true, true) => Rewrite::Text("src_ptr".into()), + (true, true) => { + if option { + format_rewrite!("src_ptr.unwrap_or_else(|| Box::new({}))", zeroize_expr) + } else { + Rewrite::Text("src_ptr".into()) + } + } }; Rewrite::Block(stmts, Some(Box::new(expr))) } diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index 9d9569edd..16d673790 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -111,6 +111,7 @@ pub enum RewriteKind { elem_size: u64, src_single: bool, dest_single: bool, + option: bool, }, CallocSafe { zero_ty: ZeroizeType, @@ -810,17 +811,25 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { }; // Cast input to either `Box` or `Box<[T]>`, as in `free`. + let mut option = false; v.enter_call_arg(0, |v| { - v.emit_cast_lty_adjust(src_lty, |desc| TypeDesc { - own: Ownership::Box, - qty: if src_single { - Quantity::Single - } else { - Quantity::Slice - }, - dyn_owned: false, - option: desc.option, - pointee_ty: desc.pointee_ty, + v.emit_cast_lty_adjust(src_lty, |desc| { + // `realloc(NULL, ...)` is explicitly allowed by the spec, so + // we can't force an unwrap here by returning `option: false`. + // Instead, we record the `option` flag as part of the rewrite + // so the nullable case can be handled appropriately. + option = desc.option; + TypeDesc { + own: Ownership::Box, + qty: if src_single { + Quantity::Single + } else { + Quantity::Slice + }, + dyn_owned: false, + option: desc.option, + pointee_ty: desc.pointee_ty, + } }); }); @@ -829,21 +838,25 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { elem_size, src_single, dest_single, + option, }); // Cast output from `Box`/`Box<[T]>` to the target type, as in // `malloc`. v.emit_cast_adjust_lty( - |desc| TypeDesc { - own: Ownership::Box, - qty: if dest_single { - Quantity::Single - } else { - Quantity::Slice - }, - dyn_owned: false, - option: false, - pointee_ty: desc.pointee_ty, + |desc| { + TypeDesc { + own: Ownership::Box, + qty: if dest_single { + Quantity::Single + } else { + Quantity::Slice + }, + dyn_owned: false, + // We always return non-null from `realloc`. + option: false, + pointee_ty: desc.pointee_ty, + } }, dest_lty, ); From 14cf393af9d727a1d942219ab2bed761d7f0b76a Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Thu, 11 Jul 2024 11:04:45 -0700 Subject: [PATCH 11/11] analyze: mir_op: support nullable pointers in ZeroizeType --- c2rust-analyze/src/rewrite/expr/convert.rs | 2 + c2rust-analyze/src/rewrite/expr/mir_op.rs | 68 ++++++++++++++++++++-- 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/c2rust-analyze/src/rewrite/expr/convert.rs b/c2rust-analyze/src/rewrite/expr/convert.rs index e2857e97d..6ee0b0bde 100644 --- a/c2rust-analyze/src/rewrite/expr/convert.rs +++ b/c2rust-analyze/src/rewrite/expr/convert.rs @@ -698,6 +698,7 @@ fn generate_zeroize_code(zero_ty: &ZeroizeType, lv: &str) -> String { match *zero_ty { ZeroizeType::Int => format!("{lv} = 0"), ZeroizeType::Bool => format!("{lv} = false"), + ZeroizeType::Option => format!("{lv} = None"), ZeroizeType::Array(ref elem_zero_ty) => format!( " {{ @@ -731,6 +732,7 @@ fn generate_zeroize_expr(zero_ty: &ZeroizeType) -> String { match *zero_ty { ZeroizeType::Int => format!("0"), ZeroizeType::Bool => format!("false"), + ZeroizeType::Option => format!("None"), ZeroizeType::Array(ref elem_zero_ty) => format!( "std::array::from_fn(|| {})", generate_zeroize_expr(elem_zero_ty) diff --git a/c2rust-analyze/src/rewrite/expr/mir_op.rs b/c2rust-analyze/src/rewrite/expr/mir_op.rs index 16d673790..6db229a59 100644 --- a/c2rust-analyze/src/rewrite/expr/mir_op.rs +++ b/c2rust-analyze/src/rewrite/expr/mir_op.rs @@ -176,6 +176,8 @@ pub enum ZeroizeType { Int, /// Zeroize by storing the literal `false`. Bool, + /// Zeroize by storing `None`. + Option, /// Iterate over `x.iter_mut()` and zeroize each element. Array(Box), /// Zeroize each named field. @@ -686,15 +688,15 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> { let single = !v.perms[dest_lty.label] .intersects(PermissionSet::OFFSET_ADD | PermissionSet::OFFSET_SUB); - // TODO: use rewritten types here, so that the `ZeroizeType` will - // reflect the actual types and fields after rewriting. - let zero_ty = match ZeroizeType::from_ty(tcx, orig_pointee_ty) { + let opt_zero_ty = + ZeroizeType::from_lty(&v.acx, &v.perms, &v.flags, pointee_lty); + let zero_ty = match opt_zero_ty { Some(x) => x, // TODO: emit void* cast before bailing out None => { trace!( "{callee:?}: failed to compute ZeroizeType \ - for {orig_pointee_ty:?}" + for {pointee_lty:?}" ); return; } @@ -1278,6 +1280,64 @@ impl ZeroizeType { _ => return None, }) } + + fn from_lty<'tcx>( + acx: &AnalysisCtxt<'_, 'tcx>, + perms: &PointerTable<'_, PermissionSet>, + flags: &PointerTable<'_, FlagSet>, + lty: LTy<'tcx>, + ) -> Option { + Some(match *lty.kind() { + TyKind::Int(_) | TyKind::Uint(_) => ZeroizeType::Int, + TyKind::Bool => ZeroizeType::Bool, + TyKind::Adt(adt_def, substs) => { + if !adt_def.is_struct() { + eprintln!("ZeroizeType::from_lty({lty:?}): not a struct"); + return None; + } + let variant = adt_def.non_enum_variant(); + let mut fields = Vec::with_capacity(variant.fields.len()); + for (field_idx, field) in variant.fields.iter().enumerate() { + let name = field.name.to_string(); + let ty = field.ty(acx.tcx(), substs); + let lty = acx.projection_lty(lty, &PlaceElem::Field(field_idx.into(), ty)); + let zero = ZeroizeType::from_lty(acx, perms, flags, lty)?; + fields.push((name, zero)); + } + + let name_printer = FmtPrinter::new(acx.tcx(), Namespace::ValueNS); + let name = name_printer + .print_value_path(adt_def.did(), &[]) + .unwrap() + .into_buffer(); + + ZeroizeType::Struct(name, fields) + } + TyKind::Array(_, _) => { + let elem_zero = ZeroizeType::from_lty(acx, perms, flags, lty.args[0])?; + ZeroizeType::Array(Box::new(elem_zero)) + } + TyKind::Ref(..) | TyKind::RawPtr(..) => { + if lty.label.is_none() { + eprintln!("ZeroizeType::from_lty({lty:?}): ptr has no label"); + return None; + } + let ptr = lty.label; + if flags[ptr].contains(FlagSet::FIXED) { + eprintln!("ZeroizeType::from_lty({lty:?}): ptr is FIXED"); + return None; + } + let nullable = !perms[ptr].contains(PermissionSet::NON_NULL); + if nullable { + ZeroizeType::Option + } else { + eprintln!("ZeroizeType::from_lty({lty:?}): ptr is NON_NULL"); + return None; + } + } + _ => return None, + }) + } } pub struct CastBuilder<'a, 'tcx, PT1, PT2, F> {