[annot reachability] apply sanitizers in more cases

Summary:
Change the logic of the annotation reachability checker in the following
ways:

1. Sanitizers take priority over sinks, i.e. a procedure that is both a
sink and a sanitizer is not a sink. This changes the existing tests
that seemed to assume the opposite. However I think that way is more
useful and goes better with the fact that sanitizers are specified as
"overrides".

2. When applying a summary, check again that we are not in a sanitizer
for the corresponding sink.

Without (2) this there was a subtle bug when several rules were
specified.  For example, if `sink_wrapper()` wraps `sink()` for a rule
`R` then the summary of `sink_wrapper()` will be: `R-sink : call to sink()`.

Then, suppose `sanitizer()` calls `sink_wrapper()` and `sanitizer()` is
a sanitizer for `R` but not for another rule `R'`. The previous code
would add the call to `sink()` to the summary of `sanitizer()` because
it's not a sanitizer for `R'`, even though `sink()` is not a sink for
`R'`!

The current code will re-apply the rules correctly so that sinks are
matched only against the right sanitizers.

Reviewed By: skcho

Differential Revision: D16895577

fbshipit-source-id: 266cc4940

infer/src/checkers/annotationReachability.ml

@@ -495,7 +495,9 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
   type extras = AnnotationSpec.t list
 
   let is_sink tenv (spec : AnnotationSpec.t) ~caller_pname ~callee_pname =
-    spec.sink_predicate tenv callee_pname && not (spec.sanitizer_predicate tenv caller_pname)
+    spec.sink_predicate tenv callee_pname
+    && (not (spec.sanitizer_predicate tenv callee_pname))
+    && not (spec.sanitizer_predicate tenv caller_pname)
 
 
   let check_call tenv ~caller_pname ~callee_pname call_site astate specs =
@@ -507,7 +509,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
         else astate )
 
 
-  let merge_callee_map call_site summary callee_pname tenv specs astate =
+  let merge_callee_map call_site summary ~caller_pname ~callee_pname tenv specs astate =
     match Payload.read ~caller_summary:summary ~callee_pname with
     | None ->
         astate
@@ -516,12 +518,17 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
         let add_call_site annot sink calls astate =
           if Domain.CallSites.is_empty calls then astate
           else
-            let pname = Summary.get_proc_name summary in
-            List.fold
-              ~f:(fun astate (spec : AnnotationSpec.t) ->
-                if spec.sanitizer_predicate tenv pname then astate
-                else Domain.add_call_site annot sink call_site astate )
-              ~init:astate specs
+            (* Add the sink to the current state only if the caller pname is not a sanitizer for
+               that sink. Ideally we would check only the spec that was responsible for adding the
+               sink but it is not obvious how to link back from annot to specs. Instead see if one
+               of the specs thinks that this sink is indeed a sink. *)
+            List.fold ~init:astate specs ~f:(fun astate (spec : AnnotationSpec.t) ->
+                if is_sink tenv spec ~caller_pname ~callee_pname:sink then (
+                  L.d_printf "%s: Adding sink call from `%a`'s summary `%a -> %a`@\n"
+                    spec.description Typ.Procname.pp callee_pname Typ.Procname.pp caller_pname
+                    Typ.Procname.pp sink ;
+                  Domain.add_call_site annot sink call_site astate )
+                else astate )
         in
         Domain.fold
           (fun annot sink_map astate -> Domain.SinkMap.fold (add_call_site annot) sink_map astate)
@@ -533,7 +540,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
         let caller_pname = Summary.get_proc_name summary in
         let call_site = CallSite.make callee_pname call_loc in
         check_call tenv ~callee_pname ~caller_pname call_site astate extras
-        |> merge_callee_map call_site summary callee_pname tenv extras
+        |> merge_callee_map call_site summary ~callee_pname ~caller_pname tenv extras
     | _ ->
         astate
 

infer/tests/build_systems/annotation-reachability-sources-override/issues.exp

@@ -1,2 +1 @@
 codetoanalyze/cpp/annotation-reachability/reachability.cpp, CheckFrom::danger_via, 2, TEST_ANNOT_REACH, no_bucket, ERROR, []
-codetoanalyze/cpp/annotation-reachability/reachability.cpp, CheckFrom::death_via, 0, TEST_ANNOT_REACH, no_bucket, ERROR, []

infer/tests/codetoanalyze/cpp/annotation-reachability/Makefile

