Do not generate parameters for array elements in FnSig analysis

src/cwe_checker_lib/src/analysis/fixpoint.rs

@@ -234,6 +234,16 @@ impl<T: Context> Computation<T> {
         &self.node_values
     }
 
+    /// Get a mutable iterator over all node values.
+    /// Also add all nodes that have values to the worklist, because one can change all their values through the iterator.
+    pub fn node_values_mut(&mut self) -> impl Iterator<Item = &mut T::NodeValue> {
+        for node in self.node_values.keys() {
+            let priority = self.node_priority_list[node.index()];
+            self.worklist.insert(priority);
+        }
+        self.node_values.values_mut()
+    }
+
     /// Get a reference to the underlying graph
     pub fn get_graph(&self) -> &DiGraph<T::NodeLabel, T::EdgeLabel> {
         self.fp_context.get_graph()

src/cwe_checker_lib/src/analysis/function_signature/context/tests.rs

@@ -68,6 +68,7 @@ fn test_call_stub_handling() {
         &Tid::new("func"),
         &project.stack_pointer_register,
         project.get_standard_calling_convention().unwrap(),
+        2,
     );
     let extern_symbol = ExternSymbol::mock_malloc_symbol_arm();
     let call_tid = Tid::new("call_malloc");
@@ -93,6 +94,7 @@ fn test_call_stub_handling() {
         &Tid::new("func"),
         &project.stack_pointer_register,
         project.get_standard_calling_convention().unwrap(),
+        2,
     );
     // Set the format string param register to a pointer to the string 'cat %s %s %s %s'.
     state.set_register(&variable!("r1:4"), bitvec!("0x6000:4").into());

src/cwe_checker_lib/src/analysis/function_signature/mod.rs

@@ -31,6 +31,9 @@
 //!   Nevertheless, it should result in an overapproximation of parameters and their access patterns in most cases.
 //! * The nesting depth for tracked nested parameters is limited
 //!   to avoid generating infinitely many parameters for recursive types like linked lists.
+//! * For arrays no parameters should be created for the array elements.
+//!   However, if only a particular element in an array is accessed without iteration over the array,
+//!   then a parameter might be generated for that element.
 
 use crate::abstract_domain::AbstractDomain;
 use crate::abstract_domain::AbstractLocation;
@@ -55,16 +58,12 @@ mod global_var_propagation;
 use global_var_propagation::propagate_globals;
 pub mod stubs;
 
-/// The recursion depth limit for abstract locations to be tracked by the function signature analysis,
-/// i.e. how many dereference operations an abstract location is allowed to contain
-/// before the analysis stops tracking the location.
-const POINTER_RECURSION_DEPTH_LIMIT: u64 = 2;
-
 /// Generate the computation object for the fixpoint computation
 /// and set the node values for all function entry nodes.
 fn generate_fixpoint_computation<'a>(
     project: &'a Project,
     graph: &'a Graph,
+    pointer_recursion_depth_limit: u64,
 ) -> Computation<GeneralizedContext<'a, Context<'a>>> {
     let context = Context::new(project, graph);
     let mut computation = create_computation(context, None);
@@ -81,6 +80,7 @@ fn generate_fixpoint_computation<'a>(
                         &sub.tid,
                         &project.stack_pointer_register,
                         calling_convention,
+                        pointer_recursion_depth_limit,
                     );
                     if project.cpu_architecture.contains("MIPS") {
                         let _ = fn_start_state
@@ -152,8 +152,35 @@ pub fn compute_function_signatures<'a>(
     project: &'a Project,
     graph: &'a Graph,
 ) -> (BTreeMap<Tid, FunctionSignature>, Vec<LogMessage>) {
-    let mut computation = generate_fixpoint_computation(project, graph);
+    let max_pointer_recursion_depth_limit: u64 = 2;
+    // We gradually increase the recursion depth limit used in the fixpoint computation.
+    // The idea is that for array accesses the offset has time to converge to `Top` before IDs for nested objects are created.
+    // Otherwise the algorithm would generate an object for the first element of an array
+    // before it can check that it is an array access that we want to ignore.
+    let mut computation = generate_fixpoint_computation(project, graph, 0);
     computation.compute_with_max_steps(100);
+    for pointer_recursion_limit in 1..=max_pointer_recursion_depth_limit {
+        for node_value in computation.node_values_mut() {
+            match node_value {
+                NodeValue::Value(state) => {
+                    state.set_pointer_recursion_depth_limit(pointer_recursion_limit)
+                }
+                NodeValue::CallFlowCombinator {
+                    call_stub,
+                    interprocedural_flow,
+                } => {
+                    for state in call_stub.iter_mut() {
+                        state.set_pointer_recursion_depth_limit(pointer_recursion_limit);
+                    }
+                    for state in interprocedural_flow.iter_mut() {
+                        state.set_pointer_recursion_depth_limit(pointer_recursion_limit);
+                    }
+                }
+            }
+        }
+        computation.compute_with_max_steps(100);
+    }
+
     let mut fn_sig_map = extract_fn_signatures_from_fixpoint(project, graph, computation);
     // Sanitize the parameters
     let mut logs = Vec::new();

src/cwe_checker_lib/src/analysis/function_signature/state/memory_handling.rs

@@ -1,5 +1,4 @@
 use super::State;
-use super::POINTER_RECURSION_DEPTH_LIMIT;
 use crate::abstract_domain::*;
 use crate::intermediate_representation::*;
 
@@ -48,7 +47,7 @@ impl State {
                 Err(_) => DataDomain::new_top(size),
             }
         } else if let (true, Ok(constant_offset)) = (
-            id.get_location().recursion_depth() < POINTER_RECURSION_DEPTH_LIMIT,
+            id.get_location().recursion_depth() < self.pointer_recursion_depth_limit,
             offset.try_to_offset(),
         ) {
             // Extend the abstract location string

src/cwe_checker_lib/src/analysis/function_signature/state/mod.rs

@@ -1,5 +1,4 @@
 use super::AccessPattern;
-use super::POINTER_RECURSION_DEPTH_LIMIT;
 use crate::abstract_domain::*;
 use crate::intermediate_representation::*;
 use crate::prelude::*;
@@ -27,6 +26,10 @@ pub struct State {
     stack: MemRegion<DataDomain<BitvectorDomain>>,
     /// Maps each tracked ID to an [`AccessPattern`], which tracks known access patterns to the object.
     tracked_ids: DomainMap<AbstractIdentifier, AccessPattern, UnionMergeStrategy>,
+    /// The recursion depth limit for abstract locations to be tracked by the function signature analysis,
+    /// i.e. how many dereference operations an abstract location is allowed to contain
+    /// before the analysis stops tracking the location.
+    pointer_recursion_depth_limit: u64,
 }
 
 impl State {
@@ -37,6 +40,7 @@ impl State {
         func_tid: &Tid,
         stack_register: &Variable,
         calling_convention: &CallingConvention,
+        pointer_recursion_depth_limit: u64,
     ) -> State {
         let mut register_map = BTreeMap::new();
         let mut tracked_ids = BTreeMap::new();
@@ -64,9 +68,18 @@ impl State {
             stack_id,
             stack,
             tracked_ids: DomainMap::from(tracked_ids),
+            pointer_recursion_depth_limit,
         }
     }
 
+    /// Set the pointer recursion depth limit to the provided value.
+    ///
+    /// Note that one should call this function for all states,
+    /// because merging two states with different depth limits will panic.
+    pub fn set_pointer_recursion_depth_limit(&mut self, limit: u64) {
+        self.pointer_recursion_depth_limit = limit;
+    }
+
     /// Set the MIPS link register `t9` to the address of the function TID.
     ///
     /// According to the System V ABI for MIPS the caller has to save the callee address in register `t9`
@@ -219,7 +232,7 @@ impl State {
                 }
             };
             location.extend(mem_location, self.stack_id.bytesize());
-            if location.recursion_depth() <= POINTER_RECURSION_DEPTH_LIMIT {
+            if location.recursion_depth() <= self.pointer_recursion_depth_limit {
                 eval_result = eval_result.merge(&DataDomain::from_target(
                     AbstractIdentifier::new(id.get_tid().clone(), location),
                     Bitvector::zero(target_size.into()).into(),
@@ -382,13 +395,18 @@ fn get_pointer_inputs_vars_of_address_expression(expr: &Expression) -> Vec<&Vari
 impl AbstractDomain for State {
     /// Merge two states
     fn merge(&self, other: &Self) -> Self {
+        assert_eq!(
+            self.pointer_recursion_depth_limit,
+            other.pointer_recursion_depth_limit
+        );
         let stack_id = self.stack_id.clone();
         let stack = self.stack.merge(&other.stack);
         State {
             register: self.register.merge(&other.register),
             stack_id,
             stack,
             tracked_ids: self.tracked_ids.merge(&other.tracked_ids),
+            pointer_recursion_depth_limit: self.pointer_recursion_depth_limit,
         }
     }
 

src/cwe_checker_lib/src/analysis/function_signature/state/tests.rs

@@ -1,5 +1,10 @@
 use super::*;
-use crate::{bitvec, expr, variable};
+use crate::{
+    analysis::{
+        forward_interprocedural_fixpoint::Context as _, function_signature::context::Context,
+    },
+    bitvec, defs, expr, variable,
+};
 
 impl State {
     /// Generate a mock state for an ARM-32 state.
@@ -8,6 +13,7 @@ impl State {
             &Tid::new("mock_fn"),
             &variable!("sp:4"),
             &CallingConvention::mock_arm32(),
+            2,
         )
     }
 
@@ -17,6 +23,7 @@ impl State {
             &Tid::new(tid_name),
             &variable!("RSP:8"),
             &CallingConvention::mock_x64(),
+            2,
         )
     }
 }
@@ -137,3 +144,52 @@ fn test_substitute_global_mem_address() {
         DataDomain::from_target(expected_global_id, bitvec!("0x0:4").into())
     );
 }
+
+#[test]
+fn test_pointer_recursion_depth_limit_handling() {
+    let project = Project::mock_arm32();
+    let graph = crate::analysis::graph::get_program_cfg(&project.program);
+    let context = Context::new(&project, &graph);
+    // Test interaction of gradually increasing the pointer recursion depth limit with a loop that
+    // - iterates over an array
+    // - recursively dereferences a variable
+    let mut state = State::mock_arm32();
+    let defs = defs![
+        "instr_1: Store at r1:4 := r0:4",
+        "instr_2: r1:4 = r1:4 + 0x1:4",
+        "instr_3: r3:4 := Load from r3:4"
+    ];
+    let array_elem_location = AbstractLocation::mock("r1:4", &[0], 4);
+    let array_elem_id = AbstractIdentifier::new(
+        state.get_current_function_tid().clone(),
+        array_elem_location,
+    );
+    let recursive_elem_location = AbstractLocation::mock("r3:4", &[0], 4);
+    let recursive_elem_id = AbstractIdentifier::new(
+        state.get_current_function_tid().clone(),
+        recursive_elem_location,
+    );
+    // Iteration with depth limit 0
+    state.pointer_recursion_depth_limit = 0;
+    let prev_state = state.clone();
+    for def in &defs {
+        state = context.update_def(&state, def).unwrap();
+    }
+    state = state.merge(&prev_state);
+    // No array element ID should have been created.
+    assert!(state.tracked_ids.get(&array_elem_id).is_none());
+    // No recursive access ID should have been created.
+    assert!(state.tracked_ids.get(&recursive_elem_id).is_none());
+
+    // Iteration with depth limit 1
+    state.pointer_recursion_depth_limit = 1;
+    let prev_state = state.clone();
+    for def in &defs {
+        state = context.update_def(&state, def).unwrap();
+    }
+    state = state.merge(&prev_state);
+    // No array element ID should have been created.
+    assert!(state.tracked_ids.get(&array_elem_id).is_none());
+    // But the recursive access ID should now exist.
+    assert!(state.tracked_ids.get(&recursive_elem_id).is_some());
+}