Fixed issue #250 by using correct calling convention. (#269)

Makefile

@@ -21,7 +21,7 @@ test:
 		echo "Acceptance test binaries not found. Please see test/artificial_samples/Readme.md for build instructions."; \
 		exit -1; \
 	fi
-	cargo test --no-fail-fast -p acceptance_tests_ghidra -- --show-output --ignored
+	cargo test --no-fail-fast -p acceptance_tests_ghidra -- --show-output --ignored --test-threads 1
 
 compile_test_files:
 	cd test/artificial_samples \

src/cwe_checker_lib/src/analysis/backward_interprocedural_fixpoint/tests.rs

@@ -89,6 +89,7 @@ fn mock_program() -> Term<Program> {
         term: Sub {
             name: "sub1".to_string(),
             blocks: vec![sub1_blk1, sub1_blk2],
+            calling_convention: None,
         },
     };
     let cond_jump = Jmp::CBranch {
@@ -124,6 +125,7 @@ fn mock_program() -> Term<Program> {
         term: Sub {
             name: "sub2".to_string(),
             blocks: vec![sub2_blk1, sub2_blk2],
+            calling_convention: None,
         },
     };
     let program = Term {

src/cwe_checker_lib/src/analysis/dead_variable_elimination/mod.rs

@@ -160,6 +160,7 @@ mod tests {
             term: Sub {
                 name: "sub".to_string(),
                 blocks: vec![block],
+                calling_convention: None,
             },
         };
         let mut project = Project::mock_empty();

src/cwe_checker_lib/src/analysis/forward_interprocedural_fixpoint.rs

@@ -57,6 +57,7 @@ pub trait Context<'a> {
         value: &Self::Value,
         call: &Term<Jmp>,
         target: &Node,
+        calling_convention: &Option<String>,
     ) -> Option<Self::Value>;
 
     /// Transition function for return instructions.
@@ -68,6 +69,7 @@ pub trait Context<'a> {
         value_before_call: Option<&Self::Value>,
         call_term: &Term<Jmp>,
         return_term: &Term<Jmp>,
+        calling_convention: &Option<String>,
     ) -> Option<Self::Value>;
 
     /// Transition function for calls to functions not contained in the binary.
@@ -163,7 +165,12 @@ impl<'a, T: Context<'a>> GeneralFPContext for GeneralizedContext<'a, T> {
             Edge::CallCombine(_) => Some(Self::NodeValue::Value(node_value.unwrap_value().clone())),
             Edge::Call(call) => self
                 .context
-                .update_call(node_value.unwrap_value(), call, &graph[end_node])
+                .update_call(
+                    node_value.unwrap_value(),
+                    call,
+                    &graph[end_node],
+                    &graph[end_node].get_sub().term.calling_convention,
+                )
                 .map(NodeValue::Value),
             Edge::CrCallStub => Some(NodeValue::CallFlowCombinator {
                 call_stub: Some(node_value.unwrap_value().clone()),
@@ -179,11 +186,11 @@ impl<'a, T: Context<'a>> GeneralFPContext for GeneralizedContext<'a, T> {
                     call_stub,
                     interprocedural_flow,
                 } => {
-                    let return_from_block = match graph.node_weight(start_node) {
+                    let (return_from_block, return_from_sub) = match graph.node_weight(start_node) {
                         Some(Node::CallReturn {
                             call: _,
-                            return_: (return_from_block, _),
-                        }) => return_from_block,
+                            return_: (return_from_block, return_from_sub),
+                        }) => (return_from_block, return_from_sub),
                         _ => panic!("Malformed Control flow graph"),
                     };
                     let return_from_jmp = &return_from_block.term.jmps[0];
@@ -193,6 +200,7 @@ impl<'a, T: Context<'a>> GeneralFPContext for GeneralizedContext<'a, T> {
                             call_stub.as_ref(),
                             call_term,
                             return_from_jmp,
+                            &return_from_sub.term.calling_convention,
                         )
                         .map(NodeValue::Value)
                 }

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

