From 102018734f6dd65ef7d6b383a21998141508c03e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ezgi=20=C3=87i=C3=A7ek?= Date: Mon, 8 Jul 2019 09:29:37 -0700 Subject: [PATCH] [inefficientKeysetIterator] Add missing type checks Summary: Adding typechecks to prevent potential FPs like the added test Reviewed By: ngorogiannis Differential Revision: D16149511 fbshipit-source-id: 6d3fe0ad4 --- infer/src/absint/PatternMatch.ml | 2 ++ infer/src/absint/PatternMatch.mli | 3 ++ .../src/checkers/inefficientKeysetIterator.ml | 34 +++++++++++++------ .../java/inefficientKeysetIterator/Test.java | 8 +++++ 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/infer/src/absint/PatternMatch.ml b/infer/src/absint/PatternMatch.ml index 07531233c..5d3e8be9a 100644 --- a/infer/src/absint/PatternMatch.ml +++ b/infer/src/absint/PatternMatch.ml @@ -78,6 +78,8 @@ let implements_io class_name = implements ("java.io." ^ class_name) let implements_map = implements "java.util.Map" +let implements_set = implements "java.util.Set" + let implements_map_entry = implements "java.util.Map$Entry" let implements_queue = implements "java.util.Queue" diff --git a/infer/src/absint/PatternMatch.mli b/infer/src/absint/PatternMatch.mli index e7ca0d942..c8165ad96 100644 --- a/infer/src/absint/PatternMatch.mli +++ b/infer/src/absint/PatternMatch.mli @@ -64,6 +64,9 @@ val implements_io : string -> Tenv.t -> string -> bool val implements_map : Tenv.t -> string -> bool (** Check whether class implements a Java's Map *) +val implements_set : Tenv.t -> string -> bool +(** Check whether class implements a Java's Set *) + val implements_map_entry : Tenv.t -> string -> bool (** Check whether class implements a Java's Map$Entry *) diff --git a/infer/src/checkers/inefficientKeysetIterator.ml b/infer/src/checkers/inefficientKeysetIterator.ml index bd0a51f4b..2165285bf 100644 --- a/infer/src/checkers/inefficientKeysetIterator.ml +++ b/infer/src/checkers/inefficientKeysetIterator.ml @@ -16,9 +16,11 @@ let find_loaded_pvar id = function None -let find_first_arg_id ~fun_name ~lhs_f = function +let find_first_arg_id ~fun_name ~class_name_f ~lhs_f = function | Sil.Call ((lhs_id, _), Exp.Const (Const.Cfun pname), (Exp.Var rhs_id, _) :: _, _, _) - when lhs_f lhs_id && String.equal fun_name (Typ.Procname.get_method pname) -> + when lhs_f lhs_id + && String.equal fun_name (Typ.Procname.get_method pname) + && Typ.Procname.get_class_name pname |> Option.exists ~f:class_name_f -> Some rhs_id | _ -> None @@ -31,26 +33,29 @@ let find_first_arg_id ~fun_name ~lhs_f = function n$X+1 = fun_name(n$X,....) *&$irvarZ = n$X+1 *) -let find_first_arg_pvar node ~fun_name = +let find_first_arg_pvar node ~fun_name ~class_name_f = let instrs = Procdesc.Node.get_instrs node in if Instrs.count instrs >= 4 then let instr_arr = Instrs.get_underlying_not_reversed instrs in match instr_arr.(3) with | Sil.Store (Exp.Lvar _, _, Exp.Var rhs_id, _) -> - find_first_arg_id ~fun_name ~lhs_f:(Ident.equal rhs_id) instr_arr.(2) + find_first_arg_id ~fun_name ~class_name_f ~lhs_f:(Ident.equal rhs_id) instr_arr.(2) |> Option.bind ~f:(fun arg_id -> find_loaded_pvar arg_id instr_arr.(0)) | _ -> None else None -let report_matching_get summary pvar loop_nodes : unit = +let report_matching_get tenv summary pvar loop_nodes : unit = LoopNodes.iter (fun node -> let instrs = Procdesc.Node.get_instrs node in if Instrs.count instrs >= 5 then let instr_arr = Instrs.get_underlying_not_reversed instrs in - find_first_arg_id ~fun_name:"get" ~lhs_f:(fun _ -> true) instr_arr.(3) + find_first_arg_id ~fun_name:"get" + ~class_name_f:(PatternMatch.implements_map tenv) + ~lhs_f:(fun _ -> true) + instr_arr.(3) |> Option.iter ~f:(fun arg_id -> find_loaded_pvar arg_id instr_arr.(0) |> Option.iter ~f:(fun arg_pvar -> @@ -78,19 +83,28 @@ let when_dominating_pred_satisfies idom my_node ~f = match preds with [pred_node] -> f pred_node | _ -> () -let checker Callbacks.{summary} : Summary.t = +let checker Callbacks.{summary; tenv} : Summary.t = let proc_desc = Summary.get_proc_desc summary in let cfg = CFG.from_pdesc proc_desc in let _, loop_head_to_loop_nodes = Loop_control.get_loop_control_maps cfg in let idom = Dominators.get_idoms proc_desc in Procdesc.NodeMap.iter (fun loop_head loop_nodes -> - if find_first_arg_pvar loop_head ~fun_name:"hasNext" |> Option.is_some then + if + find_first_arg_pvar loop_head ~fun_name:"hasNext" + ~class_name_f:(PatternMatch.implements_iterator tenv) + |> Option.is_some + then when_dominating_pred_satisfies idom loop_head ~f:(fun itr_node -> - if Option.is_some (find_first_arg_pvar itr_node ~fun_name:"iterator") then + if + Option.is_some + (find_first_arg_pvar itr_node ~fun_name:"iterator" + ~class_name_f:(PatternMatch.implements_set tenv)) + then when_dominating_pred_satisfies idom itr_node ~f:(fun keySet_node -> find_first_arg_pvar keySet_node ~fun_name:"keySet" + ~class_name_f:(PatternMatch.implements_map tenv) |> Option.iter ~f:(fun get_pvar -> - report_matching_get summary get_pvar loop_nodes ) ) ) ) + report_matching_get tenv summary get_pvar loop_nodes ) ) ) ) loop_head_to_loop_nodes ; summary diff --git a/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java b/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java index e24e182db..0260374fd 100644 --- a/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java +++ b/infer/tests/codetoanalyze/java/inefficientKeysetIterator/Test.java @@ -4,6 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. */ +import android.os.Bundle; import java.util.HashMap; import java.util.Iterator; import java.util.Map; @@ -57,4 +58,11 @@ class Test { testMap2.get(key); } } + + // Bundle doesn't implement Map hence have any entrySet + public void from_bundle_ok(Bundle extras) { + for (String key : extras.keySet()) { + Object t = extras.get(key); + } + } }