analyze: misc fixes for lighttpd buffer (part 1/2) (#1178)

Various small fixes for lighttpd's `buffer` module:

* In `rewrite::expr::mir_op`, skip generating casts for `Deref`
projections on `FIXED` pointers
* In borrowck, don't treat `StorageLive`/`StorageDead` as uses (which
would invalidate loans)
* In `rewrite::ty::HirTyVisitor`, add a missing `walk_stmt` call. This
was causing type rewrites to be skipped in some places.
* In `rewrite::expr::mir_op`, add a conversion from `&[T]` to `&T`
before dereferencing if needed. This can happen in cases like
`p.offset(...); x = *p;` because `*p` uses `p` directly, which has the
`OFFSET` permission due to the previous `p.offset()` call, instead of
copying `p` into a temporary, which would not need the `OFFSET`
permission.
* In `GlobalAnalysisCtxt::fn_analysis_invalid`, ignore the flag
`DontRewriteFnReason::REWRITE_INVALID`, since rewriting errors don't
affect the validity of the analysis results.

c2rust-analyze/src/borrowck/def_use.rs

@@ -34,12 +34,31 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {
         // path and not the unwind path. -nmatsakis
         PlaceContext::MutatingUse(MutatingUseContext::Call) |
         PlaceContext::MutatingUse(MutatingUseContext::AsmOutput) |
-        PlaceContext::MutatingUse(MutatingUseContext::Yield) |
+        PlaceContext::MutatingUse(MutatingUseContext::Yield) => Some(DefUse::Def),
 
         // Storage live and storage dead aren't proper defines, but we can ignore
         // values that come before them.
+        //
+        // C2Rust: For borrowchecking purposes, we ignore `StorageLive` and `StorageDead`.  In the
+        // original version of this function, they're considered to be `DefUse::Def`s, but this
+        // approach creates a problem for code like this:
+        //
+        // ```
+        // let q = {
+        //   let p = &mut ...;
+        //   p
+        // };
+        // *q = 1;
+        // ```
+        //
+        // The MIR for this has an assignment like `q = p`, followed by `StorageDead(p)`.  We
+        // interpret the assignment as a reborrow of `p`, and if `StorageDead(p)` was considered a
+        // `Def`, we would invalidate the loan at the end of the block when `StorageDead` "writes"
+        // to `p`.  However, this code is perfectly valid, and omitting `loan_invalidated_at` for
+        // `StorageLive` and `StorageDead` appears to be consistent with `rustc -Z nll-facts`
+        // output (tested on `tests/filecheck/move_mut.rs`).
         PlaceContext::NonUse(NonUseContext::StorageLive) |
-        PlaceContext::NonUse(NonUseContext::StorageDead) => Some(DefUse::Def),
+        PlaceContext::NonUse(NonUseContext::StorageDead) => None,
 
         ///////////////////////////////////////////////////////////////////////////
         // REGULAR USES

c2rust-analyze/src/context.rs

@@ -245,7 +245,6 @@ bitflags! {
             | Self::DATAFLOW_INVALID.bits
             | Self::BORROWCK_INVALID.bits
             | Self::MISC_ANALYSIS_INVALID.bits
-            | Self::REWRITE_INVALID.bits
             | Self::FAKE_INVALID_FOR_TESTING.bits;
     }
 }

c2rust-analyze/src/rewrite/expr/mir_op.rs

@@ -220,6 +220,21 @@ impl PlaceAccess {
     }
 }
 
+/// Named boolean for use with `visit_place`.  `RequireSinglePointer::Yes` means that if the
+/// `Place` ends with a `Deref` projection, the pointer being dereferenced must have
+/// `Quantity::Single`.
+#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
+enum RequireSinglePointer {
+    No,
+    Yes,
+}
+
+impl RequireSinglePointer {
+    pub fn as_bool(self) -> bool {
+        self == RequireSinglePointer::Yes
+    }
+}
+
 struct ExprRewriteVisitor<'a, 'tcx> {
     acx: &'a AnalysisCtxt<'a, 'tcx>,
     perms: &'a GlobalPointerTable<PermissionSet>,
@@ -475,7 +490,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
                         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);
