From aefa92bb6621208d708102be17753374e562f3cc Mon Sep 17 00:00:00 2001
From: Ivan Gotovchits <ivg@ieee.org>
Date: Tue, 13 Aug 2019 11:59:08 -0400
Subject: [PATCH] fixes arguments in the call-return observation (#970)

* fixes arguments in the call-return observation

When a call-return is filled in with the arguments we shouldn't
re-evaluate any actuals that are not `Out`, as `In` (obviously)
and `In Out` (not that obviously) argument actuals could be already
overwritten.

A deeper story. When the C language is detected and when we have a
prototype for a function, we classify arguments that are passed by
reference (i.e., those that are passed by a non-const referenced) as
`In Out`. However, when a value is passed by reference the reference
itself is passed by value.

The fun fact, or why we didn't notice it before. There are two places
where `call-return` observation is made, namely in the Primus
Interpreter and in the Primus Lisp Interpreter. The latter was always
doing the right thing - it was passing the input arguments, and
appended the output (as this PR is doing now for the former). Since,
our analyses were always focusing on external functions (such as
malloc, strcpy, strlen, etc) which were handled by the Primus Lisp
interpreter we never noticed anything fishy.

Thanks @Isweet for finding this issue up.

* updates testsuite

* properly init bap-veri sub sub module
---
 .travis_install.sh                       |  4 ++--
 lib/bap_primus/bap_primus_interpreter.ml | 21 ++++++++++++---------
 testsuite                                |  2 +-
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/.travis_install.sh b/.travis_install.sh
index 7a1dd696b..4b58f7a50 100755
--- a/.travis_install.sh
+++ b/.travis_install.sh
@@ -5,8 +5,8 @@ export TESTS=false
 if [ "$WITH_BUILD_CACHE" == "true" ]; then
     export POST_INSTALL_HOOK='
 rm -rf bap-veri
-git clone https://github.com/BinaryAnalysisPlatform/bap-veri.git
-opam pin add bap-veri bap-veri/ -y
+git submodule update --init --recursive
+opam pin add bap-veri testsuite/veri/bap-veri/ -y
 OPAM_SWITCH=`opam config var switch`
 mkdir -p $HOME/save_opam/$OPAM_SWITCH/lib/bap
 cp -r $HOME/.opam/$OPAM_SWITCH/bin/ $HOME/save_opam/$OPAM_SWITCH/
diff --git a/lib/bap_primus/bap_primus_interpreter.ml b/lib/bap_primus/bap_primus_interpreter.ml
index e1221a589..8b2db20ff 100644
--- a/lib/bap_primus/bap_primus_interpreter.ml
+++ b/lib/bap_primus/bap_primus_interpreter.ml
@@ -141,10 +141,10 @@ let sexp_of_concat ((x,y),r) = results r @@ sexps [
   ]
 
 let sexp_of_ite ((cond, yes, no), r) = results r @@ sexps [
-  string_of_value cond;
-  string_of_value yes;
-  string_of_value no;
-]
+    string_of_value cond;
+    string_of_value yes;
+    string_of_value no;
+  ]
 
 let binop,on_binop =
   Observation.provide ~inspect:sexp_of_binop "binop"
@@ -524,7 +524,7 @@ module Make (Machine : Machine) = struct
   let arg_def = term normal arg_t arg_def
 
   let arg_use t = match Arg.intent t with
-    | None | Some (Out|Both) -> Arg.lhs t := Arg.rhs t
+    | Some Out -> Arg.lhs t := Arg.rhs t
     | _ -> Machine.return ()
 
   let arg_use = term normal arg_t arg_use
@@ -532,7 +532,9 @@ module Make (Machine : Machine) = struct
   let get_arg t = Env.get (Arg.lhs t)
   let get_args ~input sub =
     Term.enum arg_t sub |>
-    Seq.filter ~f:(fun x -> not input || Arg.intent x <> Some Out) |>
+    Seq.filter ~f:(fun x -> match input with
+        | true ->  Arg.intent x <> Some Out
+        | false -> Arg.intent x = Some Out) |>
     Machine.Seq.map ~f:get_arg
 
   let iter_args t f = Machine.Seq.iter (Term.enum arg_t t) ~f
@@ -542,11 +544,12 @@ module Make (Machine : Machine) = struct
     | Some entry ->
       let name = Sub.name t in
       iter_args t arg_def >>= fun () ->
-      get_args ~input:true t >>| Seq.to_list >>= fun args ->
-      !!Linker.Trace.call_entered (name,args) >>= fun () ->
+      get_args ~input:true t >>| Seq.to_list_rev >>= fun inputs ->
+      !!Linker.Trace.call_entered (name,List.rev inputs) >>= fun () ->
       blk entry >>= fun () ->
       iter_args t arg_use >>= fun () ->
-      get_args ~input:false t >>| Seq.to_list >>= fun args ->
+      get_args ~input:false t >>| Seq.to_list >>= fun rets ->
+      let args = List.rev_append inputs rets in
       !!Linker.Trace.call_returned (name,args)
 
 
diff --git a/testsuite b/testsuite
index c4a0d7341..16065e0f2 160000
--- a/testsuite
+++ b/testsuite
@@ -1 +1 @@
-Subproject commit c4a0d73411064f2ffbfd384c10dd881877e9b0af
+Subproject commit 16065e0f2dc8367a0ea8ccce9556658e0745cda4