diff --git a/infer/src/checkers/annotationReachability.ml b/infer/src/checkers/annotationReachability.ml index 11ac83a2f..d3c60640f 100644 --- a/infer/src/checkers/annotationReachability.ml +++ b/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 diff --git a/infer/tests/build_systems/annotation-reachability-sources-override/issues.exp b/infer/tests/build_systems/annotation-reachability-sources-override/issues.exp index fc71bc72f..77c1af81b 100644 --- a/infer/tests/build_systems/annotation-reachability-sources-override/issues.exp +++ b/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, [] diff --git a/infer/tests/codetoanalyze/cpp/annotation-reachability/Makefile b/infer/tests/codetoanalyze/cpp/annotation-reachability/Makefile index 7e63df8b2..342021630 100644 --- a/infer/tests/codetoanalyze/cpp/annotation-reachability/Makefile +++ b/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 diff --git a/infer/tests/codetoanalyze/cpp/annotation-reachability/forbidden/library.cpp b/infer/tests/codetoanalyze/cpp/annotation-reachability/forbidden/library.cpp new file mode 100644 index 000000000..b64783050 --- /dev/null +++ b/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 diff --git a/infer/tests/codetoanalyze/cpp/annotation-reachability/forbidden/library.h b/infer/tests/codetoanalyze/cpp/annotation-reachability/forbidden/library.h new file mode 100644 index 000000000..fb1188886 --- /dev/null +++ b/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 diff --git a/infer/tests/codetoanalyze/cpp/annotation-reachability/issues.exp b/infer/tests/codetoanalyze/cpp/annotation-reachability/issues.exp index 702989418..d4c9dcab5 100644 --- a/infer/tests/codetoanalyze/cpp/annotation-reachability/issues.exp +++ b/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, [] diff --git a/infer/tests/codetoanalyze/cpp/annotation-reachability/sources/client.cpp b/infer/tests/codetoanalyze/cpp/annotation-reachability/sources/client.cpp new file mode 100644 index 000000000..f6a10a875 --- /dev/null +++ b/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