+                                v.visit_place(rv_pl, PlaceAccess::Mut, RequireSinglePointer::No);
                                 eprintln!("END visit_place for ownership transfer");
                             });
                         });
@@ -501,7 +516,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
                     v.visit_rvalue(rv, Some(rv_lty));
                     v.emit_cast_lty_lty(rv_lty, pl_lty)
                 });
-                self.enter_dest(|v| v.visit_place(pl, PlaceAccess::Mut));
+                self.enter_dest(|v| v.visit_place(pl, PlaceAccess::Mut, RequireSinglePointer::Yes));
             }
             StatementKind::FakeRead(..) => {}
             StatementKind::SetDiscriminant { .. } => todo!("statement {:?}", stmt),
@@ -905,7 +920,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
                     BorrowKind::Mut { .. } => true,
                     BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Unique => false,
                 };
-                self.enter_rvalue_place(0, |v| v.visit_place(pl, PlaceAccess::from_bool(mutbl)));
+                self.enter_rvalue_place(0, |v| {
+                    v.visit_place(pl, PlaceAccess::from_bool(mutbl), RequireSinglePointer::No)
+                });
 
                 if let Some(expect_ty) = expect_ty {
                     if self.is_nullable(expect_ty.label) {
@@ -919,7 +936,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
                 // TODO
             }
             Rvalue::AddressOf(mutbl, pl) => {
-                self.enter_rvalue_place(0, |v| v.visit_place(pl, PlaceAccess::from_mutbl(mutbl)));
+                self.enter_rvalue_place(0, |v| {
+                    v.visit_place(pl, PlaceAccess::from_mutbl(mutbl), RequireSinglePointer::No)
+                });
                 if let Some(expect_ty) = expect_ty {
                     let desc = type_desc::perms_to_desc_with_pointee(
                         self.acx.tcx(),
@@ -941,7 +960,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
                 }
             }
             Rvalue::Len(pl) => {
-                self.enter_rvalue_place(0, |v| v.visit_place(pl, PlaceAccess::Imm));
+                self.enter_rvalue_place(0, |v| {
+                    v.visit_place(pl, PlaceAccess::Imm, RequireSinglePointer::No)
+                });
             }
             Rvalue::Cast(_kind, ref op, ty) => {
                 if util::is_null_const_operand(op) && ty.is_unsafe_ptr() {
@@ -1000,7 +1021,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
                 self.enter_rvalue_operand(0, |v| v.visit_operand(op, None));
             }
             Rvalue::Discriminant(pl) => {
-                self.enter_rvalue_place(0, |v| v.visit_place(pl, PlaceAccess::Imm));
+                self.enter_rvalue_place(0, |v| {
+                    v.visit_place(pl, PlaceAccess::Imm, RequireSinglePointer::No)
+                });
             }
             Rvalue::Aggregate(ref _kind, ref ops) => {
                 for (i, op) in ops.iter().enumerate() {
@@ -1011,7 +1034,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
                 self.enter_rvalue_operand(0, |v| v.visit_operand(op, None));
             }
             Rvalue::CopyForDeref(pl) => {
-                self.enter_rvalue_place(0, |v| v.visit_place(pl, PlaceAccess::Imm));
+                self.enter_rvalue_place(0, |v| {
+                    v.visit_place(pl, PlaceAccess::Imm, RequireSinglePointer::No)
+                });
             }
         }
     }
@@ -1022,7 +1047,9 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
         match *op {
             Operand::Copy(pl) | Operand::Move(pl) => {
                 // TODO: should this be Move, Imm, or dependent on the type?
-                self.enter_operand_place(|v| v.visit_place(pl, PlaceAccess::Move));
+                self.enter_operand_place(|v| {
+                    v.visit_place(pl, PlaceAccess::Move, RequireSinglePointer::Yes)
+                });
 
                 if let Some(expect_ty) = expect_ty {
                     let ptr_lty = self.acx.type_of(pl);
@@ -1040,7 +1067,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
         match *op {
             Operand::Copy(pl) | Operand::Move(pl) => {
                 // TODO: should this be Move, Imm, or dependent on the type?
-                self.visit_place(pl, PlaceAccess::Move);
+                self.visit_place(pl, PlaceAccess::Move, RequireSinglePointer::Yes);
 
                 let ptr_lty = self.acx.type_of(pl);
                 if !ptr_lty.label.is_none() {
@@ -1051,14 +1078,19 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
         }
     }
 
-    fn visit_place(&mut self, pl: Place<'tcx>, access: PlaceAccess) {
+    fn visit_place(
+        &mut self,
+        pl: Place<'tcx>,
+        access: PlaceAccess,
+        require_single_ptr: RequireSinglePointer,
+    ) {
         let mut ltys = Vec::with_capacity(1 + pl.projection.len());
         ltys.push(self.acx.type_of(pl.local));
         for proj in pl.projection {
             let prev_lty = ltys.last().copied().unwrap();
             ltys.push(self.acx.projection_lty(prev_lty, &proj));
         }
-        self.visit_place_ref(pl.as_ref(), &ltys, access);
+        self.visit_place_ref(pl.as_ref(), &ltys, access, require_single_ptr);
     }
 
     /// Generate rewrites for a `Place` represented as a `PlaceRef`.  `proj_ltys` gives the `LTy`
@@ -1069,6 +1101,7 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
         pl: PlaceRef<'tcx>,
         proj_ltys: &[LTy<'tcx>],
         access: PlaceAccess,
+        require_single_ptr: RequireSinglePointer,
     ) {
         let (&last_proj, rest) = match pl.projection.split_last() {
             Some(x) => x,
@@ -1090,39 +1123,53 @@ impl<'a, 'tcx> ExprRewriteVisitor<'a, 'tcx> {
         match last_proj {
             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()`.
+                    v.visit_place_ref(base_pl, proj_ltys, access, RequireSinglePointer::Yes);
+                    if !v.flags[base_lty.label].contains(FlagSet::FIXED) {
                         let desc = type_desc::perms_to_desc(
                             base_lty.ty,
                             v.perms[base_lty.label],
                             v.flags[base_lty.label],
                         );
-                        if !desc.own.is_copy() {
-                            v.emit(RewriteKind::OptionDowngrade {
+                        // TODO: This logic is quite similar to the cast builder code and could
+                        // probably be replaced with a call to `emit_cast_lty_adjust`.  But
+                        // currently this tries to introduce casts on the borrow projection in
+                        // `array.as_mut_ptr()`, which causes rewriting to fail.
+                        if v.is_nullable(base_lty.label) {
+                            // If the pointer type is non-copy, downgrade (borrow) before calling
+                            // `unwrap()`.
+                            if !desc.own.is_copy() {
+                                v.emit(RewriteKind::OptionDowngrade {
+                                    mutbl: access == PlaceAccess::Mut,
+                                    deref: !desc.dyn_owned,
+                                });
+                            }
+                            v.emit(RewriteKind::OptionUnwrap);
+                            if desc.dyn_owned {
+                                v.emit(RewriteKind::Deref);
+                            }
+                        }
+                        if desc.dyn_owned {
+                            v.emit(RewriteKind::DynOwnedDowngrade {
                                 mutbl: access == PlaceAccess::Mut,
-                                deref: !dyn_owned,
                             });
                         }
-                        v.emit(RewriteKind::OptionUnwrap);
-                        if dyn_owned {
-                            v.emit(RewriteKind::Deref);
+                        if require_single_ptr.as_bool() && desc.qty != Quantity::Single {
+                            v.emit(RewriteKind::SliceFirst {
+                                mutbl: access == PlaceAccess::Mut,
+                            });
                         }
                     }
-                    if dyn_owned {
-                        v.emit(RewriteKind::DynOwnedDowngrade {
-                            mutbl: access == PlaceAccess::Mut,
-                        });
-                    }
                 });
             }
             PlaceElem::Field(_idx, _ty) => {
-                self.enter_place_field_base(|v| v.visit_place_ref(base_pl, proj_ltys, access));
+                self.enter_place_field_base(|v| {
+                    v.visit_place_ref(base_pl, proj_ltys, access, RequireSinglePointer::Yes)
+                });
             }
             PlaceElem::Index(_) | PlaceElem::ConstantIndex { .. } | PlaceElem::Subslice { .. } => {
-                self.enter_place_index_array(|v| v.visit_place_ref(base_pl, proj_ltys, access));
+                self.enter_place_index_array(|v| {
+                    v.visit_place_ref(base_pl, proj_ltys, access, RequireSinglePointer::Yes)
+                });
             }
             PlaceElem::Downcast(_, _) => {}
         }

c2rust-analyze/src/rewrite/ty.rs

@@ -681,6 +681,7 @@ impl<'tcx, 'a> intravisit::Visitor<'tcx> for HirTyVisitor<'a, 'tcx> {
             }
             _ => (),
         }
+        intravisit::walk_stmt(self, s);
     }
 }
 

c2rust-analyze/tests/filecheck.rs

@@ -55,11 +55,13 @@ define_tests! {
     insertion_sort_driver,
     insertion_sort_rewrites,
     known_fn,
+    move_mut,
     non_null,
     non_null_force,
     non_null_rewrites,
     offset1,
     offset2,
+    offset_rewrites,
     pointee,
     ptrptr1,
     regions_fixed,

c2rust-analyze/tests/filecheck/alloc.rs

@@ -96,6 +96,24 @@ unsafe extern "C" fn realloc1(n: libc::c_ulong) {
     free(buf as *mut libc::c_void);
 }
 
+
+// CHECK-LABEL: final labeling for "malloc_return"
+pub unsafe extern "C" fn malloc_return(mut cnt: libc::c_int) -> *mut i32 {
+    // CHECK-DAG: ([[@LINE+1]]: mut i): addr_of = UNIQUE | NON_NULL | STACK, type = READ | WRITE | UNIQUE | FREE | NON_NULL | HEAP#
+    let mut i: *mut i32 = malloc(::std::mem::size_of::<i32>() as libc::c_ulong) as *mut i32;
+    i
+}
+
+// CHECK-LABEL: final labeling for "malloc_return_free1"
+pub unsafe extern "C" fn malloc_return_free1(mut cnt: libc::c_int) {
+    // CHECK-DAG: ([[@LINE+1]]: mut i): addr_of = UNIQUE | NON_NULL | STACK, type = READ | WRITE | UNIQUE | FREE | NON_NULL | HEAP#
+    let mut i: *mut i32 = malloc_return(cnt);
+    *i = 2;
+    // CHECK-DAG: ([[@LINE+1]]: i{{.*}}): {{.*}}type = UNIQUE | FREE | NON_NULL | HEAP#
+    free(i as *mut libc::c_void);
+}
+
+
 // Rewrites of malloc/calloc/realloc/memset should use `mem::size_of` to convert byte counts to
 // element counts.
 // CHECK: let n = byte_len as usize / std::mem::size_of::<i32>();

c2rust-analyze/tests/filecheck/move_mut.rs

@@ -0,0 +1,30 @@
+use std::ptr;
+
+// CHECK-LABEL: final labeling for "move_mut1"
+pub unsafe fn move_mut1(mut x: i32) -> i32 {
+    // CHECK-DAG: ([[@LINE+1]]: q): addr_of = UNIQUE | NON_NULL | STACK, type = READ | WRITE | UNIQUE | NON_NULL | STACK#
+    let q: *mut i32 = {
+        // CHECK-DAG: ([[@LINE+1]]: p): addr_of = UNIQUE | NON_NULL | STACK, type = READ | WRITE | UNIQUE | NON_NULL | STACK#
+        let p: *mut i32 = ptr::addr_of_mut!(x);
+        *p = 1;
+        // This produces a move from `p` into `q`.  c2rust-analyze interprets this as a reborrow,
+        // which gets invalidated at the end of `p`'s lifetime.  Currently we work around this by
+        // ignoring `StorageLive` and `StorageDead`, so they don't invalidate any borrows.
+        p
+    };
+    *q = 2;
+    *q
+}
+
+// Safe version of `move_mut1`.  This can be run through `rustc -Z nll-facts` to produce Polonius
+// facts from the reference implementation for comparison.
+// CHECK-LABEL: final labeling for "move_mut1_safe"
+pub fn move_mut1_safe(mut x: i32) -> i32 {
+    let q: &mut i32 = {
+        let p: &mut i32 = &mut x;
+        *p = 1;
+        p
+    };
+    *q = 2;
+    *q
+}

c2rust-analyze/tests/filecheck/non_null_force.rs

@@ -1,40 +1,37 @@
 #![feature(register_tool)]
 #![register_tool(c2rust_analyze_test)]
 
-// TODO: All the pointers here are currently inferred to be non-`UNIQUE`, even though the access
-// patterns look fine.
-
 use std::ptr;
 
 // CHECK-LABEL: final labeling for "f"
 fn f(cond: bool) {
     let x = 1_i32;
-    // CHECK: ([[@LINE+1]]: mut y): {{.*}}, type = STACK#
+    // CHECK: ([[@LINE+1]]: mut y): {{.*}}, type = UNIQUE | STACK#
     let mut y = ptr::addr_of!(x);
     if cond {
         y = 0 as *const _;
     }
     // The expression `y` is considered nullable even though it's passed for argument `p` of `g`,
     // which is forced to be `NON_NULL`.
-    // CHECK: ([[@LINE+1]]: y): {{.*}}, type = STACK#
+    // CHECK: ([[@LINE+1]]: y): {{.*}}, type = UNIQUE | STACK#
     g(cond, y);
 }
 
 // CHECK-LABEL: final labeling for "g"
 // `p` should be non-null, as it's forced to be by the attribute.  This emulates the "unsound" PDG
 // case, where a variable is forced to stay `NON_NULL` even though a null possibly flows into it.
-// CHECK: ([[@LINE+2]]: p): {{.*}}, type = NON_NULL | STACK#
+// CHECK: ([[@LINE+2]]: p): {{.*}}, type = UNIQUE | NON_NULL | STACK#
 #[c2rust_analyze_test::force_non_null_args]
 fn g(cond: bool, p: *const i32) {
     // `q` is not forced to be `NON_NULL`, so it should be inferred nullable due to the null
     // assignment below.
-    // CHECK: ([[@LINE+1]]: mut q): {{.*}}, type = STACK#
+    // CHECK: ([[@LINE+1]]: mut q): {{.*}}, type = UNIQUE | STACK#
     let mut q = p;
     if cond {
         q = 0 as *const _;
     }
     // `r` is derived from `q` (and is not forced), so it should also be nullable.
-    // CHECK: ([[@LINE+1]]: r): {{.*}}, type = STACK#
+    // CHECK: ([[@LINE+1]]: r): {{.*}}, type = UNIQUE | STACK#
     let r = q;
 }
 

c2rust-analyze/tests/filecheck/offset_rewrites.rs

@@ -0,0 +1,12 @@
+
+// Test dereferencing a pointer that has OFFSET permissions.  This requires inserting a cast from
+// `&mut [T]` to `&mut T`.
+// CHECK-LABEL: fn offset_deref
+pub unsafe fn offset_deref(x: *mut i32, off: isize) -> *mut i32 {
+    // CHECK: *&mut (x)[0] = 0;
+    *x = 0;
+    // CHECK: *&mut (&mut (x)[((1) as usize) ..])[0] = 1;
+    *x.offset(1) = 1;
+    // CHECK: {{.*}}x{{.*}}
+    x
+}