Rename the complex feature set into the return access path set

Summary:
The complex feature set was probably meant to contain different features, but it ended up containing only return access paths.
It currently has custom logic for widening access paths, which wouldn't handle other kind of features.
Let's rename it as `ReturnAccessPathSet` to prevent any confusion.

Reviewed By: dkgi

Differential Revision: D28983928

fbshipit-source-id: b40ed09052840ae0ad685a733323b454674d6e87

Author: arthaud

source/interprocedural_analyses/taint/backwardAnalysis.ml

@@ -88,13 +88,10 @@ module AnalysisInstance (FunctionContext : FUNCTION_CONTEXT) = struct
 
 
   let transform_non_leaves path taint =
-    let f feature =
-      match feature with
-      | Features.Complex.ReturnAccessPath prefix -> Features.Complex.ReturnAccessPath (prefix @ path)
-    in
+    let f prefix = prefix @ path in
     match path with
     | Abstract.TreeDomain.Label.AnyIndex :: _ -> taint
-    | _ -> BackwardTaint.transform BackwardTaint.complex_feature Map ~f taint
+    | _ -> BackwardTaint.transform Features.ReturnAccessPathSet.Element Map ~f taint
 
 
   let read_tree = BackwardState.Tree.read ~transform_non_leaves
@@ -197,13 +194,10 @@ module AnalysisInstance (FunctionContext : FUNCTION_CONTEXT) = struct
               let extra_paths =
                 match kind with
                 | Sinks.LocalReturn ->
-                    let gather_paths (Features.Complex.ReturnAccessPath extra_path) paths =
-                      extra_path :: paths
-                    in
                     BackwardTaint.fold
-                      BackwardTaint.complex_feature
+                      Features.ReturnAccessPathSet.Element
                       element
-                      ~f:gather_paths
+                      ~f:List.cons
                       ~init:[]
                 | _ ->
                     (* No special path handling for side effect taint *)
