From c98570f899bb7691747b1825355e8450bc50226a Mon Sep 17 00:00:00 2001 From: Dino Distefano Date: Tue, 31 Oct 2017 11:11:49 -0700 Subject: [PATCH] do not report on captured vars in objc blocks Reviewed By: jeremydubreil Differential Revision: D6173238 fbshipit-source-id: dc93a54 --- Makefile | 4 +- infer/src/IR/Procdesc.ml | 19 +++ infer/src/IR/Procdesc.mli | 4 + infer/src/checkers/liveness.ml | 21 +-- infer/src/checkers/uninit.ml | 123 +++++------------- infer/src/clang/cAst_utils.ml | 1 - .../tests/codetoanalyze/objc/uninit/Makefile | 20 +++ .../codetoanalyze/objc/uninit/issues.exp | 1 + .../codetoanalyze/objc/uninit/uninit_blocks.m | 49 +++++++ 9 files changed, 129 insertions(+), 113 deletions(-) create mode 100644 infer/tests/codetoanalyze/objc/uninit/Makefile create mode 100644 infer/tests/codetoanalyze/objc/uninit/issues.exp create mode 100644 infer/tests/codetoanalyze/objc/uninit/uninit_blocks.m diff --git a/Makefile b/Makefile index afba70699..e09dfc195 100644 --- a/Makefile +++ b/Makefile @@ -48,7 +48,7 @@ BUILD_SYSTEMS_TESTS += \ DIRECT_TESTS += \ c_biabduction c_bufferoverrun c_errors c_frontend \ - cpp_bufferoverrun cpp_errors cpp_frontend cpp_liveness cpp_quandary cpp_racerd cpp_siof cpp_uninit cpp_nullable \ + cpp_bufferoverrun cpp_errors cpp_frontend cpp_liveness cpp_quandary cpp_racerd cpp_siof cpp_uninit cpp_nullable \ ifneq ($(BUCK),no) BUILD_SYSTEMS_TESTS += buck-clang-db buck_flavors buck_flavors_run buck_flavors_deterministic @@ -67,7 +67,7 @@ BUILD_SYSTEMS_TESTS += xcodebuild_no_xcpretty objc_getters_setters DIRECT_TESTS += \ objc_frontend objc_errors objc_linters objc_ioslints \ objcpp_frontend objcpp_linters objc_linters-for-test-only objcpp_linters-for-test-only \ - objc_linters-def-folder objc_checkers objc_liveness + objc_linters-def-folder objc_checkers objc_liveness objc_uninit ifneq ($(XCPRETTY),no) BUILD_SYSTEMS_TESTS += xcodebuild endif diff --git a/infer/src/IR/Procdesc.ml b/infer/src/IR/Procdesc.ml index 9b3acd846..303e9fc27 100644 --- a/infer/src/IR/Procdesc.ml +++ b/infer/src/IR/Procdesc.ml @@ -544,3 +544,22 @@ let is_specialized pdesc = let attributes = get_attributes pdesc in attributes.ProcAttributes.is_specialized + +(* true if pvar is a captred variable of a cpp lambda or obcj block *) +let is_captured_var procdesc pvar = + let procname = get_proc_name procdesc in + let pvar_name = Pvar.get_name pvar in + let pvar_matches (name, _) = Mangled.equal name pvar_name in + let is_captured_var_cpp_lambda = + (* var is captured if the procedure is a lambda and the var is not in the locals or formals *) + Typ.Procname.is_cpp_lambda procname + && not + ( List.exists ~f:pvar_matches (get_locals procdesc) + || List.exists ~f:pvar_matches (get_formals procdesc) ) + in + let is_captured_var_objc_block = + (* var is captured if the procedure is a objc block and the var is in the captured *) + Typ.Procname.is_objc_block procname && List.exists ~f:pvar_matches (get_captured procdesc) + in + is_captured_var_cpp_lambda || is_captured_var_objc_block + diff --git a/infer/src/IR/Procdesc.mli b/infer/src/IR/Procdesc.mli index 21a38db6b..951bf3a6e 100644 --- a/infer/src/IR/Procdesc.mli +++ b/infer/src/IR/Procdesc.mli @@ -249,3 +249,7 @@ val is_loop_head : t -> Node.t -> bool val pp_signature : Format.formatter -> t -> unit val is_specialized : t -> bool + +(* true if pvar is a captred variable of a cpp lambda or obcj block *) + +val is_captured_var : t -> Pvar.t -> bool diff --git a/infer/src/checkers/liveness.ml b/infer/src/checkers/liveness.ml index fe1dfeaf2..0c2511c65 100644 --- a/infer/src/checkers/liveness.ml +++ b/infer/src/checkers/liveness.ml @@ -61,25 +61,6 @@ end module CFG = ProcCfg.OneInstrPerNode (ProcCfg.Backward (ProcCfg.Exceptional)) module Analyzer = AbstractInterpreter.Make (CFG) (TransferFunctions) -let is_captured_var procdesc pvar = - let procname = Procdesc.get_proc_name procdesc in - let pvar_name = Pvar.get_name pvar in - let pvar_matches (name, _) = Mangled.equal name pvar_name in - let is_captured_var_cpp_lambda = - (* var is captured if the procedure is a lambda and the var is not in the locals or formals *) - Typ.Procname.is_cpp_lambda procname - && not - ( List.exists ~f:pvar_matches (Procdesc.get_locals procdesc) - || List.exists ~f:pvar_matches (Procdesc.get_formals procdesc) ) - in - let is_captured_var_objc_block = - (* var is captured if the procedure is a objc block and the var is in the captured *) - Typ.Procname.is_objc_block procname - && List.exists ~f:pvar_matches (Procdesc.get_captured procdesc) - in - is_captured_var_cpp_lambda || is_captured_var_objc_block - - let is_whitelisted_var var = let whitelisted_vars = ["__assert_file__"; "__assert_fn__"] in List.exists @@ -96,7 +77,7 @@ let checker {Callbacks.tenv; summary; proc_desc} : Specs.summary = | Sil.Store (Lvar pvar, _, _, loc) when not ( Pvar.is_frontend_tmp pvar || Pvar.is_return pvar || Pvar.is_global pvar - || Domain.mem (Var.of_pvar pvar) live_vars || is_captured_var proc_desc pvar + || Domain.mem (Var.of_pvar pvar) live_vars || Procdesc.is_captured_var proc_desc pvar || is_whitelisted_var pvar ) -> let issue_id = IssueType.dead_store.unique_id in let message = F.asprintf "The value written to %a is never used" (Pvar.pp Pp.text) pvar in diff --git a/infer/src/checkers/uninit.ml b/infer/src/checkers/uninit.ml index 6eebcd64d..d166774b9 100644 --- a/infer/src/checkers/uninit.ml +++ b/infer/src/checkers/uninit.ml @@ -31,20 +31,32 @@ end) let intraprocedural_only = true -let is_address_pvar e = match e with Exp.Lvar _ -> true | _ -> false +let is_type_pointer t = match t.Typ.desc with Typ.Tptr _ -> true | _ -> false + +let exp_in_set exp vset = + let _, pvars = Exp.get_vars exp in + List.exists ~f:(fun pv -> D.mem (Var.of_pvar pv) vset) pvars + module TransferFunctions (CFG : ProcCfg.S) = struct module CFG = CFG module Domain = RecordDomain - type extras = FormalMap.t + let report message loc summary = + let issue_id = IssueType.uninitialized_value.unique_id in + let ltr = [Errlog.make_trace_element 0 loc "" []] in + let exn = Exceptions.Checkers (issue_id, Localise.verbatim_desc message) in + Reporting.log_error summary ~loc ~ltr exn + - let exec_instr (astate: Domain.astate) {ProcData.extras} _ (instr: HilInstr.t) = + type extras = FormalMap.t * Specs.summary + + let exec_instr (astate: Domain.astate) {ProcData.pdesc; ProcData.extras} _ (instr: HilInstr.t) = match instr with - | Assign (((lhs_var, _), _), HilExp.AccessPath (((rhs_var, rhs_typ) as rhs_base), _), _) -> + | Assign (((lhs_var, lhs_typ), _), HilExp.AccessPath (((rhs_var, rhs_typ) as rhs_base), _), loc) -> let uninit_vars = D.remove lhs_var astate.uninit_vars in let prepost = - if FormalMap.is_formal rhs_base extras + if FormalMap.is_formal rhs_base (fst extras) && match rhs_typ.desc with Typ.Tptr _ -> true | _ -> false then let pre' = D.add rhs_var (fst astate.prepost) in @@ -52,6 +64,16 @@ module TransferFunctions (CFG : ProcCfg.S) = struct (pre', post) else astate.prepost in + ( match rhs_var with + | Var.ProgramVar pv + when not (Pvar.is_frontend_tmp pv) && not (Procdesc.is_captured_var pdesc pv) + && exp_in_set (Exp.Lvar pv) uninit_vars && not (is_type_pointer lhs_typ) -> + let message = + F.asprintf "The value read from %a was never initialized" Exp.pp (Exp.Lvar pv) + in + report message loc (snd extras) + | _ -> + () ) ; {astate with uninit_vars; prepost} | Assign (((lhs_var, _), _), _, _) -> let uninit_vars = D.remove lhs_var astate.uninit_vars in @@ -65,6 +87,9 @@ module TransferFunctions (CFG : ProcCfg.S) = struct match actual_exp with | HilExp.AccessPath ((actual_var, {Typ.desc= Tptr _}), _) -> D.remove actual_var acc + | HilExp.Closure (_, apl) -> + (* remove the captured variables of a block *) + List.fold ~f:(fun acc' ((av, _), _) -> D.remove av acc') ~init:acc apl | _ -> acc) ~init:astate.uninit_vars actuals @@ -97,92 +122,10 @@ let checker {Callbacks.tenv; summary; proc_desc} : Specs.summary = , IdAccessPathMapDomain.empty ) in let invariant_map = - Analyzer.exec_cfg cfg (ProcData.make proc_desc tenv formal_map) ~initial:init ~debug:false - in - (* Check if an expression is in set of variables *) - let exp_in_set exp vset = - let _, pvars = Exp.get_vars exp in - List.exists ~f:(fun pv -> D.mem (Var.of_pvar pv) vset) pvars - in - let zip_actual_formal_params callee_pname actual_params = - match Ondemand.get_proc_desc callee_pname with - | Some pdesc -> - let formals, _ = List.unzip (Procdesc.get_formals pdesc) in - let actual, _ = List.unzip actual_params in - (List.zip actual formals, actual) - | _ -> - (None, []) - in - let deref_actual_params callee_pname actual_params deref_formal_params = - match zip_actual_formal_params callee_pname actual_params with - | None, _ -> - [] - | Some assoc_actual_formal, _ -> - List.fold - ~f:(fun acc (a, f) -> - let fe = Exp.Lvar (Pvar.mk f callee_pname) in - if exp_in_set fe deref_formal_params then a :: acc else acc) - ~init:[] assoc_actual_formal - in - let report_uninit_value uninit_vars instr = - let report message loc = - let issue_id = IssueType.uninitialized_value.unique_id in - let ltr = [Errlog.make_trace_element 0 loc "" []] in - let exn = Exceptions.Checkers (issue_id, Localise.verbatim_desc message) in - Reporting.log_error summary ~loc ~ltr exn - in - match instr with - | Sil.Load (_, Exp.Lvar pv, _, loc) - when not (Pvar.is_frontend_tmp pv) && exp_in_set (Exp.Lvar pv) uninit_vars -> - let message = - F.asprintf "The value read from %a was never initialized" Exp.pp (Exp.Lvar pv) - in - report message loc - | Sil.Store (lhs, _, Exp.Lvar pv, loc) - when not (Pvar.is_frontend_tmp pv) && exp_in_set (Exp.Lvar pv) uninit_vars - && not (is_address_pvar lhs) -> - let message = - F.asprintf "The value read from %a was never initialized" Exp.pp (Exp.Lvar pv) - in - report message loc - | Sil.Call (_, Exp.Const Const.Cfun callee_pname, actual_params, loc, _) - when not intraprocedural_only -> ( - match Summary.read_summary proc_desc callee_pname with - | Some {pre= deref_formal_params; post= _} -> - let deref_actual_params = - deref_actual_params callee_pname actual_params deref_formal_params - in - List.iter - ~f:(fun (e, _) -> - if exp_in_set e uninit_vars && List.mem ~equal:Exp.equal deref_actual_params e then - let message = - F.asprintf "The value of %a is read in %a but was never initialized" Exp.pp e - Typ.Procname.pp callee_pname - in - report message loc) - actual_params - | _ -> - () ) - | _ -> - () - in - let report_on_node node = - List.iter (CFG.instr_ids node) ~f:(fun (instr, node_id_opt) -> - match node_id_opt with - | Some node_id -> ( - match Analyzer.extract_pre node_id invariant_map with - | Some - ( { RecordDomain.uninit_vars= uninitialized_vars - ; RecordDomain.aliased_vars= _ - ; RecordDomain.prepost= _ } - , _ ) -> - report_uninit_value uninitialized_vars instr - | None -> - () ) - | None -> - () ) + Analyzer.exec_cfg cfg + (ProcData.make proc_desc tenv (formal_map, summary)) + ~initial:init ~debug:false in - List.iter (CFG.nodes cfg) ~f:report_on_node ; match Analyzer.extract_post (CFG.id (CFG.exit_node cfg)) invariant_map with | Some ( {RecordDomain.uninit_vars= _; RecordDomain.aliased_vars= _; RecordDomain.prepost= pre, post} diff --git a/infer/src/clang/cAst_utils.ml b/infer/src/clang/cAst_utils.ml index 88dc3e299..23a6ed821 100644 --- a/infer/src/clang/cAst_utils.ml +++ b/infer/src/clang/cAst_utils.ml @@ -568,4 +568,3 @@ let has_block_attribute decl = let is_implicit_decl decl = let decl_info = Clang_ast_proj.get_decl_tuple decl in decl_info.Clang_ast_t.di_is_implicit - diff --git a/infer/tests/codetoanalyze/objc/uninit/Makefile b/infer/tests/codetoanalyze/objc/uninit/Makefile new file mode 100644 index 000000000..108b1c258 --- /dev/null +++ b/infer/tests/codetoanalyze/objc/uninit/Makefile @@ -0,0 +1,20 @@ +# Copyright (c) 2017 - present Facebook, Inc. +# All rights reserved. +# +# This source code is licensed under the BSD style license found in the +# LICENSE file in the root directory of this source tree. An additional grant +# of patent rights can be found in the PATENTS file in the same directory. + +TESTS_DIR = ../../.. + +ANALYZER = checkers +# see explanations in cpp/errors/Makefile for the custom isystem +CLANG_OPTIONS = -x objective-c -c +INFER_OPTIONS = --uninit-only --debug-exceptions --project-root $(TESTS_DIR) +INFERPRINT_OPTIONS = --issues-tests + +SOURCES = uninit_blocks.m + +include $(TESTS_DIR)/clang.make + +infer-out/report.json: $(MAKEFILE_LIST) diff --git a/infer/tests/codetoanalyze/objc/uninit/issues.exp b/infer/tests/codetoanalyze/objc/uninit/issues.exp new file mode 100644 index 000000000..58bd2b4bb --- /dev/null +++ b/infer/tests/codetoanalyze/objc/uninit/issues.exp @@ -0,0 +1 @@ +codetoanalyze/objc/uninit/uninit_blocks.m, A_bad1, 5, UNINITIALIZED_VALUE, [] diff --git a/infer/tests/codetoanalyze/objc/uninit/uninit_blocks.m b/infer/tests/codetoanalyze/objc/uninit/uninit_blocks.m new file mode 100644 index 000000000..61f2e3099 --- /dev/null +++ b/infer/tests/codetoanalyze/objc/uninit/uninit_blocks.m @@ -0,0 +1,49 @@ +/* + * Copyright (c) 2017 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ +#import + +@interface A : NSObject +@end + +typedef void (^MyBlock)(); + +@interface C : NSObject + ++ (void)use_block:(MyBlock)block; + +@end + +@implementation A + ++ (int)ok1 { + __block int a; + [C use_block:^() { + a = 10; + }]; + return a; +}; + ++ (int)bad1 { + int a; + [C use_block:^() { + int x = 0; + }]; + return a; +}; + ++ (instancetype)ok2 { + static id sharedInstance; + ^{ + sharedInstance = [[self alloc] init]; + }(); + + return sharedInstance; +} + +@end