[siof] add --siof-safe-methods whitelisting option

Summary:
This gives a way for users to flag safe methods regarding SIOF, for instance if
the problematic paths in the method cannot happen before `main()` has started.

Reviewed By: akotulski

Differential Revision: D4578700

fbshipit-source-id: 6542dcf

.inferconfig

@@ -1,4 +1,6 @@
 {
+  "siof-safe-methods": ["getGlobalNonPODWhitelisted", "whitelisted::getGlobalNonPOD",
+                        "whitelisted::TemplatedObject::getGlobalNonPOD"],
   "skip-translation": [
     {
       "language": "Java",

infer/src/IR/Procname.re

@@ -574,3 +574,52 @@ let module Set = Caml.Set.Make {
 
 /** Pretty print a set of proc names */
 let pp_set fmt set => Set.iter (fun pname => F.fprintf fmt "%a " pp pname) set;
+
+let fuzzy_qualifiers_equal fuzzy_qualifiers::fuzzy_qualifiers qualifiers => {
+  let qual_equal q1 q2 => {
+    /* qual_name may have qualifiers with template parameters -
+       drop them to whitelist all instantiations */
+    let no_template_name s => List.hd_exn (String.split on::'<' s);
+    String.equal (no_template_name q1) (no_template_name q2)
+  };
+  let is_std_qual = String.equal "std";
+  switch fuzzy_qualifiers {
+  | [first, ...rest] when is_std_qual first =>
+    /* add special handling for std:: namespace to avoid problems with inconsistent
+       inline namespaces (such as __1 in libc++) */
+    List.hd qualifiers |> Option.value_map default::false f::is_std_qual &&
+    List.is_prefix (List.rev qualifiers) prefix::(List.rev rest) equal::qual_equal
+  | _ => List.equal equal::qual_equal fuzzy_qualifiers qualifiers
+  }
+};
+
+/* This is simplistic and will give the wrong answer in some cases, eg
+   "foo<bar::baz<goo>>::someMethod" will get parsed as ["foo<bar", "baz<goo>>",
+   "someMethod"]. Ideally, we would keep the list of qualifiers in the procname, which would save us
+   from having to properly parse them. */
+let qualifiers_of_qual_name = {
+  let class_sep_regex = Str.regexp_string "::";
+  /* wait until here to define the function so that [class_sep_regex] is only computed once */
+  Str.split class_sep_regex
+};
+
+let qualifiers_of_fuzzy_qual_name qual_name => {
+  /* Fail if we detect templates in the fuzzy name. Template instantiations are not taken into
+     account when fuzzy matching, and templates may produce wrong results when parsing qualified
+     names. */
+  if (String.contains qual_name '<') {
+    failwithf "Unexpected template in fuzzy qualified name %s." qual_name
+  };
+  qualifiers_of_qual_name qual_name
+};
+
+let fuzzy_equal fuzzy_qualifiers::fuzzy_qualifiers pname => {
+  let qualifiers =
+    switch pname {
+    | C c => fst c |> qualifiers_of_qual_name
+    | ObjC_Cpp objc_cpp =>
+      List.append (qualifiers_of_qual_name objc_cpp.class_name) [objc_cpp.method_name]
+    | _ => []
+    };
+  fuzzy_qualifiers_equal fuzzy_qualifiers::fuzzy_qualifiers qualifiers
+};

infer/src/IR/Procname.rei

@@ -275,3 +275,38 @@ let to_unique_id: t => string;
 
 /** Convert a proc name to a filename. */
 let to_filename: t => string;
+
+
+/** Return whether two qualified C++ procnames match up to namescapes and templating. In particular,
+     this deals with the following issues:
+
+     1. 'std::' namespace may have inline namespace afterwards: std::move becomes std::__1::move. This
+         happens on libc++ and to some extent on libstdc++. To work around this problem, make matching
+         against 'std::' more fuzzier: std::X::Y::Z will match std::.*::X::Y::Z (but only for the
+         'std' namespace).
+
+     2. The names are allowed not to commit to a template specialization: we want std::move to match
+        std::__1::move<const X&> and std::__1::move<int>. To do so, comparison function for qualifiers
+        will ignore template specializations.
+
+        For example:
+        ["std", "move"]:
+        matches: ["std", "blah", "blah<int>","move"]
+        does not match: ["std","blah", "move", "BAD"] - we don't want std::.*::X::.* to pass
+        does not match: ["stdBAD", "move"], - it's not std namespace anymore
+
+        ["folly", "someFunction"]
+        matches: ["folly","someFunction"]
+        matches: ["folly","someFunction<int>"]
+        matches: ["folly<int>","someFunction"]
+        does not match: ["folly", "BAD", "someFunction"] - unlike 'std' any other namespace needs all
+                                                           qualifiers to match
+        does not match: ["folly","someFunction<int>", "BAD"] - same as previous example
+    */
+let fuzzy_equal: fuzzy_qualifiers::list string => t => bool;
+
+let fuzzy_qualifiers_equal: fuzzy_qualifiers::list string => list string => bool;
+
+
+/** parse the argument into a list::of::qualifiers::without::templates */
+let qualifiers_of_fuzzy_qual_name: string => list string;

infer/src/base/CommandLineOption.ml

@@ -55,11 +55,13 @@ let warnf =
   else if not is_originator then fun fmt -> F.ifprintf F.err_formatter fmt
   else F.eprintf
 
-type section = Analysis | BufferOverrun | Clang | Crashcontext | Driver | Java | Print | Quandary
+type section =
+    Analysis | BufferOverrun | Checkers | Clang | Crashcontext | Driver | Java | Print | Quandary
 [@@deriving compare]
 
 let equal_section = [%compare.equal : section ]
-let all_sections = [ Analysis; BufferOverrun; Clang; Crashcontext; Driver; Java; Print; Quandary ]
+let all_sections =
+  [ Analysis; BufferOverrun; Checkers; Clang; Crashcontext; Driver; Java; Print; Quandary ]
 
 type 'a parse = Infer of 'a | Javac | NoParse [@@deriving compare]
 
@@ -78,7 +80,8 @@ let to_parse_tag = function | Infer _ -> Infer () | Javac -> Javac | NoParse ->
 
 let accept_unknown_args = function
   | Infer Print | Javac | NoParse -> true
-  | Infer (Analysis | BufferOverrun | Clang | Crashcontext | Driver | Java | Quandary) -> false
+  | Infer (Analysis | BufferOverrun | Checkers | Clang | Crashcontext | Driver | Java | Quandary) ->
+      false
 
 type desc = {
   long: string; short: string; meta: string; doc: string; spec: spec;
@@ -640,6 +643,7 @@ let set_curr_speclist_for_parse_action ~incomplete ~usage parse_action =
   curr_speclist := [];
   if equal_parse_action parse_action (Infer Driver) then (
     add_to_curr_speclist ~add_help:true ~header:"Driver options" (Infer Driver);
+    add_to_curr_speclist ~header:"Checkers options" (Infer Checkers);
     add_to_curr_speclist ~header:"Clang-specific options" (Infer Clang);
     add_to_curr_speclist ~header:"Java-specific options" (Infer Java);
     add_to_curr_speclist ~header:"Quandary checker options" (Infer Quandary)

infer/src/base/CommandLineOption.mli

@@ -12,7 +12,8 @@
 open! IStd
 
 (** a section is a part of infer that can be affected by an infer option *)
-type section = Analysis | BufferOverrun | Clang | Crashcontext | Driver | Java | Print | Quandary
+type section =
+    Analysis | BufferOverrun | Checkers | Clang | Crashcontext | Driver | Java | Print | Quandary
 [@@deriving compare]
 
 val all_sections : section list

infer/src/base/Config.ml

@@ -222,17 +222,17 @@ let use_jar_cache = true
 let weak = "<\"Weak\">"
 
 let whitelisted_cpp_methods = [
-  ["std"; "move"];
+  "std::move";
-  ["std"; "forward"];
+  "std::forward";
-  ["std"; "min"];
+  "std::min";
-  ["std"; "max"];
+  "std::max";
-  ["google"; "CheckNotNull"];
+  "google::CheckNotNull";
 ]
 
 let whitelisted_cpp_classes = [
-  ["std"; "__less"];
+  "std::__less";
-  ["std"; "__wrap_iter"]; (* libc++ internal name of vector iterator *)
+  "std::__wrap_iter"; (* libc++ internal name of vector iterator *)
-  ["std"; "__normal_iterator"]; (* libstdc++ internal name of vector iterator *)
+  "std::__normal_iterator"; (* libstdc++ internal name of vector iterator *)
 ]
 
 type dynamic_dispatch_policy = [
@@ -1151,6 +1151,12 @@ and seconds_per_iteration =
   CLOpt.mk_float_opt ~deprecated:["seconds_per_iteration"] ~long:"seconds-per-iteration"
     ~meta:"float" "Set the number of seconds per iteration (see --iterations)"
 
+and siof_safe_methods =
+  CLOpt.mk_string_list ~long:"siof-safe-methods"
+    ~parse_mode:CLOpt.(Infer [Checkers])
+    "Methods that are SIOF-safe; \"foo::bar\" will match \"foo::bar()\", \"foo<int>::bar()\", \
+     etc. (can be specified multiple times)"
+
 and skip_analysis_in_path =
   CLOpt.mk_string_list ~deprecated:["-skip-clang-analysis-in-path"] ~long:"skip-analysis-in-path"
     ~parse_mode:CLOpt.(Infer [Driver])
@@ -1575,6 +1581,7 @@ and save_analysis_results = !save_results
 and seconds_per_iteration = !seconds_per_iteration
 and show_buckets = !print_buckets
 and show_progress_bar = !progress_bar
+and siof_safe_methods = !siof_safe_methods
 and skip_analysis_in_path = !skip_analysis_in_path
 and skip_translation_headers = !skip_translation_headers
 and sources = !sources

infer/src/base/Config.mli

@@ -130,8 +130,8 @@ val unsafe_unret : string
 val use_jar_cache : bool
 val version_string : string
 val weak : string
-val whitelisted_cpp_methods : string list list
+val whitelisted_cpp_methods : string list
-val whitelisted_cpp_classes : string list list
+val whitelisted_cpp_classes : string list
 val wrappers_dir : string
 
 
@@ -266,6 +266,7 @@ val save_analysis_results : string option
 val seconds_per_iteration : float option
 val show_buckets : bool
 val show_progress_bar : bool
+val siof_safe_methods : string list
 val skip_analysis_in_path : string list
 val skip_translation_headers : string list
 val spec_abs_level : int

infer/src/base/Logging.ml

@@ -30,7 +30,7 @@ let dup_formatter fmt1 fmt2 =
 
 (** Name of dir for logging the output in the specific executable *)
 let log_dir_of_action (action : CLOpt.parse_action) = match action with
-  | Infer (Analysis | BufferOverrun | Crashcontext | Quandary) -> "analyze"
+  | Infer (Analysis | BufferOverrun | Checkers | Crashcontext | Quandary) -> "analyze"
   | Infer Driver -> "driver"
   | Infer Clang
   | Infer Java

infer/src/checkers/Siof.ml

@@ -14,6 +14,15 @@ module L = Logging
 
 module GlobalsAccesses = SiofTrace.GlobalsAccesses
 
+let whitelisted_models =
+  List.map Config.siof_safe_methods ~f:Procname.qualifiers_of_fuzzy_qual_name
+
+let is_whitelisted (pname : Procname.t) =
+  (* This is linear in the number of whitelisted models, which is not good if there are many
+     models... *)
+  List.exists whitelisted_models
+    ~f:(fun fuzzy_qualifiers -> Procname.fuzzy_equal pname ~fuzzy_qualifiers)
+
 module Summary = Summary.Make (struct
     type summary = SiofDomain.astate
 
@@ -77,6 +86,8 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
     | Prune (exp, loc, _, _) ->
         let proc_loc = Procdesc.get_loc pdesc in
         get_globals pdesc loc exp |> add_globals astate proc_loc
+    | Call (_, Const (Cfun callee_pname), _, _, _) when is_whitelisted callee_pname ->
+        at_least_nonbottom astate
     | Call (_, Const (Cfun callee_pname), _::params_without_self, loc, _)
       when Procname.is_c_method callee_pname && Procname.is_constructor callee_pname
            && Procname.is_constexpr callee_pname ->

infer/src/clang/cFrontend_decl.ml

@@ -137,56 +137,21 @@ struct
   let process_methods trans_unit_ctx tenv cg cfg curr_class decl_list =
     IList.iter (process_one_method_decl trans_unit_ctx tenv cg cfg curr_class) decl_list
 
-  (** Given qualified name list and whitelist (list of list of qualifiers), return whether
-      qualified name matches whitelist. There are some complications due to following constraints:
-      1. 'std::' namespace may have inline namespace afterwards - std::move becomes std::__1::move
-         This happens on libc++ and to some extent on libstdc++. To workaround this problem, make
-         matching against 'std::' whitelists more fuzzy:
-          std::X::Y::Z will match std::.*::X::Y::Z (but only for 'std' namespace)
-      2. whitelist is specified without template specializations, but we want std::move to
-         match std::__1::move<const X&> and std::__1::move<int>. To do so, comparison function
-         for qualifiers will ignore template specializations.
-
-      For example:
-      Whitelist:
-        [["std", "move"], ["folly", "someFunction"]]
-      Passing qualifiers:
-        ["std", "blah", "blah<int>","move"]
-        ["folly","someFunction"]
-        ["folly","someFunction<int>"]
-        ["folly<int>","someFunction"]
-      NOT passing qualifiers:
-        ["std","blah", "move", "BAD"] - we don't want std::.*::X::.* to pass
-        ["stdBAD", "move"], - it's not std namespace anymore
-        ["folly", "BAD", "someFunction"] - unlike 'std' any other namespace needs all quals equal
-        ["folly","someFunction<int>", "BAD"] - same as previous example
-  *)
   let is_whitelisted_qual_name qual_name whitelist =
-    let qual_equal q1 q2 =
+    List.exists whitelist
-      (* qual_name may have qualifiers with template parameters -
+      ~f:(fun fuzzy_qualifiers -> Procname.fuzzy_qualifiers_equal ~fuzzy_qualifiers qual_name)
-         drop them to whitelist all instantiations *)
-      let no_template_name s = List.hd_exn (String.split ~on:'<' s) in
-      String.equal (no_template_name q1) (no_template_name q2) in
-    let is_std_qual = String.equal "std" in
-    let method_matches whitelisted_method =
-      match whitelisted_method with
-      | first :: rest when is_std_qual first ->
-          (* add special handling for std:: namespace to avoid problems with inconsistent
-             inline namespaces (such as __1 in libc++) *)
-          List.hd qual_name |> Option.value_map ~default:false ~f:is_std_qual
-          && List.is_prefix (List.rev qual_name) ~prefix:(List.rev rest) ~equal:qual_equal
-      | _ -> List.equal ~equal:qual_equal whitelisted_method qual_name in
-    List.exists ~f:method_matches whitelist
 
   (** Given REVERSED list of method qualifiers (method_name::class_name::rest_quals), return
       whether method should be translated based on method and class whitelists *)
-  let is_whitelisted_cpp_method qual_method_rev =
+  let is_whitelisted_cpp_method =
-    (* method is either explictely whitelisted, or all method of a class are whitelisted *)
+    let method_whitelist =
-    is_whitelisted_qual_name (List.rev qual_method_rev) Config.whitelisted_cpp_methods ||
+      List.map ~f:Procname.qualifiers_of_fuzzy_qual_name Config.whitelisted_cpp_methods in
-    is_whitelisted_qual_name (List.tl_exn qual_method_rev |> List.rev)
+    let class_whitelist =
-      Config.whitelisted_cpp_classes
+      List.map ~f:Procname.qualifiers_of_fuzzy_qual_name Config.whitelisted_cpp_classes in
-
+    fun qual_method_rev ->
-
+      (* method is either explictely whitelisted, or all method of a class are whitelisted *)
+      is_whitelisted_qual_name (List.rev qual_method_rev) method_whitelist ||
+      is_whitelisted_qual_name (List.tl_exn qual_method_rev |> List.rev) class_whitelist
 
   let should_translate_decl trans_unit_ctx dec decl_trans_context =
     let info = Clang_ast_proj.get_decl_tuple dec in

infer/tests/clang.make

@@ -14,7 +14,7 @@ OBJECTS = $(foreach source,$(SOURCES),$(basename $(source)).o)
 include $(TESTS_DIR)/base.make
 include $(TESTS_DIR)/clang-base.make
 
-infer-out$(TEST_SUFFIX)/report.json: $(CLANG_DEPS) $(SOURCES) $(HEADERS)
+infer-out$(TEST_SUFFIX)/report.json: $(CLANG_DEPS) $(SOURCES) $(HEADERS) $(TESTS_DIR)/.inferconfig
 	$(call silent_on_success,\
 	  $(INFER_BIN) --results-dir $(@D) --check-duplicate-symbols \
 	    $(INFER_OPTIONS) -a $(ANALYZER) -- \

infer/tests/codetoanalyze/cpp/checkers/siof/siof.cpp

@@ -41,6 +41,14 @@ int X::static_pod_accesses_non_pod = access_to_non_pod(); // SIOF!
 SomeNonPODObject initWithStatic = getFunctionStaticNonPOD(); // OK
 SomeNonPODObject initWithGlobal = getGlobalNonPOD(); // SIOF!
 
+SomeNonPODObject initWithGlobalWhitelisted = getGlobalNonPODWhitelisted(); // OK
+
+SomeNonPODObject initWithGlobalWhitelistedNamespaced =
+    whitelisted::getGlobalNonPOD(); // OK
+
+SomeNonPODObject initWithGlobalWhitelistedTemplated =
+    whitelisted::TemplatedObject<int>::getGlobalNonPOD(); // OK
+
 extern SomeConstexprObject& getGlobalConstexpr();
 SomeConstexprObject initWithConstexpr = getGlobalConstexpr();
 

infer/tests/codetoanalyze/cpp/checkers/siof/siof_different_tu.cpp

@@ -33,6 +33,11 @@ SomeNonPODObject& getGlobalNonPOD() {
   return global_object2;
 }
 
+SomeNonPODObject& getGlobalNonPODWhitelisted() {
+  some_other_global_object2.some_method();
+  return global_object2;
+}
+
 // initialise static class field
 SomeConstexprObject SomeConstexprObject::instance_;
 
@@ -40,6 +45,23 @@ SomeConstexprObject& getGlobalConstexpr() {
   return SomeConstexprObject::singletonMethod();
 }
 
+namespace whitelisted {
+
+SomeNonPODObject& getGlobalNonPOD() {
+  some_other_global_object2.some_method();
+  return global_object2;
+}
+
+template <typename T>
+SomeNonPODObject& TemplatedObject<T>::getGlobalNonPOD() {
+  some_other_global_object2.some_method();
+  return global_object2;
+}
+
+// instantiate template so that infer analyses it
+template struct TemplatedObject<int>;
+} // namespace whitelisted
+
 // initialise static class field
 template <typename T>
 SomeTemplatedConstexprObject<T> SomeTemplatedConstexprObject<T>::instance_;

infer/tests/codetoanalyze/cpp/checkers/siof/siof_types.h

@@ -57,3 +57,13 @@ int access_to_templated_non_pod();
 int access_to_non_pod();
 SomeNonPODObject& getFunctionStaticNonPOD();
 SomeNonPODObject& getGlobalNonPOD();
+SomeNonPODObject& getGlobalNonPODWhitelisted();
+
+namespace whitelisted {
+SomeNonPODObject& getGlobalNonPOD();
+
+template <typename T>
+struct TemplatedObject {
+  static SomeNonPODObject& getGlobalNonPOD();
+};
+} // namespace whitelisted