@@ -1372,7 +1366,7 @@ let extract_tito_and_sink_models define ~is_constructor ~resolution ~existing_ba
     TaintConfiguration.analysis_model_constraints =
       {
         maximum_model_width;
-        maximum_complex_access_path_length;
+        maximum_return_access_path_length;
         maximum_trace_length;
         maximum_tito_depth;
         _;
@@ -1405,7 +1399,7 @@ let extract_tito_and_sink_models define ~is_constructor ~resolution ~existing_ba
     |> BackwardState.Tree.limit_to
          ~transform:(BackwardTaint.add_features Features.widen_broadening)
          ~width:maximum_model_width
-    |> BackwardState.Tree.approximate_complex_access_paths ~maximum_complex_access_path_length
+    |> BackwardState.Tree.approximate_return_access_paths ~maximum_return_access_path_length
   in
 
   let split_and_simplify model (parameter, name, original) =

source/interprocedural_analyses/taint/domains.ml

@@ -187,7 +187,7 @@ module FlowDetails = struct
 
     type 'a slot =
       | SimpleFeature : Features.SimpleSet.t slot
-      | ComplexFeature : Features.ComplexSet.t slot
+      | ReturnAccessPath : Features.ReturnAccessPathSet.t slot
       | TraceLength : TraceLength.t slot
       | TitoPosition : Features.TitoPositionSet.t slot
       | LeafName : Features.LeafNameSet.t slot
@@ -200,7 +200,7 @@ module FlowDetails = struct
     let slot_name (type a) (slot : a slot) =
       match slot with
       | SimpleFeature -> "SimpleFeature"
-      | ComplexFeature -> "ComplexFeature"
+      | ReturnAccessPath -> "ReturnAccessPath"
       | TraceLength -> "TraceLength"
       | TitoPosition -> "TitoPosition"
       | LeafName -> "LeafName"
@@ -211,7 +211,7 @@ module FlowDetails = struct
     let slot_domain (type a) (slot : a slot) =
       match slot with
       | SimpleFeature -> (module Features.SimpleSet : Abstract.Domain.S with type t = a)
-      | ComplexFeature -> (module Features.ComplexSet : Abstract.Domain.S with type t = a)
+      | ReturnAccessPath -> (module Features.ReturnAccessPathSet : Abstract.Domain.S with type t = a)
       | TraceLength -> (module TraceLength : Abstract.Domain.S with type t = a)
       | TitoPosition -> (module Features.TitoPositionSet : Abstract.Domain.S with type t = a)
       | LeafName -> (module Features.LeafNameSet : Abstract.Domain.S with type t = a)
@@ -241,10 +241,6 @@ module FlowDetails = struct
 
   let simple_feature_self = Features.SimpleSet.Self
 
-  let complex_feature = Features.ComplexSet.Element
-
-  let complex_feature_set = Features.ComplexSet.Self
-
   let tito_position_element = Features.TitoPositionSet.Element
 
   let product_pp = pp (* shadow *)
@@ -287,10 +283,6 @@ module type TAINT_DOMAIN = sig
 
   val simple_feature_self : Features.SimpleSet.t Abstract.Domain.part
 
-  val complex_feature : Features.Complex.t Abstract.Domain.part
-
-  val complex_feature_set : Features.ComplexSet.t Abstract.Domain.part
-
   val first_indices : Features.FirstIndexSet.t Abstract.Domain.part
 
   val first_fields : Features.FirstFieldSet.t Abstract.Domain.part
@@ -383,10 +375,6 @@ end = struct
 
   let simple_feature_element = FlowDetails.simple_feature_element
 
-  let complex_feature = FlowDetails.complex_feature
-
-  let complex_feature_set = FlowDetails.complex_feature_set
-
   let first_fields = Features.FirstFieldSet.Self
 
   let first_indices = Features.FirstIndexSet.Self
@@ -411,12 +399,10 @@ end = struct
               let breadcrumb_json = Features.Breadcrumb.to_json breadcrumb ~on_all_paths:in_under in
               breadcrumb_json :: breadcrumbs
         in
-        let gather_return_access_path feature leaves =
-          match feature with
-          | Features.Complex.ReturnAccessPath path ->
-              let path_name = Abstract.TreeDomain.Label.show_path path in
-              `Assoc ["kind", leaf_kind_json; "name", `String path_name; "depth", `Int trace_length]
-              :: leaves
+        let gather_return_access_path path leaves =
+          let path_name = Abstract.TreeDomain.Label.show_path path in
+          `Assoc ["kind", leaf_kind_json; "name", `String path_name; "depth", `Int trace_length]
+          :: leaves
         in
         let breadcrumbs =
           FlowDetails.(fold simple_feature_element ~f:gather_json ~init:[] features)
@@ -437,7 +423,12 @@ end = struct
           |> Features.FirstField.to_json
         in
         ( List.concat [first_index_breadcrumbs; first_field_breadcrumbs; breadcrumbs],
-          FlowDetails.(fold complex_feature ~f:gather_return_access_path ~init:leaves features) )
+          FlowDetails.(
+            fold
+              Features.ReturnAccessPathSet.Element
+              ~f:gather_return_access_path
+              ~init:leaves
+              features) )
       in
       let tito_positions =
         FlowDetails.get FlowDetails.Slots.TitoPosition features
@@ -587,39 +578,39 @@ module MakeTaintTree (Taint : TAINT_DOMAIN) () = struct
 
   let is_empty = is_bottom
 
-  let compute_essential_features ~essential_complex_features tree =
+  let compute_essential_features ~essential_return_access_paths tree =
     let essential_trace_info = function
       | _ -> TraceInfo.Declaration { leaf_name_provided = false }
     in
     let essential_simple_features _ = Features.SimpleSet.bottom in
     let essential_tito_positions _ = Features.TitoPositionSet.bottom in
     let essential_leaf_names _ = Features.LeafNameSet.bottom in
     transform Taint.trace_info Map ~f:essential_trace_info tree