@@ -176,6 +176,7 @@ impl<'a> forward_interprocedural_fixpoint::Context<'a> for Context<'a> {
         _state: &State,
         _call: &Term<Jmp>,
         _target: &crate::analysis::graph::Node,
+        _calling_convention: &Option<String>,
     ) -> Option<State> {
         // No knowledge is transferred from the caller to the callee.
         None
@@ -215,6 +216,7 @@ impl<'a> forward_interprocedural_fixpoint::Context<'a> for Context<'a> {
         state_before_call: Option<&State>,
         call_term: &Term<Jmp>,
         _return_term: &Term<Jmp>,
+        _calling_convention: &Option<String>,
     ) -> Option<State> {
         if state.is_none() || state_before_call.is_none() {
             return None;

src/cwe_checker_lib/src/analysis/graph.rs

@@ -538,6 +538,7 @@ mod tests {
             term: Sub {
                 name: "sub1".to_string(),
                 blocks: vec![sub1_blk1, sub1_blk2],
+                calling_convention: None,
             },
         };
         let cond_jump = Jmp::CBranch {
@@ -573,6 +574,7 @@ mod tests {
             term: Sub {
                 name: "sub2".to_string(),
                 blocks: vec![sub2_blk1, sub2_blk2],
+                calling_convention: None,
             },
         };
         let program = Term {
@@ -617,6 +619,7 @@ mod tests {
             term: Sub {
                 name: "sub".to_string(),
                 blocks: vec![blk_term],
+                calling_convention: None,
             },
         };
         let mut program = Program::mock_empty();

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

@@ -167,11 +167,14 @@ fn context_problem_implementation() {
         term: Sub {
             name: "caller_sub".into(),
             blocks: vec![target_block.clone()],
+            calling_convention: None,
         },
     };
     let target_node = crate::analysis::graph::Node::BlkStart(&target_block, &sub);
     let call = call_term("func");
-    let mut callee_state = context.update_call(&state, &call, &target_node).unwrap();
+    let mut callee_state = context
+        .update_call(&state, &call, &target_node, &None)
+        .unwrap();
     assert_eq!(callee_state.stack_id, new_id("func", "RSP"));
     assert_eq!(callee_state.caller_stack_ids.len(), 1);
     assert_eq!(
@@ -208,6 +211,7 @@ fn context_problem_implementation() {
             Some(&state),
             &call,
             &return_term("return_target"),
+            &None,
         )
         .unwrap();
     assert_eq!(return_state.stack_id, new_id("main", "RSP"));
@@ -354,6 +358,7 @@ fn update_return() {
             Some(&state_before_call),
             &call_term("callee"),
             &return_term("return_target"),
+            &None,
         )
         .unwrap();
 

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

@@ -120,6 +120,7 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
         state: &State,
         call_term: &Term<Jmp>,
         _target_node: &crate::analysis::graph::Node,
+        calling_convention: &Option<String>,
     ) -> Option<State> {
         if let Jmp::Call {
             target: ref callee_tid,
@@ -143,7 +144,10 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
             // Remove callee-saved register, since the callee should not use their values anyway.
             // This should prevent recursive references to all stack frames in the call tree
             // since the source for it, the stack frame base pointer, is callee-saved.
-            if let Some(cconv) = self.project.get_standard_calling_convention() {
+            if let Some(cconv) = self
+                .project
+                .get_specific_calling_convention(calling_convention)
+            {
                 // Note that this may lead to analysis errors if the function uses another calling convention.
                 callee_state.remove_callee_saved_register(cconv);
             }
@@ -209,6 +213,7 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
         state_before_call: Option<&State>,
         call_term: &Term<Jmp>,
         return_term: &Term<Jmp>,
+        calling_convention: &Option<String>,
     ) -> Option<State> {
         // TODO: For the long term we may have to replace the IDs representing callers with something
         // that identifies the edge of the call and not just the callsite.
@@ -280,7 +285,10 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
 
         state_after_return.readd_caller_objects(state_before_call);
 
-        if let Some(cconv) = self.project.get_standard_calling_convention() {
+        if let Some(cconv) = self
+            .project
+            .get_specific_calling_convention(calling_convention)
+        {
             // Restore information about callee-saved register from the caller state.
             // TODO: Implement some kind of check to ensure that the callee adheres to the given calling convention!
             // The current workaround should be reasonably exact for programs written in C,

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

@@ -91,6 +91,7 @@ impl<'a, T: AbstractDomain + DomainInsertion + HasTop + Eq + From<String>>
         _state: &State<T>,
         _call: &Term<Jmp>,
         _target: &crate::analysis::graph::Node,
+        _calling_convention: &Option<String>,
     ) -> Option<State<T>> {
         None
     }
@@ -101,6 +102,7 @@ impl<'a, T: AbstractDomain + DomainInsertion + HasTop + Eq + From<String>>
         state_before_call: Option<&State<T>>,
         _call_term: &Term<Jmp>,
         _return_term: &Term<Jmp>,
+        _calling_convention: &Option<String>,
     ) -> Option<State<T>> {
         if let Some(state) = state_before_call {
             let mut new_state = state.clone();

src/cwe_checker_lib/src/analysis/string_abstraction/context/trait_impls/tests.rs

@@ -189,6 +189,7 @@ fn test_update_return() {
         None,
         &Jmp::branch("start1", "end1"),
         &Jmp::branch("start2", "end2"),
+        &None,
     );
 
     assert_eq!(None, new_state);
@@ -200,6 +201,7 @@ fn test_update_return() {
             Some(&setup.state_before_call),
             &Jmp::branch("start1", "end1"),
             &Jmp::branch("start2", "end2"),
+            &None,
         )
         .unwrap();
 

src/cwe_checker_lib/src/checkers/cwe_476/context.rs

@@ -260,7 +260,13 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
 
     /// Generate a CWE warning if taint may be contained in the function parameters.
     /// Always returns `None` so that the analysis stays intraprocedural.
-    fn update_call(&self, state: &State, call: &Term<Jmp>, _target: &Node) -> Option<Self::Value> {
+    fn update_call(
+        &self,
+        state: &State,
+        call: &Term<Jmp>,
+        _target: &Node,
+        _calling_convention: &Option<String>,
+    ) -> Option<Self::Value> {
         let pi_state_option = self.get_current_pointer_inference_state(state, &call.tid);
         if state.check_generic_function_params_for_taint(self.project, pi_state_option.as_ref()) {
             self.generate_cwe_warning(&call.tid);
@@ -390,6 +396,7 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
         state_before_call: Option<&State>,
         call_term: &Term<Jmp>,
         return_term: &Term<Jmp>,
+        _calling_convention: &Option<String>,
     ) -> Option<State> {
         if let Some(state) = state_before_return {
             // If taint is returned, generate a CWE warning

src/cwe_checker_lib/src/intermediate_representation/project.rs

@@ -39,6 +39,19 @@ impl Project {
             .or_else(|| self.calling_conventions.get("__cdecl"))
     }
 
+    /// Try to find a specific calling convention in the list of calling conventions in the project.
+    /// If not given a calling convention (i.e. given `None`) then falls back to `get_standard_calling_convention`
+    pub fn get_specific_calling_convention(
+        &self,
+        cconv_name_opt: &Option<String>,
+    ) -> Option<&CallingConvention> {
+        if let Some(cconv_name) = cconv_name_opt {
+            self.calling_conventions.get(cconv_name)
+        } else {
+            self.get_standard_calling_convention()
+        }
+    }
+
     /// Return the calling convention associated to the given extern symbol.
     /// If the extern symbol has no annotated calling convention
     /// then return the standard calling convention of the project instead.
@@ -140,6 +153,7 @@ impl Project {
                             indirect_jmp_targets: Vec::new(),
                         },
                     }],
+                    calling_convention: None,
                 },
             };
             self.program

src/cwe_checker_lib/src/intermediate_representation/project/block_duplication_normalization.rs

@@ -232,6 +232,7 @@ mod tests {
             term: Sub {
                 name: sub_name.to_string(),
                 blocks,
+                calling_convention: None,
             },
         }
     }

src/cwe_checker_lib/src/intermediate_representation/sub.rs

@@ -13,6 +13,8 @@ pub struct Sub {
     /// The basic blocks belonging to the subroutine.
     /// The first block is also the entry point of the subroutine.
     pub blocks: Vec<Term<Blk>>,
+    /// The calling convention used to call if known
+    pub calling_convention: Option<String>,
 }
 
 /// A parameter or return argument of a function.
@@ -206,6 +208,7 @@ mod tests {
                 term: Sub {
                     name: name.to_string(),
                     blocks: Vec::new(),
+                    calling_convention: None,
                 },
             }
         }

src/cwe_checker_lib/src/pcode/term.rs

@@ -337,6 +337,9 @@ pub struct Sub {
     ///
     /// Note that the first block of the array may *not* be the function entry point!
     pub blocks: Vec<Term<Blk>>,
+
+    /// The calling convention used (as reported by Ghidra, i.e. this may not be correct).
+    pub calling_convention: Option<String>,
 }
 
 impl Term<Sub> {
@@ -376,6 +379,7 @@ impl Term<Sub> {
             term: IrSub {
                 name: self.term.name,
                 blocks,
+                calling_convention: self.term.calling_convention,
             },
         }
     }

src/ghidra/p_code_extractor/internal/TermCreator.java

@@ -43,7 +43,11 @@ public class TermCreator {
      * Creates a Sub Term with an unique TID consisting of the prefix sub and its entry address.
      */
     public static Term<Sub> createSubTerm(Function func) {
-        return new Term<Sub>(HelperFunctions.functionEntryPoints.get(func.getEntryPoint().toString()), new Sub(func.getName(), func.getBody()));
+        Sub subInTerm = new Sub(func.getName(), func.getBody());
+        if (func.getCallingConvention() != null) {
+            subInTerm.setCallingConvention(func.getCallingConvention().toString());
+        }
+        return new Term<Sub>(HelperFunctions.functionEntryPoints.get(func.getEntryPoint().toString()), subInTerm);
     }
 
 

src/ghidra/p_code_extractor/term/Sub.java

@@ -12,6 +12,8 @@ public class Sub {
     private AddressSetView addresses;
     @SerializedName("blocks")
     private ArrayList<Term<Blk>> blocks;
+    @SerializedName("calling_convention")
+    private String callingConvention;
 
     public Sub() {
     }
@@ -54,4 +56,12 @@ public class Sub {
     public void setAddresses(AddressSetView addresses) {
         this.addresses = addresses;
     }
+
+    public String getCallingConvention() {
+        return this.callingConvention;
+    }
+
+    public void setCallingConvention(String callingConvention) {
+        this.callingConvention = callingConvention;
+    }
 }

test/src/lib.rs

@@ -434,7 +434,7 @@ mod tests {
         // We would need knowledge about alignment guarantees for the stack pointer at the start of main() to fix this.
         mark_skipped(&mut tests, "x86", "gcc");
 
-        mark_compiler_skipped(&mut tests, "mingw32-gcc"); // TODO: Check reason for failure!
+        mark_skipped(&mut tests, "x86", "mingw32-gcc"); // TODO: Check reason for failure! Probably same as above?
 
         for test_case in tests {
             let num_expected_occurences = 2;
@@ -462,7 +462,7 @@ mod tests {
         // We would need knowledge about alignment guarantees for the stack pointer at the start of main() to fix this.
         mark_skipped(&mut tests, "x86", "gcc");
 
-        mark_compiler_skipped(&mut tests, "mingw32-gcc"); // TODO: Check reason for failure!
+        mark_skipped(&mut tests, "x86", "mingw32-gcc"); // TODO: Check reason for failure! Probably same as above?
 
         for test_case in tests {
             let num_expected_occurences = 1;