@@ -19,13 +19,28 @@ INFER_OPTIONS = --annotation-reachability-only --debug-exceptions --project-root
         }, \
         "symbols": [ "Danger::", "death" ] \
       } \
+    }, \
+    "API_ACCESS_VIOLATION": { \
+      "doc_url": "https://example.com/some_doc.html", \
+      "sources": { \
+        "paths": [ "codetoanalyze/cpp/annotation-reachability/sources/" ] \
+      }, \
+      "sinks": { \
+        "desc": "a disallowed API", \
+        "overrides": { \
+          "symbols": [ \
+            "safewrapper::" \
+          ] \
+        }, \
+        "symbols" : [ "details::" ], \
+        "paths": [ "codetoanalyze/cpp/annotation-reachability/forbidden/" ] \
+      } \
     } \
   }'
 
 INFERPRINT_OPTIONS = --issues-tests
 
-
-SOURCES = $(wildcard *.cpp)
+SOURCES = $(wildcard *.cpp sources/*.cpp forbidden/*.cpp)
 
 include $(TESTS_DIR)/clang.make
 

infer/tests/codetoanalyze/cpp/annotation-reachability/forbidden/library.cpp

@@ -0,0 +1,31 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// low-level implementation, clients shouldn't use directly
+namespace details {
+
+struct LowLevel {
+  LowLevel() {}
+  ~LowLevel() {}
+};
+
+void low_level() {}
+} // namespace details
+
+// calls into this namespace from client code are allowed
+namespace safewrapper {
+
+struct Wrapper {
+  details::LowLevel wrapped;
+
+  Wrapper() : wrapped() {}
+  ~Wrapper() {}
+};
+
+void wrapper() { details::low_level(); }
+
+} // namespace safewrapper

infer/tests/codetoanalyze/cpp/annotation-reachability/forbidden/library.h

@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+namespace safewrapper {
+
+struct Wrapper {
+  Wrapper();
+  ~Wrapper();
+};
+
+void wrapper();
+} // namespace safewrapper
+
+namespace details {
+void low_level();
+
+struct LowLevel {
+  LowLevel();
+  ~LowLevel();
+};
+} // namespace details

infer/tests/codetoanalyze/cpp/annotation-reachability/issues.exp

@@ -1,5 +1,5 @@
-codetoanalyze/cpp/annotation-reachability/reachability.cpp, CheckFrom::Destructive::~Destructive, 0, TEST_ANNOT_REACH, no_bucket, ERROR, []
 codetoanalyze/cpp/annotation-reachability/reachability.cpp, CheckFrom::danger_via, 2, TEST_ANNOT_REACH, no_bucket, ERROR, []
-codetoanalyze/cpp/annotation-reachability/reachability.cpp, CheckFrom::death_via, 0, TEST_ANNOT_REACH, no_bucket, ERROR, []
 codetoanalyze/cpp/annotation-reachability/reachability.cpp, CheckFrom::imminent_danger, 0, TEST_ANNOT_REACH, no_bucket, ERROR, []
-codetoanalyze/cpp/annotation-reachability/reachability.cpp, CheckFrom::imminent_death, 0, TEST_ANNOT_REACH, no_bucket, ERROR, []
+codetoanalyze/cpp/annotation-reachability/sources/client.cpp, client::CallLowLevelBad::CallLowLevelBad, 0, API_ACCESS_VIOLATION, no_bucket, ERROR, []
+codetoanalyze/cpp/annotation-reachability/sources/client.cpp, client::CallLowLevelBad::~CallLowLevelBad, 0, API_ACCESS_VIOLATION, no_bucket, ERROR, []
+codetoanalyze/cpp/annotation-reachability/sources/client.cpp, client::call_protected_api_bad, 0, API_ACCESS_VIOLATION, no_bucket, ERROR, []

infer/tests/codetoanalyze/cpp/annotation-reachability/sources/client.cpp

@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+#include "../forbidden/library.h"
+
+namespace client {
+
+void call_protected_api_bad() { details::low_level(); }
+
+void call_wrapper_ok() { safewrapper::wrapper(); }
+
+struct CallWrapperOk : public safewrapper::Wrapper {
+  CallWrapperOk() {}
+  ~CallWrapperOk() {}
+};
+
+struct CallLowLevelBad {
+  details::LowLevel wrapped;
+
+  CallLowLevelBad() {}
+  ~CallLowLevelBad() {}
+};
+
+} // namespace client