Bugfix Stack Register Computation (#135)

cwe_checker_rs/src/abstract_domain/bitvector.rs

@@ -342,10 +342,10 @@ impl std::convert::TryFrom<&BitvectorDomain> for Bitvector {
 impl std::fmt::Display for BitvectorDomain {
     fn fmt(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
         match self {
-            Self::Top(bytesize) => write!(formatter, "Top:i{}", BitSize::from(*bytesize)),
+            Self::Top(bytesize) => write!(formatter, "Top:u{}", BitSize::from(*bytesize)),
             Self::Value(bitvector) => write!(
                 formatter,
-                "0x{:016x}:i{:?}",
+                "0x{:016x}:u{:?}",
                 bitvector,
                 bitvector.width().to_usize()
             ),

cwe_checker_rs/src/analysis/pointer_inference/context/mod.rs

@@ -87,12 +87,13 @@ impl<'a> Context<'a> {
     fn detect_stack_pointer_information_loss_on_return(
         &self,
         state_before_return: &State,
-        return_term: &Term<Jmp>,
-    ) {
+    ) -> Result<(), Error> {
         let expected_stack_pointer_offset = match self.project.cpu_architecture.as_str() {
-            "x86" | "x86_64" => Bitvector::from_u64(u64::from(self.project.get_pointer_bytesize()))
+            "x86" | "x86_32" | "x86_64" => {
+                Bitvector::from_u64(u64::from(self.project.get_pointer_bytesize()))
                     .into_truncate(apint::BitWidth::from(self.project.get_pointer_bytesize()))
-                .unwrap(),
+                    .unwrap()
+            }
             _ => Bitvector::zero(apint::BitWidth::from(self.project.get_pointer_bytesize())),
         };
         match state_before_return.get_register(&self.project.stack_pointer_register) {
@@ -102,26 +103,21 @@ impl<'a> Context<'a> {
                     if *id != state_before_return.stack_id
                         || *offset != expected_stack_pointer_offset.into()
                     {
-                        self.log_debug(
-                            Err(anyhow!(
-                                "Unexpected stack register value at return instruction"
-                            )),
-                            Some(&return_term.tid),
-                        );
+                        Err(anyhow!("Unexpected stack register value on return"))
+                    } else {
+                        Ok(())
                     }
+                } else {
+                    Err(anyhow!(
+                        "Unexpected number of stack register targets on return"
+                    ))
                 }
             }
-            Ok(Data::Top(_)) => self.log_debug(
-                Err(anyhow!(
+            Ok(Data::Top(_)) => Err(anyhow!(
                 "Stack register value lost during function execution"
             )),
-                Some(&return_term.tid),
-            ),
-            Ok(Data::Value(_)) => self.log_debug(
-                Err(anyhow!("Unexpected stack register value on return")),
-                Some(&return_term.tid),
-            ),
-            Err(err) => self.log_debug(Err(err), Some(&return_term.tid)),
+            Ok(Data::Value(_)) => Err(anyhow!("Unexpected stack register value on return")),
+            Err(err) => Err(err),
         }
     }
 

cwe_checker_rs/src/analysis/pointer_inference/context/tests.rs

@@ -163,6 +163,22 @@ fn context_problem_implementation() {
             Data::Value(bv(33).into()),
         )
         .unwrap();
+    // Emulate  removing the return pointer from the stack for x64
+    let stack_pointer_update_def = Term {
+        tid: Tid::new("stack_pointer_update_def"),
+        term: Def::Assign {
+            var: register("RSP"),
+            value: BinOp {
+                op: BinOpType::IntAdd,
+                lhs: Box::new(Var(register("RSP"))),
+                rhs: Box::new(Const(Bitvector::from_i64(8))),
+            },
+        },
+    };
+    callee_state = context
+        .update_def(&callee_state, &stack_pointer_update_def)
+        .unwrap();
+    // Test update_return
     let return_state = context
         .update_return(
             Some(&callee_state),
@@ -176,7 +192,10 @@ fn context_problem_implementation() {
     assert_eq!(return_state.memory, state.memory);
     assert_eq!(
         return_state.get_register(&register("RSP")).unwrap(),
-        state.get_register(&register("RSP")).unwrap()
+        state
+            .get_register(&register("RSP"))
+            .unwrap()
+            .bin_op(BinOpType::IntAdd, &Bitvector::from_i64(8).into())
     );
 
     state.set_register(&register("callee_saved_reg"), Data::Value(bv(13)));

cwe_checker_rs/src/analysis/pointer_inference/context/trait_impls.rs

@@ -190,7 +190,13 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
         let stack_offset_on_call = self.get_current_stack_offset(state_before_call);
 
         // Detect possible information loss on the stack pointer and report it.
-        self.detect_stack_pointer_information_loss_on_return(state_before_return, return_term);
+        if let Err(err) = self.detect_stack_pointer_information_loss_on_return(state_before_return)
+        {
+            self.log_debug(Err(err), Some(&return_term.tid));
+            // This is an indicator of an analysis error
+            // or a call to a non-returning extern function that was not marked as non-returning.
+            return None;
+        }
 
         // Check whether state_before_return actually knows the `caller_stack_id`.
         // If not, we are returning from a state that cannot correspond to this callsite.
@@ -228,7 +234,11 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
             // The current workaround should be reasonably exact for programs written in C,
             // but may introduce a lot of errors
             // if the compiler often uses other calling conventions for internal function calls.
-            state_after_return.restore_callee_saved_register(state_before_call, cconv);
+            state_after_return.restore_callee_saved_register(
+                state_before_call,
+                cconv,
+                &self.project.stack_pointer_register,
+            );
         }
 
         // remove non-referenced objects from the state

cwe_checker_rs/src/analysis/pointer_inference/state/mod.rs

@@ -284,19 +284,22 @@ impl State {
         self.memory.append_unknown_objects(&caller_state.memory);
     }
 
-    /// Restore the content of callee-saved registers from the caller state.
+    /// Restore the content of callee-saved registers from the caller state
+    /// with the exception of the stack register.
     ///
     /// This function does not check what the callee state currently contains in these registers.
     /// If the callee does not adhere to the given calling convention, this may introduce analysis errors!
-    /// It will also mask cases,
+    /// It will also mask cases
     /// where a callee-saved register was incorrectly modified (e.g. because of a bug in the callee).
     pub fn restore_callee_saved_register(
         &mut self,
         caller_state: &State,
         cconv: &CallingConvention,
+        stack_register: &Variable,
     ) {
         for (register, value) in caller_state.register.iter() {
-            if cconv
+            if register != stack_register
+                && cconv
                     .callee_saved_register
                     .iter()
                     .any(|reg_name| *reg_name == register.name)

cwe_checker_rs/src/analysis/pointer_inference/state/tests.rs

@@ -374,7 +374,7 @@ fn remove_and_restore_callee_saved_register() {
 
     let other_value: Data = Bitvector::from_u64(13).into();
     callee_state.set_register(&register("RAX"), other_value.clone());
-    callee_state.restore_callee_saved_register(&state, &cconv);
+    callee_state.restore_callee_saved_register(&state, &cconv, &register("RSP"));
     assert_eq!(callee_state.get_register(&register("RBP")).unwrap(), value);
     assert_eq!(
         callee_state.get_register(&register("RAX")).unwrap(),