adds mangling of duplicated subroutines #813

gitoleg · 2018-04-04T20:28:55Z

We track every addition of a subroutine to a program by storing names and tids in a table. Every addition replaces previous entries with the same name. When someone requests a result, it's easy to tell if a subroutine name needs in demangling: if a tid in the table for this name is differ from a subroutine's one, then other subroutine started to use this name, and we need to demangle current name. The same is true for subroutine aliases.

Implemented like an iteration over existed subroutines

ivg · 2018-04-04T20:39:25Z

lib/bap_types/bap_ir.ml

+    Term.get_attr s Bap_attributes.address
+
+  let mangle_name addr tid name =
+    let mname = match addr with


there is an above zero chance that we will have intersections between the set of text representations of addresses and the set of text representation of tids, especially if we are working with objects, where the address space of the code sections starts from zero, the same as tids.

That's why we need to disjoin them with some prefix notation, e.g., malloc@deadbeef and malloc%deadbeef.

ivg · 2018-04-04T20:49:53Z

lib/bap_types/bap_ir.ml

+  let need_to_mangle s names =
+    let tid = Term.tid s in
+    let sub_names = Ir_sub.name s :: sub_aliases s in
+    List.for_all sub_names ~f:(fun n ->


it should be exists not for_all. With for_all you will be satisfied even if first name is unique and the rest won't be checked.

ivg · 2018-04-04T20:56:26Z

lib/bap_types/bap_ir.ml

+        | Some tid' -> not (Tid.equal tid tid'))
+
+  let fix_names names subs =
+    let may_mangle addr tid name =


just name it mangle

ivg · 2018-04-04T21:10:18Z

lib/bap_types/bap_ir.ml

+          let sub = Term.set_attr sub Ir_sub.aliases als in
+          let sub = Ir_sub.with_name sub nam in
+          (i, sub) :: acc) in
+    List.iter ~f:(fun (i, s) -> Vec.set subs i s) subs'


I really don't like using vector here... it is abusing.

ivg · 2018-04-04T21:10:34Z

lib/bap_types/bap_ir.ml

  module Builder = struct
-    type t = tid option * sub term vector
+    type t = tid option * tid String.Table.t * sub term vector


You shall not do this on the builder interface.

ivg · 2018-04-04T21:16:40Z

You shall enforce this invariant on the program data structure, so every time a value of type program is created you need to check it (non-invasive) and if the invariant holds, then return the program unchanged otherwise rename the offending subroutines.

I would suggest you to make the program type private to ensure that it is not created anywhere without a check (that means that you need to hide it inside a module). Of course, this should all be done in the implementation, nothing should be changed in the interface.

ivg

Let me cite myself from #808

Next, we need a function fix_names: ?old_names:string Tid.Map.t -> program -> program that will enforce the desired predicates. . When a conflict is detected, i.e., we have a name, that is already assigned an address, then we need to rename one of the two names. We need to give a preference to one or another, trying to preserve the new name and rename the old one. The name is old if it is in the old_names and has the same name. Otherwise it is new one. If both names are new, then pick the rename a name that corresponds to a tid that is smaller.

So, please, implement this priority clause. In the setter you have both the old program and the new program. If a term is renamed then the fresh name should be chosen. In other words there should be the following invariant, that if you update a program with a subroutine s with name n then it will contain the subroutine s with name n (i.e., only other subroutines will be renamed).

forall p:program, s:sub, n:name, x:string, (p[s][n] <- x)[s][n] = x

ivg · 2018-04-05T16:52:14Z

lib/bap_types/bap_ir.ml