-    |> transform Features.ComplexSet.Self Map ~f:essential_complex_features
+    |> transform Features.ReturnAccessPathSet.Self Map ~f:essential_return_access_paths
     |> transform Features.SimpleSet.Self Map ~f:essential_simple_features
     |> transform Features.TitoPositionSet.Self Map ~f:essential_tito_positions
     |> transform Features.LeafNameSet.Self Map ~f:essential_leaf_names
 
 
   (* Keep only non-essential structure. *)
   let essential tree =
-    let essential_complex_features _ = Features.ComplexSet.bottom in
-    compute_essential_features ~essential_complex_features tree
+    let essential_return_access_paths _ = Features.ReturnAccessPathSet.bottom in
+    compute_essential_features ~essential_return_access_paths tree
 
 
   let essential_for_constructor tree =
-    let essential_complex_features set = set in
-    compute_essential_features ~essential_complex_features tree
+    let essential_return_access_paths set = set in
+    compute_essential_features ~essential_return_access_paths tree
 
 
-  let approximate_complex_access_paths ~maximum_complex_access_path_length tree =
+  let approximate_return_access_paths ~maximum_return_access_path_length tree =
     let cut_off features =
-      if Features.ComplexSet.count features > maximum_complex_access_path_length then
-        Features.ComplexSet.singleton (Features.Complex.ReturnAccessPath [])
+      if Features.ReturnAccessPathSet.count features > maximum_return_access_path_length then
+        Features.ReturnAccessPathSet.singleton []
       else
         features
     in
-    transform Taint.complex_feature_set Map ~f:cut_off tree
+    transform Features.ReturnAccessPathSet.Self Map ~f:cut_off tree
 
 
   let prune_maximum_length maximum_length =
@@ -742,6 +733,6 @@ let local_return_taint =
       Part (BackwardTaint.trace_info, TraceInfo.Declaration { leaf_name_provided = false });
       Part (BackwardTaint.leaf, Sinks.LocalReturn);
       Part (TraceLength.Self, 0);
-      Part (BackwardTaint.complex_feature, Features.Complex.ReturnAccessPath []);
+      Part (Features.ReturnAccessPathSet.Element, []);
       Part (Features.SimpleSet.Self, Features.SimpleSet.empty);
     ]

source/interprocedural_analyses/taint/features.ml

@@ -179,34 +179,26 @@ end
 
 module SimpleSet = Abstract.OverUnderSetDomain.Make (Simple)
 
-(* Set of complex features, where element can be abstracted and joins are expensive. Should only be
-   used for elements that need this kind of joining. *)
-module Complex = struct
-  let name = "complex features"
+module ReturnAccessPath = struct
+  let name = "return access paths"
 
-  type t = ReturnAccessPath of Abstract.TreeDomain.Label.path
-  [@@deriving show { with_path = false }, compare]
-
-  let less_or_equal ~left ~right =
-    match left, right with
-    | ReturnAccessPath left_path, ReturnAccessPath right_path ->
-        Abstract.TreeDomain.Label.is_prefix ~prefix:right_path left_path
+  type t = Abstract.TreeDomain.Label.path [@@deriving show { with_path = false }, compare]
 
+  let less_or_equal ~left ~right = Abstract.TreeDomain.Label.is_prefix ~prefix:right left
 
   let widen set =
     let truncate = function
-      | ReturnAccessPath p when List.length p > TaintConfiguration.maximum_return_access_path_depth
-        ->
-          ReturnAccessPath (List.take p TaintConfiguration.maximum_return_access_path_depth)
+      | p when List.length p > TaintConfiguration.maximum_return_access_path_depth ->
+          List.take p TaintConfiguration.maximum_return_access_path_depth
       | x -> x
     in
     if List.length set > TaintConfiguration.maximum_return_access_path_width then
-      [ReturnAccessPath []]
+      [[]]
     else
       List.map ~f:truncate set
 end
 
-module ComplexSet = Abstract.ElementSetDomain.Make (Complex)
+module ReturnAccessPathSet = Abstract.ElementSetDomain.Make (ReturnAccessPath)
 
 let obscure = Simple.Breadcrumb Breadcrumb.Obscure
 

source/interprocedural_analyses/taint/forwardAnalysis.ml

