From 7e6654cd2555dbd3e68c5eeff5f7a3f53c4c0dfa Mon Sep 17 00:00:00 2001 From: Sungkeun Cho Date: Fri, 19 Feb 2021 06:08:38 -0800 Subject: [PATCH] [ConfigImpact] Add a new checker: ConfigImpactAnalysis Summary: This checker is for internal usage. Reviewed By: ezgicicek Differential Revision: D26453750 fbshipit-source-id: a9397f2e4 --- Makefile | 1 + infer/man/man1/infer-analyze.txt | 9 + infer/man/man1/infer-full.txt | 11 + infer/man/man1/infer-report.txt | 1 + infer/man/man1/infer.txt | 11 + infer/src/backend/Payloads.ml | 3 + infer/src/backend/Payloads.mli | 1 + infer/src/backend/registerCheckers.ml | 8 +- infer/src/base/Checker.ml | 26 +- infer/src/base/Checker.mli | 1 + infer/src/base/IssueType.ml | 5 + infer/src/base/IssueType.mli | 2 + infer/src/checkers/ConfigImpactAnalysis.ml | 239 ++++++++++++++++++ infer/src/checkers/ConfigImpactAnalysis.mli | 16 ++ .../java/fb-config-impact/Makefile | 13 + 15 files changed, 345 insertions(+), 2 deletions(-) create mode 100644 infer/src/checkers/ConfigImpactAnalysis.ml create mode 100644 infer/src/checkers/ConfigImpactAnalysis.mli create mode 100644 infer/tests/codetoanalyze/java/fb-config-impact/Makefile diff --git a/Makefile b/Makefile index 95614280a..3705cf052 100644 --- a/Makefile +++ b/Makefile @@ -172,6 +172,7 @@ DIRECT_TESTS += \ ifeq ($(IS_FACEBOOK_TREE),yes) DIRECT_TESTS += \ + java_fb-config-impact \ java_fb-gk-interaction \ java_fb-performance endif diff --git a/infer/man/man1/infer-analyze.txt b/infer/man/man1/infer-analyze.txt index f37c64289..8a62b6369 100644 --- a/infer/man/man1/infer-analyze.txt +++ b/infer/man/man1/infer-analyze.txt @@ -57,6 +57,15 @@ OPTIONS other checkers (Conversely: --no-config-checks-between-markers-only) + --config-impact-analysis + Activates: checker config-impact-analysis: [EXPERIMENTAL] Collects + function that are called without config checks. (Conversely: + --no-config-impact-analysis) + + --config-impact-analysis-only + Activates: Enable config-impact-analysis and disable all other + checkers (Conversely: --no-config-impact-analysis-only) + --continue-analysis Activates: Continue the analysis after more targets are captured by --continue. The other analysis options should be given the same diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index 181c90908..26a0a8f65 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -258,6 +258,16 @@ OPTIONS other checkers (Conversely: --no-config-checks-between-markers-only) See also infer-analyze(1). + --config-impact-analysis + Activates: checker config-impact-analysis: [EXPERIMENTAL] Collects + function that are called without config checks. (Conversely: + --no-config-impact-analysis) See also infer-analyze(1). + + --config-impact-analysis-only + Activates: Enable config-impact-analysis and disable all other + checkers (Conversely: --no-config-impact-analysis-only) + See also infer-analyze(1). + --continue Activates: Continue the capture for the reactive analysis, increasing the changed files/procedures. (If a procedure was @@ -418,6 +428,7 @@ OPTIONS CONDITION_ALWAYS_FALSE (disabled by default), CONDITION_ALWAYS_TRUE (disabled by default), CONFIG_CHECKS_BETWEEN_MARKERS (disabled by default), + CONFIG_IMPACT (disabled by default), CONSTANT_ADDRESS_DEREFERENCE (disabled by default), CREATE_INTENT_FROM_URI (enabled by default), CROSS_SITE_SCRIPTING (enabled by default), diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt index 11ec2a2a3..63a812cc3 100644 --- a/infer/man/man1/infer-report.txt +++ b/infer/man/man1/infer-report.txt @@ -121,6 +121,7 @@ OPTIONS CONDITION_ALWAYS_FALSE (disabled by default), CONDITION_ALWAYS_TRUE (disabled by default), CONFIG_CHECKS_BETWEEN_MARKERS (disabled by default), + CONFIG_IMPACT (disabled by default), CONSTANT_ADDRESS_DEREFERENCE (disabled by default), CREATE_INTENT_FROM_URI (enabled by default), CROSS_SITE_SCRIPTING (enabled by default), diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index 1e4f99bad..85f3696ae 100644 --- a/infer/man/man1/infer.txt +++ b/infer/man/man1/infer.txt @@ -258,6 +258,16 @@ OPTIONS other checkers (Conversely: --no-config-checks-between-markers-only) See also infer-analyze(1). + --config-impact-analysis + Activates: checker config-impact-analysis: [EXPERIMENTAL] Collects + function that are called without config checks. (Conversely: + --no-config-impact-analysis) See also infer-analyze(1). + + --config-impact-analysis-only + Activates: Enable config-impact-analysis and disable all other + checkers (Conversely: --no-config-impact-analysis-only) + See also infer-analyze(1). + --continue Activates: Continue the capture for the reactive analysis, increasing the changed files/procedures. (If a procedure was @@ -418,6 +428,7 @@ OPTIONS CONDITION_ALWAYS_FALSE (disabled by default), CONDITION_ALWAYS_TRUE (disabled by default), CONFIG_CHECKS_BETWEEN_MARKERS (disabled by default), + CONFIG_IMPACT (disabled by default), CONSTANT_ADDRESS_DEREFERENCE (disabled by default), CREATE_INTENT_FROM_URI (enabled by default), CROSS_SITE_SCRIPTING (enabled by default), diff --git a/infer/src/backend/Payloads.ml b/infer/src/backend/Payloads.ml index e84667790..9ad648623 100644 --- a/infer/src/backend/Payloads.ml +++ b/infer/src/backend/Payloads.ml @@ -14,6 +14,7 @@ type t = ; buffer_overrun_analysis: BufferOverrunAnalysisSummary.t option ; buffer_overrun_checker: BufferOverrunCheckerSummary.t option ; config_checks_between_markers: ConfigChecksBetweenMarkers.Summary.t option + ; config_impact_analysis: ConfigImpactAnalysis.Summary.t option ; cost: CostDomain.summary option ; lab_resource_leaks: ResourceLeakDomain.summary option ; dotnet_resource_leaks: ResourceLeakCSDomain.summary option @@ -46,6 +47,7 @@ let fields = ~buffer_overrun_checker:(fun f -> mk f "BufferOverrunChecker" BufferOverrunCheckerSummary.pp) ~config_checks_between_markers:(fun f -> mk f "ConfigChecksBetweenMarkers" ConfigChecksBetweenMarkers.Summary.pp ) + ~config_impact_analysis:(fun f -> mk f "ConfigImpactAnalysis" ConfigImpactAnalysis.Summary.pp) ~cost:(fun f -> mk f "Cost" CostDomain.pp_summary) ~litho_required_props:(fun f -> mk f "Litho Required Props" LithoDomain.pp_summary) ~pulse:(fun f -> mk f "Pulse" PulseSummary.pp) @@ -71,6 +73,7 @@ let empty = ; buffer_overrun_analysis= None ; buffer_overrun_checker= None ; config_checks_between_markers= None + ; config_impact_analysis= None ; cost= None ; lab_resource_leaks= None ; dotnet_resource_leaks= None diff --git a/infer/src/backend/Payloads.mli b/infer/src/backend/Payloads.mli index a315eb485..1086dd722 100644 --- a/infer/src/backend/Payloads.mli +++ b/infer/src/backend/Payloads.mli @@ -18,6 +18,7 @@ include sig ; buffer_overrun_analysis: BufferOverrunAnalysisSummary.t option ; buffer_overrun_checker: BufferOverrunCheckerSummary.t option ; config_checks_between_markers: ConfigChecksBetweenMarkers.Summary.t option + ; config_impact_analysis: ConfigImpactAnalysis.Summary.t option ; cost: CostDomain.summary option ; lab_resource_leaks: ResourceLeakDomain.summary option ; dotnet_resource_leaks: ResourceLeakCSDomain.summary option diff --git a/infer/src/backend/registerCheckers.ml b/infer/src/backend/registerCheckers.ml index 463da72cb..58b5b7387 100644 --- a/infer/src/backend/registerCheckers.ml +++ b/infer/src/backend/registerCheckers.ml @@ -197,7 +197,13 @@ let all_checkers = interprocedural Payloads.Fields.config_checks_between_markers ConfigChecksBetweenMarkers.checker in - [(checker, Clang); (checker, Java)] ) } ] + [(checker, Clang); (checker, Java)] ) } + ; { checker= ConfigImpactAnalysis + ; callbacks= + (let checker = + interprocedural Payloads.Fields.config_impact_analysis ConfigImpactAnalysis.checker + in + [(checker, Java)] ) } ] let get_active_checkers () = diff --git a/infer/src/base/Checker.ml b/infer/src/base/Checker.ml index 88afdd052..a5459a1bd 100644 --- a/infer/src/base/Checker.ml +++ b/infer/src/base/Checker.ml @@ -14,6 +14,7 @@ type t = | BufferOverrunAnalysis | BufferOverrunChecker | ConfigChecksBetweenMarkers + | ConfigImpactAnalysis | Cost | Eradicate | FragmentRetainsView @@ -128,12 +129,35 @@ let config_unsafe checker = ; kind= UserFacing { title= "Config Checks between Markers" - ; markdown_body= "This checker is currently only useful for certain Facebook code." } + ; markdown_body= + "This checker collects config checkings in some program regions determined by \ + pairs of marker-starts and marker-ends. The set of config checking functions, \ + marker-start functions, and marker-end functions is hardcoded and empty by \ + default for now, so to use this checker, please modify the code directly in \ + [FbGKInteraction.ml](https://github.com/facebook/infer/tree/master/infer/src/opensource)." + } ; support= supports_clang_and_java_experimental ; short_documentation= "[EXPERIMENTAL] Collects config checks between marker start and end." ; cli_flags= Some {deprecated= []; show_in_help= true} ; enabled_by_default= false ; activates= [] } + | ConfigImpactAnalysis -> + { id= "config-impact-analysis" + ; kind= + UserFacing + { title= "Config Impact Analysis" + ; markdown_body= + "This checker collects functions whose execution isn't gated by certain \ + pre-defined gating functions. The set of gating functions is hardcoded and empty \ + by default for now, so to use this checker, please modify the code directly in \ + [FbGKInteraction.ml](https://github.com/facebook/infer/tree/master/infer/src/opensource)." + } + ; support= supports_clang_and_java_experimental + ; short_documentation= + "[EXPERIMENTAL] Collects function that are called without config checks." + ; cli_flags= Some {deprecated= []; show_in_help= true} + ; enabled_by_default= false + ; activates= [] } | Cost -> { id= "cost" ; kind= diff --git a/infer/src/base/Checker.mli b/infer/src/base/Checker.mli index 33085fd60..c47510f51 100644 --- a/infer/src/base/Checker.mli +++ b/infer/src/base/Checker.mli @@ -13,6 +13,7 @@ type t = | BufferOverrunAnalysis | BufferOverrunChecker | ConfigChecksBetweenMarkers + | ConfigImpactAnalysis | Cost | Eradicate | FragmentRetainsView diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index e7d8d82f8..3ffe4ef76 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -414,6 +414,11 @@ let config_checks_between_markers = ~user_documentation:"A config checking is done between a marker's start and end" +let config_impact_analysis = + register ~enabled:false ~id:"CONFIG_IMPACT" Advice ConfigImpactAnalysis + ~user_documentation:"A function is called without a config check" + + let constant_address_dereference = register ~enabled:false ~id:"CONSTANT_ADDRESS_DEREFERENCE" Warning Pulse ~user_documentation:[%blob "../../documentation/issues/CONSTANT_ADDRESS_DEREFERENCE.md"] diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index c758c96f0..31dac7d50 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -135,6 +135,8 @@ val condition_always_true : t val config_checks_between_markers : t +val config_impact_analysis : t + val constant_address_dereference : t val create_intent_from_uri : t diff --git a/infer/src/checkers/ConfigImpactAnalysis.ml b/infer/src/checkers/ConfigImpactAnalysis.ml new file mode 100644 index 000000000..3dc06e2c0 --- /dev/null +++ b/infer/src/checkers/ConfigImpactAnalysis.ml @@ -0,0 +1,239 @@ +(* + * 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. + *) + +open! IStd +module F = Format +module ConfigName = FbGKInteraction.ConfigName + +module Branch = struct + type t = True | False | Top + + let pp f = function + | True -> + F.pp_print_string f "true branch" + | False -> + F.pp_print_string f "false branch" + | Top -> + AbstractDomain.TopLiftedUtils.pp_top f + + + let leq ~lhs ~rhs = + match (lhs, rhs) with True, True | False, False | _, Top -> true | _, _ -> false + + + let join x y = match (x, y) with True, True -> True | False, False -> False | _, _ -> Top + + let widen ~prev ~next ~num_iters:_ = join prev next + + let top = Top + + let is_top = function Top -> true | True | False -> false + + let neg = function True -> False | False -> True | Top -> Top +end + +module ConfigChecks = AbstractDomain.SafeInvertedMap (ConfigName) (Branch) + +module UncheckedCallee = struct + type t = {callee: Procname.t; location: Location.t} [@@deriving compare] + + let pp f {callee; location} = + F.fprintf f "%a is called at %a" Procname.pp callee Location.pp location + + + let get_location {location} = location + + let report {InterproceduralAnalysis.proc_desc; err_log} x = + let desc = F.asprintf "%a without config check" pp x in + let trace = + (* TODO *) + [] + in + Reporting.log_issue proc_desc err_log ~loc:(get_location x) ~ltr:trace ConfigImpactAnalysis + IssueType.config_impact_analysis desc +end + +module UncheckedCallees = struct + include AbstractDomain.FiniteSet (UncheckedCallee) + + let report analysis_data unchecked_callees = + iter (UncheckedCallee.report analysis_data) unchecked_callees +end + +module Loc = struct + type t = Ident of Ident.t | Pvar of Pvar.t [@@deriving compare] + + let pp f = function Ident id -> Ident.pp f id | Pvar pvar -> Pvar.pp Pp.text f pvar + + let of_id id = Ident id + + let of_pvar pvar = Pvar pvar +end + +module ConfigLifted = AbstractDomain.Flat (ConfigName) + +module Val = struct + type t = {config: ConfigLifted.t} + + let pp f {config} = F.fprintf f "@[config:@,%a@]" ConfigLifted.pp config + + let leq ~lhs ~rhs = ConfigLifted.leq ~lhs:lhs.config ~rhs:rhs.config + + let join x y = {config= ConfigLifted.join x.config y.config} + + let widen ~prev ~next ~num_iters = + {config= ConfigLifted.widen ~prev:prev.config ~next:next.config ~num_iters} + + + let bottom = {config= ConfigLifted.bottom} + + let of_config config = {config= ConfigLifted.v config} + + let get_config_opt {config} = ConfigLifted.get config +end + +module Mem = struct + include AbstractDomain.Map (Loc) (Val) + + let lookup loc mem = find_opt loc mem |> Option.value ~default:Val.bottom +end + +module Summary = struct + type t = {unchecked_callees: UncheckedCallees.t} + + let pp f {unchecked_callees} = + F.fprintf f "@[unchecked callees:@,%a@]" UncheckedCallees.pp unchecked_callees +end + +module Dom = struct + type t = {config_checks: ConfigChecks.t; unchecked_callees: UncheckedCallees.t; mem: Mem.t} + + let pp f {config_checks; unchecked_callees; mem} = + F.fprintf f "@[@[config checks:@,%a@]@ @[unchecked callees:@,%a@]@ @[mem:%,%a@]@]" + ConfigChecks.pp config_checks UncheckedCallees.pp unchecked_callees Mem.pp mem + + + let leq ~lhs ~rhs = + ConfigChecks.leq ~lhs:lhs.config_checks ~rhs:rhs.config_checks + && UncheckedCallees.leq ~lhs:lhs.unchecked_callees ~rhs:rhs.unchecked_callees + && Mem.leq ~lhs:lhs.mem ~rhs:rhs.mem + + + let join x y = + { config_checks= ConfigChecks.join x.config_checks y.config_checks + ; unchecked_callees= UncheckedCallees.join x.unchecked_callees y.unchecked_callees + ; mem= Mem.join x.mem y.mem } + + + let widen ~prev ~next ~num_iters = + { config_checks= ConfigChecks.widen ~prev:prev.config_checks ~next:next.config_checks ~num_iters + ; unchecked_callees= + UncheckedCallees.widen ~prev:prev.unchecked_callees ~next:next.unchecked_callees ~num_iters + ; mem= Mem.widen ~prev:prev.mem ~next:next.mem ~num_iters } + + + let to_summary {unchecked_callees} = {Summary.unchecked_callees} + + let init = + {config_checks= ConfigChecks.top; unchecked_callees= UncheckedCallees.bottom; mem= Mem.bottom} + + + let add_mem loc v ({mem} as astate) = {astate with mem= Mem.add loc v mem} + + let copy_mem ~tgt ~src ({mem} as astate) = add_mem tgt (Mem.lookup src mem) astate + + let call_config_check ret config astate = add_mem (Loc.of_id ret) (Val.of_config config) astate + + let load_config id pvar astate = copy_mem ~tgt:(Loc.of_id id) ~src:(Loc.of_pvar pvar) astate + + let store_config pvar id astate = copy_mem ~tgt:(Loc.of_pvar pvar) ~src:(Loc.of_id id) astate + + let boolean_value id_tgt id_src astate = + copy_mem ~tgt:(Loc.of_id id_tgt) ~src:(Loc.of_id id_src) astate + + + let neg_branch res = Option.map ~f:(fun (config, branch) -> (config, Branch.neg branch)) res + + let rec get_config_check_prune e mem = + match (e : Exp.t) with + | Var id -> + Mem.lookup (Loc.of_id id) mem + |> Val.get_config_opt + |> Option.map ~f:(fun config -> (config, Branch.True)) + | UnOp (LNot, e, _) -> + get_config_check_prune e mem |> neg_branch + | BinOp ((Eq | Ne), Const _, Const _) -> + None + | (BinOp (Eq, e, (Const _ as const)) | BinOp (Eq, (Const _ as const), e)) when Exp.is_zero const + -> + get_config_check_prune e mem |> neg_branch + | (BinOp (Ne, e, (Const _ as const)) | BinOp (Ne, (Const _ as const), e)) when Exp.is_zero const + -> + get_config_check_prune e mem + | _ -> + None + + + let prune e ({config_checks; mem} as astate) = + get_config_check_prune e mem + |> Option.value_map ~default:astate ~f:(fun (config, branch) -> + {astate with config_checks= ConfigChecks.add config branch config_checks} ) + + + let call callee location ({config_checks; unchecked_callees} as astate) = + if ConfigChecks.is_top config_checks then + let unchecked_callee = UncheckedCallee.{callee; location} in + {astate with unchecked_callees= UncheckedCallees.add unchecked_callee unchecked_callees} + else astate +end + +module TransferFunctions = struct + module CFG = ProcCfg.NormalOneInstrPerNode + module Domain = Dom + + type analysis_data = Summary.t InterproceduralAnalysis.t + + let is_java_boolean_value_method pname = + Procname.get_class_name pname |> Option.exists ~f:(String.equal "java.lang.Boolean") + && Procname.get_method pname |> String.equal "booleanValue" + + + let exec_instr astate {InterproceduralAnalysis.tenv} _node instr = + match (instr : Sil.instr) with + | Load {id; e= Lvar pvar} -> + Dom.load_config id pvar astate + | Store {e1= Lvar pvar; e2= Var id} -> + Dom.store_config pvar id astate + | Call ((ret, _), Const (Cfun callee), [(Var id, _)], _, _) + when is_java_boolean_value_method callee -> + Dom.boolean_value ret id astate + | Call ((ret, _), Const (Cfun callee), args, location, _) -> ( + match FbGKInteraction.get_config_check tenv callee args with + | Some (`Config config) -> + Dom.call_config_check ret config astate + | Some (`Exp _) -> + astate + | None -> + (* normal function calls *) + Dom.call callee location astate ) + | Prune (e, _, _, _) -> + Dom.prune e astate + | _ -> + astate + + + let pp_session_name node fmt = + F.fprintf fmt "Config impact function calls %a" CFG.Node.pp_id (CFG.Node.id node) +end + +module Analyzer = AbstractInterpreter.MakeWTO (TransferFunctions) + +let checker ({InterproceduralAnalysis.proc_desc} as analysis_data) = + Option.map (Analyzer.compute_post analysis_data ~initial:Dom.init proc_desc) + ~f:(fun ({Dom.unchecked_callees} as astate) -> + UncheckedCallees.report analysis_data unchecked_callees ; + Dom.to_summary astate ) diff --git a/infer/src/checkers/ConfigImpactAnalysis.mli b/infer/src/checkers/ConfigImpactAnalysis.mli new file mode 100644 index 000000000..4aa985eb1 --- /dev/null +++ b/infer/src/checkers/ConfigImpactAnalysis.mli @@ -0,0 +1,16 @@ +(* + * 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. + *) + +open! IStd + +module Summary : sig + type t + + val pp : Format.formatter -> t -> unit +end + +val checker : Summary.t InterproceduralAnalysis.t -> Summary.t option diff --git a/infer/tests/codetoanalyze/java/fb-config-impact/Makefile b/infer/tests/codetoanalyze/java/fb-config-impact/Makefile new file mode 100644 index 000000000..84613398a --- /dev/null +++ b/infer/tests/codetoanalyze/java/fb-config-impact/Makefile @@ -0,0 +1,13 @@ +# 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. + +TESTS_DIR = ../../.. + +INFER_OPTIONS = --config-impact-analysis-only --debug-exceptions \ + --report-force-relative-path +INFERPRINT_OPTIONS = --issues-tests +SOURCES = $(wildcard *.java) + +include $(TESTS_DIR)/javac.make