+        Hashtbl.change names name ~f:(function
+            | None -> Some tid
+            | Some tid' ->
+              if Tid.(tid' < tid) then Some tid


it is called Some (Tid.max tid' tid)

ivg · 2018-04-05T16:54:42Z

lib/bap_types/bap_ir.ml

+    Array.iter subs ~f:(fun s ->
+        let name = s.self.name in
+        let tid = s.tid in
+        Hashtbl.change names name ~f:(function


use Hashtbl.update if you're not going to remove an element.

ivg · 2018-04-05T16:56:20Z

lib/bap_types/bap_ir.ml

+    let names = String.Table.create () in
+    Array.iter subs ~f:(fun s ->
+        let name = s.self.name in
+        let tid = s.tid in


is it really necessary to introduce this let bindings? They just clobber the code, I think.

ivg · 2018-04-05T17:00:37Z

lib/bap_types/bap_ir.ml

+        Hashtbl.change names name ~f:(function
+            | None -> Some tid
+            | Some tid' ->
+              if Tid.(tid' < tid) then Some tid


So this policy prioritizes a subroutine with a smaller tid, though it doesn't give the priority to the last chosen name.

ivg

Please, rewrite your implementation without relying on hashtables and tid ordering. Try to write it in a functional style. The underlying task is expressible without doing all this adding/removing and mutating lots of state.

ivg · 2018-04-09T11:56:19Z

lib/bap_types/bap_ir.ml

+let names_of_subs subs =
+  let names = String.Table.create () in
+  Array.iter subs ~f:(fun s ->
+      Hashtbl.change names s.self.name ~f:(function


there is the add_multi function for this.

ivg · 2018-04-09T11:58:04Z

lib/bap_types/bap_ir.ml

+  let self = {s.self with name} in
+  {s with self}
+
+let names_of_subs subs =


Why are you using hashtables? We prefer to use pure functional data types, i.e., map, and I so far do not see any justifications to use mutable tables here.

Justification is in elements order in an array of subroutines. If I use map, I would
have to revert a result of Array.fold , but definitely not to use Array.map

ivg · 2018-04-09T12:01:33Z

lib/bap_types/bap_ir.ml

+
+let fix_names ?(old_names=String.Table.create ()) subs =
+  let names = names_of_subs subs in
+  let conflicts = Hashtbl.count names ~f:(fun x -> List.length x > 1) in


fun x -> List.length x > 1 is in general very inefficient pattern. You do not need to know the full length of the list, to check that it is greater than 1, just do match xs with | [] | [_] -> true | _ -> false and you will go from linear to O(1) algorithm.

ivg · 2018-04-09T12:19:11Z

lib/bap_types/bap_ir.ml

+  let conflicts = Hashtbl.count names ~f:(fun x -> List.length x > 1) in
+  if conflicts = 0 then subs
+  else
+    let remove name tid = match Hashtbl.find names name with


it is a bad style to put a nested function in the else branch. Are those function has to defined only if the precondition holds? Please, move them out the function and define at the top-level. If you're concerned with a namespace pollution, then put them in a module.

ivg · 2018-04-09T12:22:24Z

lib/bap_types/bap_ir.ml

+  let conflicts = Hashtbl.count names ~f:(fun x -> List.length x > 1) in
+  if conflicts = 0 then subs
+  else
+    let remove name tid = match Hashtbl.find names name with


instead of using find/set you can use change. Though, this code already is too bad to be discussed at all...

ivg · 2018-04-09T12:27:07Z

lib/bap_types/bap_ir.ml

+        | tids ->
+          if is_old sub then mangle sub
+          else
+            let max_tid = Option.value_exn


There should not be any dependence on the tid ordering. The order relation between two tids has nothing to do with a corresponding term freshness or anything. We may switch later to uuid as tids and assign them randomly. So please, remove any dependencies.

let me quote you:

If both names are new, then pick the rename a name that corresponds to a tid that is smaller.

ivg

don't change program if not needed.

ivg · 2018-04-09T20:05:30Z

lib/bap_types/bap_ir.ml

+          if is_new sub.tid sub.self.name then
+            keep_name tids sub.self.name sub.tid
+          else tids) in
+    Array.map news ~f:(fun sub ->


Array.map will create a new array and copy the contents of the old array even if the f function is ident:

let xs = Array.init 8 ~f:ident;; val xs : int Core_kernel.Array.t = [|0; 1; 2; 3; 4; 5; 6; 7|] # let ys = Array.map xs ~f:ident;; val ys : int Core_kernel.Array.t = [|0; 1; 2; 3; 4; 5; 6; 7|] # phys_equal xs ys;; - : bool = false

In our case, it will result in double copying of the whole program even if the change was benign and didn't break the naming injectivity.

So you need to prefix the map function with the check if Array.length news <> Map.length tids then Array.map ... else news

gitoleg added 2 commits April 4, 2018 14:10

first approach to mangling duplicated subroutines

a3b151a

Implemented like an iteration over existed subroutines

refactoring

8de5ebc

gitoleg requested a review from ivg April 4, 2018 20:30

ivg requested changes Apr 4, 2018

View reviewed changes

ivg self-assigned this Apr 4, 2018

gitoleg added 2 commits April 5, 2018 11:01

work in progress: all checks are on program type

6dcad5f

made the program type private

9e357f9

ivg requested changes Apr 5, 2018

View reviewed changes

gitoleg added 2 commits April 5, 2018 15:23

Merge remote-tracking branch 'upstream/master' into sub-mangling

f3ac374

changes after review

0a9e67a

gitoleg force-pushed the sub-mangling branch from af99587 to 0a9e67a Compare April 5, 2018 19:29

ivg requested changes Apr 9, 2018

View reviewed changes

gitoleg added 3 commits April 9, 2018 12:16

reimplemented all with Map

1965534

placed everything in Program module

6655a52

refactored

0d76045

ivg requested changes Apr 9, 2018

View reviewed changes

added a check if nothing should be changed

faf3fb4

ivg approved these changes Apr 9, 2018

View reviewed changes

ivg merged commit 6dbaa2d into BinaryAnalysisPlatform:master Apr 10, 2018

gitoleg deleted the sub-mangling branch April 17, 2018 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds mangling of duplicated subroutines #813

adds mangling of duplicated subroutines #813

gitoleg commented Apr 4, 2018

ivg Apr 4, 2018

ivg Apr 4, 2018

ivg Apr 4, 2018

ivg Apr 4, 2018

ivg Apr 4, 2018

ivg commented Apr 4, 2018

ivg left a comment •

edited

Loading

ivg Apr 5, 2018

ivg Apr 5, 2018

ivg Apr 5, 2018

ivg Apr 5, 2018

ivg left a comment

ivg Apr 9, 2018

ivg Apr 9, 2018

gitoleg Apr 9, 2018 •

edited

Loading

ivg Apr 9, 2018

ivg Apr 9, 2018 •

edited

Loading

ivg Apr 9, 2018

ivg Apr 9, 2018

gitoleg Apr 9, 2018

ivg left a comment

ivg Apr 9, 2018

adds mangling of duplicated subroutines #813

adds mangling of duplicated subroutines #813

Conversation

gitoleg commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivg commented Apr 4, 2018

ivg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitoleg Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivg Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivg left a comment •

edited

Loading

gitoleg Apr 9, 2018 •

edited

Loading

ivg Apr 9, 2018 •

edited

Loading