@@ -232,13 +232,10 @@ module AnalysisInstance (FunctionContext : FUNCTION_CONTEXT) = struct
               let return_paths =
                 match kind with
                 | Sinks.LocalReturn ->
-                    let gather_paths (Features.Complex.ReturnAccessPath extra_path) paths =
-                      extra_path :: paths
-                    in
                     BackwardTaint.fold
-                      BackwardTaint.complex_feature
+                      Features.ReturnAccessPathSet.Element
                       return_taint
-                      ~f:gather_paths
+                      ~f:List.cons
                       ~init:[]
                 | _ ->
                     (* No special handling of paths for side effects *)
@@ -1574,7 +1571,7 @@ let extract_source_model ~define ~resolution ~features_to_attach exit_taint =
   let return_type_breadcrumbs = Features.type_breadcrumbs ~resolution return_annotation in
   let {
     TaintConfiguration.analysis_model_constraints =
-      { maximum_model_width; maximum_complex_access_path_length; maximum_trace_length; _ };
+      { maximum_model_width; maximum_return_access_path_length; maximum_trace_length; _ };
     _;
   }
     =
@@ -1600,7 +1597,7 @@ let extract_source_model ~define ~resolution ~features_to_attach exit_taint =
     |> ForwardState.Tree.limit_to
          ~transform:(ForwardTaint.add_features Features.widen_broadening)
          ~width:maximum_model_width
-    |> ForwardState.Tree.approximate_complex_access_paths ~maximum_complex_access_path_length
+    |> ForwardState.Tree.approximate_return_access_paths ~maximum_return_access_path_length
   in
   let attach_features taint =
     if not (Features.SimpleSet.is_bottom features_to_attach) then

source/interprocedural_analyses/taint/model.ml

@@ -336,10 +336,9 @@ let infer_class_models ~environment =
   let fold_taint position existing_state attribute =
     let leaf =
       BackwardState.Tree.create_leaf (BackwardTaint.singleton Sinks.LocalReturn)
-      |> BackwardState.Tree.transform BackwardTaint.complex_feature_set Map ~f:(fun _ ->
-             Features.ComplexSet.singleton
-               (Features.Complex.ReturnAccessPath
-                  [Abstract.TreeDomain.Label.create_name_index attribute]))
+      |> BackwardState.Tree.transform Features.ReturnAccessPathSet.Self Map ~f:(fun _ ->
+             Features.ReturnAccessPathSet.singleton
+               [Abstract.TreeDomain.Label.create_name_index attribute])
     in
     BackwardState.assign
       ~root:

source/interprocedural_analyses/taint/taintConfiguration.ml

