From 0085417e0d0ac4779f8fb258dffd5d6ef0b0ce6c Mon Sep 17 00:00:00 2001 From: Jules Villard Date: Fri, 11 Nov 2016 03:15:04 -0800 Subject: [PATCH] [siof] better error reports using `Errlog.loc_trace`s Summary: This adds generic support for reporting error traces as usual infer issues traces (instead of putting them in the textual description of the error) to Trace.ml and SinkTrace.ml. The siof checker is made to use these new traces, and gets an improved error message mentioning the name of the problematic global as well, which requires a slight API change in Pvar.re. The support in Trace.ml is incomplete: passthroughs are ignored. This missing feature will be needed by Quandary to migrate its error messages. Reviewed By: sblackshear Differential Revision: D4159542 fbshipit-source-id: 8c1101d --- infer/src/IR/Procname.re | 8 ++- infer/src/IR/Procname.rei | 6 +- infer/src/checkers/SinkTrace.ml | 7 ++ infer/src/checkers/SinkTrace.mli | 4 ++ infer/src/checkers/Siof.ml | 69 +++++++++---------- infer/src/checkers/SiofTrace.ml | 56 +++++++++------ infer/src/checkers/SiofTrace.mli | 8 ++- infer/src/checkers/Trace.ml | 35 ++++++++++ infer/src/checkers/Trace.mli | 8 +++ .../tests/codetoanalyze/cpp/checkers/Makefile | 2 +- .../codetoanalyze/cpp/checkers/issues.exp | 8 +-- 11 files changed, 143 insertions(+), 68 deletions(-) diff --git a/infer/src/IR/Procname.re b/infer/src/IR/Procname.re index be1db7de5..87e7f8f29 100644 --- a/infer/src/IR/Procname.re +++ b/infer/src/IR/Procname.re @@ -500,7 +500,13 @@ let is_infer_undefined pn => false }; -let is_globals_initializer (name, _) => string_is_prefix Config.clang_initializer_prefix name; +let get_global_name_of_initializer = + fun + | C (name, _) when string_is_prefix Config.clang_initializer_prefix name => { + let prefix_len = String.length Config.clang_initializer_prefix; + Some (String.sub name prefix_len (String.length name - prefix_len)) + } + | _ => None; /** to_string for C_function type */ diff --git a/infer/src/IR/Procname.rei b/infer/src/IR/Procname.rei index 1f3032311..838a68c7d 100644 --- a/infer/src/IR/Procname.rei +++ b/infer/src/IR/Procname.rei @@ -240,9 +240,9 @@ let is_class_initializer: t => bool; let is_infer_undefined: t => bool; -/** Is this procedure one inserted by Infer to initialize the global variables of this translation - unit? */ -let is_globals_initializer: c => bool; +/** Return the name of the global for which this procedure is the initializer if this is an + initializer, None otherwise. */ +let get_global_name_of_initializer: t => option string; /** Pretty print a proc name. */ diff --git a/infer/src/checkers/SinkTrace.ml b/infer/src/checkers/SinkTrace.ml index 53beecce0..c8ff7fd89 100644 --- a/infer/src/checkers/SinkTrace.ml +++ b/infer/src/checkers/SinkTrace.ml @@ -24,6 +24,10 @@ module type S = sig (** update sink with the given call site *) val with_callsite : t -> CallSite.t -> t + + val to_sink_loc_trace : + ?desc_of_sink:(Sink.t -> string) -> ?sink_should_nest:(Sink.t -> bool) -> + sink_path -> Errlog.loc_trace_elem list end module MakeSink(TraceElem : TraceElem.S) = struct @@ -49,6 +53,9 @@ module Make (TraceElem : TraceElem.S) = struct (fun (passthroughs, _, sinks) -> passthroughs, sinks) (get_reportable_paths t ~trace_of_pname) + let to_sink_loc_trace ?desc_of_sink ?sink_should_nest (passthroughs, sinks) = + to_loc_trace ?desc_of_sink ?sink_should_nest (passthroughs, [], sinks) + let with_callsite t call_site = IList.fold_left (fun t_acc sink -> diff --git a/infer/src/checkers/SinkTrace.mli b/infer/src/checkers/SinkTrace.mli index ad49b2e39..4ea3de3aa 100644 --- a/infer/src/checkers/SinkTrace.mli +++ b/infer/src/checkers/SinkTrace.mli @@ -20,6 +20,10 @@ module type S = sig (** update sink with the given call site *) val with_callsite : t -> CallSite.t -> t + + val to_sink_loc_trace : + ?desc_of_sink:(Sink.t -> string) -> ?sink_should_nest:(Sink.t -> bool) -> + sink_path -> Errlog.loc_trace_elem list end module MakeSink (TraceElem : TraceElem.S) : diff --git a/infer/src/checkers/Siof.ml b/infer/src/checkers/Siof.ml index c577b9b82..5b1095b8e 100644 --- a/infer/src/checkers/Siof.ml +++ b/infer/src/checkers/Siof.ml @@ -45,16 +45,12 @@ module TransferFunctions (CFG : ProcCfg.S) = struct if globals = [] then Domain.Bottom else - let sink_of_global global pname loc = - let site = CallSite.make pname loc in - SiofTrace.Sink.make global site in - let pname = Procdesc.get_proc_name pdesc in let trace = match astate with | Domain.Bottom -> SiofTrace.initial | Domain.NonBottom t -> t in let globals_trace = IList.fold_left (fun trace_acc global -> - SiofTrace.add_sink (sink_of_global global pname loc) trace_acc) + SiofTrace.add_sink (SiofTrace.make_access global loc) trace_acc) trace globals in Domain.NonBottom globals_trace @@ -102,7 +98,7 @@ module Analyzer = module Interprocedural = Analyzer.Interprocedural (Summary) -let report_siof trace pdesc tenv loc = +let report_siof tenv trace pdesc gname loc = let trace_of_pname pname = match Summary.read_summary tenv pdesc pname with | Some (SiofDomain.NonBottom summary) -> summary @@ -110,40 +106,43 @@ let report_siof trace pdesc tenv loc = let pp_sink f sink = let pp_source f v = match Pvar.get_source_file v with - | Some source_file when DB.source_file_equal DB.source_file_empty source_file -> - F.fprintf f "" - | None -> - F.fprintf f "" - | Some source_file -> - F.fprintf f " from file %s" (DB.source_file_to_string source_file) in + | Some source_file when not (DB.source_file_equal DB.source_file_empty source_file) -> + F.fprintf f " from file %s" (DB.source_file_to_string source_file) + | _ -> + () in let v = SiofTrace.Sink.kind sink in F.fprintf f "%s%a" (Pvar.get_simplified_name v) pp_source v in - let pp_path_part f path = - let pp_path f path = - let pp_sep f () = F.fprintf f " => " in - let pp_elem f (sink, _) = - CallSite.pp f (SiofTrace.Sink.call_site sink) in - (F.pp_print_list ~pp_sep) pp_elem f path in - if (IList.length path > 1) - then F.fprintf f "Full path: %a" pp_path path in - - let report_one_path (_, path) = + let trace_of_error path = + let desc_of_sink sink = + let callsite = SiofTrace.Sink.call_site sink in + if SiofTrace.is_intraprocedural_access sink then + Format.asprintf "access to %s" (Pvar.get_simplified_name (SiofTrace.Sink.kind sink)) + else + Format.asprintf "call to %a" Procname.pp (CallSite.pname callsite) in + let sink_should_nest sink = not (SiofTrace.is_intraprocedural_access sink) in + let trace_elem_of_global = + Errlog.make_trace_element 0 loc + (Format.asprintf "initialization of %s" gname) + [] in + trace_elem_of_global::(SiofTrace.to_sink_loc_trace ~desc_of_sink ~sink_should_nest path) in + + let report_one_path ((_, path) as sink_path) = let final_sink = fst (IList.hd path) in let description = F.asprintf - "This global variable initializer accesses the following globals in another translation \ - unit: %a. %a" - pp_sink final_sink - pp_path_part (IList.rev path) in + "The initializer of %s accesses global variables in another translation unit: %a" + gname + pp_sink final_sink in + let ltr = trace_of_error sink_path in let caller_pname = Procdesc.get_proc_name pdesc in - let exn = Exceptions.Checkers - ("STATIC_INITIALIZATION_ORDER_FIASCO", Localise.verbatim_desc description) in - Reporting.log_error caller_pname ~loc exn in + let msg = Localise.to_string Localise.static_initialization_order_fiasco in + let exn = Exceptions.Checkers (msg, Localise.verbatim_desc description) in + Reporting.log_error caller_pname ~loc ~ltr exn in IList.iter report_one_path (SiofTrace.get_reportable_sink_paths trace ~trace_of_pname) -let siof_check pdesc tenv = function +let siof_check tenv pdesc gname = function | Some (SiofDomain.NonBottom post) -> let attrs = Procdesc.get_attributes pdesc in let is_orig_file f = match attrs.ProcAttributes.translation_unit with @@ -158,15 +157,15 @@ let siof_check pdesc tenv = function (fun sink -> is_foreign (SiofTrace.Sink.kind sink)) (SiofTrace.sinks post) in if not (SiofTrace.Sinks.is_empty foreign_global_sinks) - then report_siof post pdesc tenv attrs.ProcAttributes.loc; + then report_siof tenv post pdesc gname attrs.ProcAttributes.loc; | Some SiofDomain.Bottom | None -> () let checker ({ Callbacks.tenv; proc_desc } as callback) = let post = Interprocedural.checker callback ProcData.empty_extras in let pname = Procdesc.get_proc_name proc_desc in - match pname with - | Procname.C c when Procname.is_globals_initializer c -> - siof_check proc_desc tenv post - | _ -> + match Procname.get_global_name_of_initializer pname with + | Some gname -> + siof_check tenv proc_desc gname post + | None -> () diff --git a/infer/src/checkers/SiofTrace.ml b/infer/src/checkers/SiofTrace.ml index 1300f271c..74912a444 100644 --- a/infer/src/checkers/SiofTrace.ml +++ b/infer/src/checkers/SiofTrace.ml @@ -12,39 +12,51 @@ open! Utils module F = Format module L = Logging -module Globals = struct +module Global = struct type t = Pvar.t let compare = Pvar.compare let pp fmt pvar = (Pvar.pp pe_text) fmt pvar end -include SinkTrace.Make(struct - module Kind = Globals +module TraceElem = struct + module Kind = Global - type t = { - site : CallSite.t; - kind: Kind.t; - } + type t = { + site : CallSite.t; + kind: [`Call | `Access] * Kind.t; + } - let call_site { site; } = site + let call_site { site; } = site - let kind { kind; } = kind + let kind { kind; } = snd kind - let make kind site = { kind; site; } + let make kind site = { kind = (`Call, kind); site; } - let with_callsite t site = { t with site; } + let with_callsite { kind=(_, kind); } site = { kind=(`Call, kind); site; } - let compare t1 t2 = - CallSite.compare t1.site t2.site - |> next Kind.compare t1.kind t2.kind + let compare t1 t2 = + let n = CallSite.compare t1.site t2.site in + if n <> 0 then n + else + let n = tags_compare (fst t1.kind) (fst t2.kind) in + if n <> 0 then n + else Kind.compare (snd t1.kind) (snd t2.kind) - let pp fmt { site; kind; } = - F.fprintf fmt "Access to %a at %a" Kind.pp kind CallSite.pp site + let pp fmt { site; kind; } = + F.fprintf fmt "Access to %a at %a" Kind.pp (snd kind) CallSite.pp site - module Set = PrettyPrintable.MakePPSet (struct - type nonrec t = t - let compare = compare - let pp_element = pp - end) + module Set = PrettyPrintable.MakePPSet (struct + type nonrec t = t + let compare = compare + let pp_element = pp + end) - end) +end + +include SinkTrace.Make(TraceElem) + +let make_access kind loc = + let site = CallSite.make Procname.empty_block loc in + { TraceElem.kind = (`Access, kind); site; } + +let is_intraprocedural_access { TraceElem.kind=(kind, _); } = kind = `Access diff --git a/infer/src/checkers/SiofTrace.mli b/infer/src/checkers/SiofTrace.mli index 7fac6e2db..521ef98dc 100644 --- a/infer/src/checkers/SiofTrace.mli +++ b/infer/src/checkers/SiofTrace.mli @@ -9,11 +9,15 @@ open! Utils -module Globals : +module Global : sig type t = Pvar.t val compare : t -> t -> int val pp : Format.formatter -> t -> unit end -include SinkTrace.S with module Sink.Kind = Globals +include SinkTrace.S with module Sink.Kind = Global + +val make_access : Global.t -> Location.t -> Sink.t + +val is_intraprocedural_access : Sink.t -> bool diff --git a/infer/src/checkers/Trace.ml b/infer/src/checkers/Trace.ml index 524edf593..55046a656 100644 --- a/infer/src/checkers/Trace.ml +++ b/infer/src/checkers/Trace.ml @@ -50,6 +50,14 @@ module type S = sig (** get a path for each of the reportable source -> sink flows in this trace *) val get_reportable_paths : t -> trace_of_pname:(Procname.t -> t) -> path list + (** create a loc_trace from a path; [source_should_nest s] should be true when we are going one + deeper into a call-chain, ie when lt_level should be bumper in the next loc_trace_elem, and + similarly for [sink_should_nest] *) + val to_loc_trace : + ?desc_of_source:(Source.t -> string) -> ?source_should_nest:(Source.t -> bool) -> + ?desc_of_sink:(Sink.t -> string) -> ?sink_should_nest:(Sink.t -> bool) -> + path -> Errlog.loc_trace + (** create a trace from a source *) val of_source : Source.t -> t @@ -214,6 +222,33 @@ module Make (Spec : Spec) = struct passthroughs, sources_passthroughs, sinks_passthroughs) (get_reports t) + let to_loc_trace + ?(desc_of_source=fun source -> + let callsite = Source.call_site source in + Format.asprintf "call to %a" Procname.pp (CallSite.pname callsite)) + ?(source_should_nest=(fun _ -> true)) + ?(desc_of_sink=fun sink -> + let callsite = Sink.call_site sink in + Format.asprintf "call to %a" Procname.pp (CallSite.pname callsite)) + ?(sink_should_nest=(fun _ -> true)) + (_, sources, sinks) = + let trace_elem_of_path_elem call_site desc should_nest = + let level = ref 0 in + fun (elem, _) -> + let lt_level = !level in + let desc = desc elem in + let callsite = call_site elem in + if should_nest elem then incr level; + Errlog.make_trace_element lt_level (CallSite.loc callsite) desc [] in + let trace_elem_of_source = + trace_elem_of_path_elem Source.call_site desc_of_source source_should_nest in + let trace_elem_of_sink = + trace_elem_of_path_elem Sink.call_site desc_of_sink sink_should_nest in + (* reverse sinks intentionally, do not reverse sources(?) *) + IList.rev_append + (IList.rev_map trace_elem_of_source sources) + (IList.map trace_elem_of_sink (IList.rev sinks)) + let of_source source = let sources = Sources.singleton source in let passthroughs = Passthroughs.empty in diff --git a/infer/src/checkers/Trace.mli b/infer/src/checkers/Trace.mli index 21ccdaecc..4ad71fb55 100644 --- a/infer/src/checkers/Trace.mli +++ b/infer/src/checkers/Trace.mli @@ -50,6 +50,14 @@ module type S = sig (** get a path for each of the reportable source -> sink flows in this trace *) val get_reportable_paths : t -> trace_of_pname:(Procname.t -> t) -> path list + (** create a loc_trace from a path; [source_should_nest s] should be true when we are going one + deeper into a call-chain, ie when lt_level should be bumper in the next loc_trace_elem, and + similarly for [sink_should_nest] *) + val to_loc_trace : + ?desc_of_source:(Source.t -> string) -> ?source_should_nest:(Source.t -> bool) -> + ?desc_of_sink:(Sink.t -> string) -> ?sink_should_nest:(Sink.t -> bool) -> + path -> Errlog.loc_trace + (** create a trace from a source *) val of_source : Source.t -> t diff --git a/infer/tests/codetoanalyze/cpp/checkers/Makefile b/infer/tests/codetoanalyze/cpp/checkers/Makefile index d79c6023e..3782206f0 100644 --- a/infer/tests/codetoanalyze/cpp/checkers/Makefile +++ b/infer/tests/codetoanalyze/cpp/checkers/Makefile @@ -12,7 +12,7 @@ ANALYZER = checkers # see explanations in cpp/errors/Makefile for the custom isystem CLANG_OPTIONS = -x c++ -std=c++11 -nostdinc++ -isystem$(MODELS_DIR)/cpp/include -isystem$(CLANG_INCLUDES)/c++/v1/ -c INFER_OPTIONS = --cxx --ml-buckets cpp --no-filtering --debug-exceptions -INFERPRINT_OPTIONS = --issues-txt +INFERPRINT_OPTIONS = --issues-tests SOURCES = \ siof/const.cpp \ diff --git a/infer/tests/codetoanalyze/cpp/checkers/issues.exp b/infer/tests/codetoanalyze/cpp/checkers/issues.exp index 7d260e6df..a7f0a843b 100644 --- a/infer/tests/codetoanalyze/cpp/checkers/issues.exp +++ b/infer/tests/codetoanalyze/cpp/checkers/issues.exp @@ -1,4 +1,4 @@ -siof/const_use.cpp:17: ERROR: STATIC_INITIALIZATION_ORDER_FIASCO This global variable initializer accesses the following globals in another translation unit: u. -siof/pod_across_translation_units-1.cpp:13: ERROR: STATIC_INITIALIZATION_ORDER_FIASCO This global variable initializer accesses the following globals in another translation unit: y from file siof/pod_across_translation_units-2.cpp. Full path: baz at [line 13] => bar at [line 12] => foo at [line 11] -siof/siof_across_translation_units-1.cpp:21: ERROR: STATIC_INITIALIZATION_ORDER_FIASCO This global variable initializer accesses the following globals in another translation unit: another_global_object from file siof/siof_across_translation_units-1.cpp. -siof/siof_across_translation_units-1.cpp:21: ERROR: STATIC_INITIALIZATION_ORDER_FIASCO This global variable initializer accesses the following globals in another translation unit: global_object. +siof/const_use.cpp, __infer_globals_initializer_use_u, 0, STATIC_INITIALIZATION_ORDER_FIASCO +siof/pod_across_translation_units-1.cpp, __infer_globals_initializer_x, 0, STATIC_INITIALIZATION_ORDER_FIASCO +siof/siof_across_translation_units-1.cpp, __infer_globals_initializer_another_global_object, 0, STATIC_INITIALIZATION_ORDER_FIASCO +siof/siof_across_translation_units-1.cpp, __infer_globals_initializer_another_global_object, 0, STATIC_INITIALIZATION_ORDER_FIASCO