reduce duplicate CWE-416 warnings (#183)

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

@@ -286,7 +286,7 @@ impl<'a> Context<'a> {
     /// Check all parameter registers of a call for dangling pointers and report possible use-after-frees.
     fn check_parameter_register_for_dangling_pointer(
         &self,
-        state: &State,
+        state: &mut State,
         call: &Term<Jmp>,
         extern_symbol: &ExternSymbol,
     ) {
@@ -298,6 +298,9 @@ impl<'a> Context<'a> {
             ) {
                 Ok(value) => {
                     if state.memory.is_dangling_pointer(&value, true) {
+                        state
+                            .memory
+                            .mark_dangling_pointer_targets_as_flagged(&value);
                         let warning = CweWarning {
                             name: "CWE416".to_string(),
                             version: VERSION.to_string(),

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

@@ -16,8 +16,9 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
 
     /// Update the state according to the effects of the given `Def` term.
     fn update_def(&self, state: &Self::Value, def: &Term<Def>) -> Option<Self::Value> {
+        let mut new_state = state.clone();
         // first check for use-after-frees
-        if state.contains_access_of_dangling_memory(&def.term) {
+        if new_state.contains_access_of_dangling_memory(&def.term) {
             let warning = CweWarning {
                 name: "CWE416".to_string(),
                 version: VERSION.to_string(),
@@ -65,7 +66,6 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
 
         match &def.term {
             Def::Store { address, value } => {
-                let mut new_state = state.clone();
                 self.log_debug(
                     new_state.handle_store(address, value, self.runtime_memory_image),
                     Some(&def.tid),
@@ -73,12 +73,10 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
                 Some(new_state)
             }
             Def::Assign { var, value } => {
-                let mut new_state = state.clone();
                 new_state.handle_register_assign(var, value);
                 Some(new_state)
             }
             Def::Load { var, address } => {
-                let mut new_state = state.clone();
                 self.log_debug(
                     new_state.handle_load(var, address, &self.runtime_memory_image),
                     Some(&def.tid),
@@ -308,13 +306,23 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
         if let Some(extern_symbol) = self.extern_symbol_map.get(call_target) {
             // Generate a CWE-message if some argument is an out-of-bounds pointer.
             self.check_parameter_register_for_out_of_bounds_pointer(state, call, extern_symbol);
+            // Check parameter for possible use-after-frees (except for possible double frees, which are handled later)
+            if !self
+                .deallocation_symbols
+                .iter()
+                .any(|free_like_fn| free_like_fn == extern_symbol.name.as_str())
+            {
+                self.check_parameter_register_for_dangling_pointer(
+                    &mut new_state,
+                    call,
+                    extern_symbol,
+                );
+            }
             // Clear non-callee-saved registers from the state.
             let cconv = extern_symbol.get_calling_convention(&self.project);
             new_state.clear_non_callee_saved_register(&cconv.callee_saved_register[..]);
             // Adjust stack register value (for x86 architecture).
             self.adjust_stack_register_on_extern_call(state, &mut new_state);
-            // Check parameter for possible use-after-frees
-            self.check_parameter_register_for_dangling_pointer(state, call, extern_symbol);
 
             match extern_symbol.name.as_str() {
                 malloc_like_fn if self.allocation_symbols.iter().any(|x| x == malloc_like_fn) => {

src/cwe_checker_lib/src/analysis/pointer_inference/object.rs

@@ -44,7 +44,7 @@ pub struct AbstractObjectInfo {
     /// Tracks whether this may represent more than one actual memory object.
     pub is_unique: bool,
     /// Is the object alive or already destroyed
-    state: Option<ObjectState>,
+    state: ObjectState,
     /// Is the object a stack frame or a heap object
     type_: Option<ObjectType>,
     /// The actual content of the memory object
@@ -65,7 +65,7 @@ impl AbstractObjectInfo {
         AbstractObjectInfo {
             pointer_targets: BTreeSet::new(),
             is_unique: true,
-            state: Some(ObjectState::Alive),
+            state: ObjectState::Alive,
             type_: Some(type_),
             memory: MemRegion::new(address_bytesize),
             lower_index_bound: BitvectorDomain::Top(address_bytesize),
@@ -208,12 +208,12 @@ impl AbstractObjectInfo {
     }
 
     /// If `self.is_unique==true`, set the state of the object. Else merge the new state with the old.
-    pub fn set_state(&mut self, new_state: Option<ObjectState>) {
+    pub fn set_state(&mut self, new_state: ObjectState) {
         if self.is_unique {
             self.state = new_state;
-        } else if self.state != new_state {
-            self.state = None;
-        } // else don't change the state
+        } else {
+            self.state = self.state.merge(new_state);
+        }
     }
 
     /// Remove the provided IDs from the target lists of all pointers in the memory object.
@@ -231,7 +231,7 @@ impl AbstractObjectInfo {
     }
 
     /// Get the state of the memory object.
-    pub fn get_state(&self) -> Option<ObjectState> {
+    pub fn get_state(&self) -> ObjectState {
         self.state
     }
 
@@ -254,20 +254,24 @@ impl AbstractObjectInfo {
     /// or the memory object may not be a heap object.
     pub fn mark_as_freed(&mut self) -> Result<(), Error> {
         if self.type_ != Some(ObjectType::Heap) {
-            self.set_state(Some(ObjectState::Dangling));
+            self.set_state(ObjectState::Flagged);
             return Err(anyhow!("Free operation on possibly non-heap memory object"));
         }
         match (self.is_unique, self.state) {
-            (true, Some(ObjectState::Alive)) => {
-                self.state = Some(ObjectState::Dangling);
+            (true, ObjectState::Alive) | (true, ObjectState::Flagged) => {
+                self.state = ObjectState::Dangling;
+                Ok(())
+            }
+            (false, ObjectState::Flagged) => {
+                self.state = ObjectState::Unknown;
                 Ok(())
             }
-            (true, _) | (false, Some(ObjectState::Dangling)) => {
-                self.state = Some(ObjectState::Dangling);
+            (true, _) | (false, ObjectState::Dangling) => {
+                self.state = ObjectState::Flagged;
                 Err(anyhow!("Object may already have been freed"))
             }
             (false, _) => {
-                self.state = None;
+                self.state = ObjectState::Unknown;
                 Ok(())
             }
         }
@@ -278,15 +282,19 @@ impl AbstractObjectInfo {
     /// or if the object may not be a heap object.
     pub fn mark_as_maybe_freed(&mut self) -> Result<(), Error> {
         if self.type_ != Some(ObjectType::Heap) {
-            self.set_state(Some(ObjectState::Dangling));
+            self.set_state(ObjectState::Flagged);
             return Err(anyhow!("Free operation on possibly non-heap memory object"));
         }
-        if self.state != Some(ObjectState::Dangling) {
-            self.state = None;
-            Ok(())
-        } else {
+        match self.state {
+            ObjectState::Dangling => {
+                self.state = ObjectState::Flagged;
                 Err(anyhow!("Object may already have been freed"))
             }
+            _ => {
+                self.state = ObjectState::Unknown;
+                Ok(())
+            }
+        }
     }
 }
 
@@ -300,7 +308,7 @@ impl AbstractDomain for AbstractObjectInfo {
                 .cloned()
                 .collect(),
             is_unique: self.is_unique && other.is_unique,
-            state: same_or_none(&self.state, &other.state),
+            state: self.state.merge(other.state),
             type_: same_or_none(&self.type_, &other.type_),
             memory: self.memory.merge(&other.memory),
             lower_index_bound: self.lower_index_bound.merge(&other.lower_index_bound),
@@ -377,6 +385,26 @@ pub enum ObjectState {
     Alive,
     /// The object is dangling, i.e. the memory has been freed already.
     Dangling,
+    /// The state of the object is unknown (due to merging different object states).
+    Unknown,
+    /// The object was referenced in an "use-after-free" or "double-free" CWE-warning.
+    /// This state is meant to be temporary to prevent obvious subsequent CWE-warnings with the same root cause.
+    Flagged,
+}
+
+impl ObjectState {
+    /// Merge two object states.
+    /// If one of the two states is `Flagged`, then the resulting state is the other object state.
+    pub fn merge(self, other: Self) -> Self {
+        use ObjectState::*;
+        match (self, other) {
+            (Flagged, state) | (state, Flagged) => state,
+            (Unknown, _) | (_, Unknown) => Unknown,
+            (Alive, Alive) => Alive,
+            (Dangling, Dangling) => Dangling,
+            (Alive, Dangling) | (Dangling, Alive) => Unknown,
+        }
+    }
 }
 
 #[cfg(test)]
@@ -387,7 +415,7 @@ mod tests {
         let obj_info = AbstractObjectInfo {
             pointer_targets: BTreeSet::new(),
             is_unique: true,
-            state: Some(ObjectState::Alive),
+            state: ObjectState::Alive,
             type_: Some(ObjectType::Heap),
             memory: MemRegion::new(ByteSize::new(8)),
             lower_index_bound: Bitvector::from_u64(0).into(),

src/cwe_checker_lib/src/analysis/pointer_inference/object_list.rs

@@ -44,20 +44,20 @@ impl AbstractObjectList {
     }
 
     /// Check the state of a memory object at a given address.
-    /// Returns True if at least one of the targets of the pointer is dangling.
-    /// If `report_none_states` is `true`,
+    /// Returns `true` if at least one of the targets of the pointer is dangling.
+    /// If `report_unknown_states` is `true`,
     /// then objects with unknown states get reported if they are unique.
     /// I.e. objects representing more than one actual object (e.g. an array of object) will not get reported,
-    /// even if their state is unknown and `report_none_states` is `true`.
-    pub fn is_dangling_pointer(&self, address: &Data, report_none_states: bool) -> bool {
+    /// even if their state is unknown and `report_unknown_states` is `true`.
+    pub fn is_dangling_pointer(&self, address: &Data, report_unknown_states: bool) -> bool {
         match address {
             Data::Value(_) | Data::Top(_) => (),
             Data::Pointer(pointer) => {
                 for id in pointer.ids() {
                     let (object, _offset_id) = self.objects.get(id).unwrap();
-                    match (report_none_states, object.get_state()) {
-                        (_, Some(ObjectState::Dangling)) => return true,
-                        (true, None) => {
+                    match (report_unknown_states, object.get_state()) {
+                        (_, ObjectState::Dangling) => return true,
+                        (true, ObjectState::Unknown) => {
                             if object.is_unique {
                                 return true;
                             }
@@ -71,6 +71,23 @@ impl AbstractObjectList {
         false
     }
 
+    /// Mark all memory objects targeted by the given `address` pointer,
+    /// whose state is either dangling or unknown,
+    /// as flagged.
+    pub fn mark_dangling_pointer_targets_as_flagged(&mut self, address: &Data) {
+        if let Data::Pointer(pointer) = address {
+            for id in pointer.ids() {
+                let (object, _) = self.objects.get_mut(id).unwrap();
+                if matches!(
+                    object.get_state(),
+                    ObjectState::Unknown | ObjectState::Dangling
+                ) {
+                    object.set_state(ObjectState::Flagged);
+                }
+            }
+        }
+    }
+
     /// Check whether a memory access at the given address (and accessing `size` many bytes)
     /// may be an out-of-bounds memory access.
     ///
@@ -578,7 +595,7 @@ mod tests {
                 .unwrap()
                 .0
                 .get_state(),
-            Some(crate::analysis::pointer_inference::object::ObjectState::Alive)
+            crate::analysis::pointer_inference::object::ObjectState::Alive
         );
         other_obj_list
             .mark_mem_object_as_freed(&modified_heap_pointer)
@@ -591,7 +608,7 @@ mod tests {
                 .unwrap()
                 .0
                 .get_state(),
-            Some(crate::analysis::pointer_inference::object::ObjectState::Dangling)
+            crate::analysis::pointer_inference::object::ObjectState::Dangling
         );
     }
 

src/cwe_checker_lib/src/analysis/pointer_inference/state/access_handling.rs

@@ -247,11 +247,19 @@ impl State {
         }
     }
 
-    /// Check if an expression contains a use-after-free
-    pub fn contains_access_of_dangling_memory(&self, def: &Def) -> bool {
+    /// Check if an expression contains a use-after-free.
+    /// If yes, mark the corresponding memory objects as flagged.
+    pub fn contains_access_of_dangling_memory(&mut self, def: &Def) -> bool {
         match def {
             Def::Load { address, .. } | Def::Store { address, .. } => {
-                self.memory.is_dangling_pointer(&self.eval(address), true)
+                let address_value = self.eval(address);
+                if self.memory.is_dangling_pointer(&address_value, true) {
+                    self.memory
+                        .mark_dangling_pointer_targets_as_flagged(&address_value);
+                    true
+                } else {
+                    false
+                }
             }
             _ => false,
         }