@@ -44,7 +44,7 @@ let empty_implicit_sources = { literal_strings = [] }
 
 type analysis_model_constraints = {
   maximum_model_width: int;
-  maximum_complex_access_path_length: int;
+  maximum_return_access_path_length: int;
   maximum_overrides_to_analyze: int option;
   maximum_trace_length: int option;
   maximum_tito_depth: int option;
@@ -53,7 +53,7 @@ type analysis_model_constraints = {
 let default_analysis_model_constraints =
   {
     maximum_model_width = 25;
-    maximum_complex_access_path_length = 10;
+    maximum_return_access_path_length = 10;
     maximum_overrides_to_analyze = None;
     maximum_trace_length = None;
     maximum_tito_depth = None;

source/interprocedural_analyses/taint/taintConfiguration.mli

@@ -38,7 +38,7 @@ type implicit_sources = { literal_strings: literal_string_source list }
 
 type analysis_model_constraints = {
   maximum_model_width: int;
-  maximum_complex_access_path_length: int;
+  maximum_return_access_path_length: int;
   maximum_overrides_to_analyze: int option;
   maximum_trace_length: int option;
   maximum_tito_depth: int option;

source/interprocedural_analyses/taint/test/domainTest.ml

@@ -59,8 +59,8 @@ let test_partition_call_map _ =
   assert_equal ~msg:"matches must be equal to original" ~printer:ForwardTaint.show matches joined
 
 
-let test_approximate_complex_access_paths _ =
-  let assert_approximate_complex_access_paths ~expected ~cutoff_at tree =
+let test_approximate_return_access_paths _ =
+  let assert_approximate_return_access_paths ~expected ~cutoff_at tree =
     let compare left right =
       ForwardState.Tree.less_or_equal ~left ~right
       && ForwardState.Tree.less_or_equal ~left:right ~right:left
@@ -69,51 +69,44 @@ let test_approximate_complex_access_paths _ =
       ~cmp:compare
       ~printer:ForwardState.Tree.show
       expected
-      (ForwardState.Tree.approximate_complex_access_paths
-         ~maximum_complex_access_path_length:cutoff_at
+      (ForwardState.Tree.approximate_return_access_paths
+         ~maximum_return_access_path_length:cutoff_at
          tree)
   in
-  let create ~features =
+  let create ~return_access_paths =
     ForwardState.Tree.create_leaf (ForwardTaint.singleton (Sources.NamedSource "Demo"))
-    |> ForwardState.Tree.transform ForwardTaint.complex_feature_set Map ~f:(fun _ ->
-           Features.ComplexSet.of_list features)
+    |> ForwardState.Tree.transform Features.ReturnAccessPathSet.Self Map ~f:(fun _ ->
+           Features.ReturnAccessPathSet.of_list return_access_paths)
   in
-  assert_approximate_complex_access_paths
-    ~expected:
-      (create ~features:[Features.Complex.ReturnAccessPath [Abstract.TreeDomain.Label.Index "a"]])
+  assert_approximate_return_access_paths
+    ~expected:(create ~return_access_paths:[[Abstract.TreeDomain.Label.Index "a"]])
     ~cutoff_at:2
-    (create ~features:[Features.Complex.ReturnAccessPath [Abstract.TreeDomain.Label.Index "a"]]);
-  assert_approximate_complex_access_paths
+    (create ~return_access_paths:[[Abstract.TreeDomain.Label.Index "a"]]);
+  assert_approximate_return_access_paths
     ~expected:
       (create
-         ~features:
-           [
-             Features.Complex.ReturnAccessPath [Abstract.TreeDomain.Label.Index "a"];
-             Features.Complex.ReturnAccessPath [Abstract.TreeDomain.Label.Index "b"];
-           ])
+         ~return_access_paths:
+           [[Abstract.TreeDomain.Label.Index "a"]; [Abstract.TreeDomain.Label.Index "b"]])
     ~cutoff_at:2
     (create
-       ~features:
-         [
-           Features.Complex.ReturnAccessPath [Abstract.TreeDomain.Label.Index "a"];
-           Features.Complex.ReturnAccessPath [Abstract.TreeDomain.Label.Index "b"];
-         ]);
-  assert_approximate_complex_access_paths
-    ~expected:(create ~features:[Features.Complex.ReturnAccessPath []])
+       ~return_access_paths:
+         [[Abstract.TreeDomain.Label.Index "a"]; [Abstract.TreeDomain.Label.Index "b"]]);
+  assert_approximate_return_access_paths
+    ~expected:(create ~return_access_paths:[[]])
     ~cutoff_at:2
     (create
-       ~features:
+       ~return_access_paths:
          [
-           Features.Complex.ReturnAccessPath [Abstract.TreeDomain.Label.Index "a"];
-           Features.Complex.ReturnAccessPath [Abstract.TreeDomain.Label.Index "b"];
-           Features.Complex.ReturnAccessPath [Abstract.TreeDomain.Label.Index "c"];
+           [Abstract.TreeDomain.Label.Index "a"];
+           [Abstract.TreeDomain.Label.Index "b"];
+           [Abstract.TreeDomain.Label.Index "c"];
          ])
 
 
 let () =
   "taint_domain"
   >::: [
          "partition_call_map" >:: test_partition_call_map;
-         "approximate_complex_access_paths" >:: test_approximate_complex_access_paths;
+         "approximate_return_access_paths" >:: test_approximate_return_access_paths;
        ]
   |> Test.run