From 5fe40bae12ba0560a2e9d4413a3d88a1096f311e Mon Sep 17 00:00:00 2001 From: Jules Villard Date: Mon, 17 Oct 2016 11:04:10 -0700 Subject: [PATCH] [siof] check origin of globals and complain if potential siof Summary: Checker for the Static Initialization Order Fiasco pattern: https://isocpp.org/wiki/faq/ctors#static-init-order 1. Collect all globals (transitively) accessed in any given procedure. 2. Once the interprocedural analysis has finished, look at globals accessed in initializers that do not belong to the current translation unit. Reviewed By: sblackshear Differential Revision: D3780266 fbshipit-source-id: 1d07161 --- infer/lib/python/infer.py | 10 -- infer/src/IR/ProcAttributes.re | 2 + infer/src/IR/ProcAttributes.rei | 1 + infer/src/IR/Procname.re | 2 + infer/src/IR/Procname.rei | 5 + infer/src/IR/Pvar.re | 12 ++ infer/src/IR/Pvar.rei | 4 + infer/src/backend/specs.ml | 4 +- infer/src/backend/specs.mli | 1 + infer/src/checkers/addressTaken.ml | 8 +- infer/src/checkers/registerCheckers.ml | 1 + infer/src/checkers/siof.ml | 112 ++++++++++++++++++ infer/src/checkers/siof.mli | 10 ++ infer/src/clang/cFrontend_utils.ml | 8 +- infer/src/clang/cMethod_trans.ml | 1 + .../tests/codetoanalyze/cpp/checkers/Makefile | 26 ++++ .../codetoanalyze/cpp/checkers/issues.exp | 2 + .../siof/pod_across_translation_units-1.cpp | 14 +++ .../siof/pod_across_translation_units-2.cpp | 12 ++ .../siof/pod_same_translation_unit.cpp | 23 ++++ .../siof/siof_across_translation_units-1.cpp | 21 ++++ 21 files changed, 260 insertions(+), 19 deletions(-) create mode 100644 infer/src/checkers/siof.ml create mode 100644 infer/src/checkers/siof.mli create mode 100644 infer/tests/codetoanalyze/cpp/checkers/Makefile create mode 100644 infer/tests/codetoanalyze/cpp/checkers/issues.exp create mode 100644 infer/tests/codetoanalyze/cpp/checkers/siof/pod_across_translation_units-1.cpp create mode 100644 infer/tests/codetoanalyze/cpp/checkers/siof/pod_across_translation_units-2.cpp create mode 100644 infer/tests/codetoanalyze/cpp/checkers/siof/pod_same_translation_unit.cpp create mode 100644 infer/tests/codetoanalyze/cpp/checkers/siof/siof_across_translation_units-1.cpp diff --git a/infer/lib/python/infer.py b/infer/lib/python/infer.py index 8d1fa6b61..f38da14a6 100755 --- a/infer/lib/python/infer.py +++ b/infer/lib/python/infer.py @@ -124,14 +124,6 @@ def create_argparser(parents=[]): return parser -def validate_args(mode, args): - if mode is not None and mode.LANG == ['clang'] and \ - args.analyzer == config.ANALYZER_CHECKERS: - utils.stderr('error: checkers are only enabled for Java.') - if not args.debug: - exit(1) - - def main(): toplevel_envvar_value = os.environ.get(TOP_LEVEL_ENVVAR, None) is_toplevel_instance = False @@ -158,8 +150,6 @@ def main(): args = global_argparser.parse_args(to_parse) - validate_args(imported_module, args) - remove_infer_out = (imported_module is not None and not args.reactive and capture_module_name != 'analyze' and diff --git a/infer/src/IR/ProcAttributes.re b/infer/src/IR/ProcAttributes.re index 6f74a1101..2f85c11ab 100644 --- a/infer/src/IR/ProcAttributes.re +++ b/infer/src/IR/ProcAttributes.re @@ -39,6 +39,7 @@ type t = { is_synthetic_method: bool, /** the procedure is a synthetic method */ language: Config.language, /** language of the procedure */ loc: Location.t, /** location of this procedure in the source code */ + translation_unit: option DB.source_file, /** translation unit to which the procedure belongs */ mutable locals: list (Mangled.t, Typ.t), /** name and type of local variables */ method_annotation: Annot.Method.t, /** annotations for java methods */ objc_accessor: option objc_accessor_type, /** type of ObjC accessor, if any */ @@ -67,6 +68,7 @@ let default proc_name language => { is_synthetic_method: false, language, loc: Location.dummy, + translation_unit: None, locals: [], method_annotation: Annot.Method.empty, objc_accessor: None, diff --git a/infer/src/IR/ProcAttributes.rei b/infer/src/IR/ProcAttributes.rei index 9ca71d253..a76977239 100644 --- a/infer/src/IR/ProcAttributes.rei +++ b/infer/src/IR/ProcAttributes.rei @@ -33,6 +33,7 @@ type t = { is_synthetic_method: bool, /** the procedure is a synthetic method */ language: Config.language, /** language of the procedure */ loc: Location.t, /** location of this procedure in the source code */ + translation_unit: option DB.source_file, /** translation unit to which the procedure belongs */ mutable locals: list (Mangled.t, Typ.t), /** name and type of local variables */ method_annotation: Annot.Method.t, /** annotations for java methods */ objc_accessor: option objc_accessor_type, /** type of ObjC accessor, if any */ diff --git a/infer/src/IR/Procname.re b/infer/src/IR/Procname.re index 606175bed..c371d3ed6 100644 --- a/infer/src/IR/Procname.re +++ b/infer/src/IR/Procname.re @@ -493,6 +493,8 @@ let is_infer_undefined pn => false }; +let is_globals_initializer (name, _) => string_is_prefix Config.clang_initializer_prefix name; + /** to_string for C_function type */ let to_readable_string (c1, c2) verbose => { diff --git a/infer/src/IR/Procname.rei b/infer/src/IR/Procname.rei index 39f3f26f6..0b334a888 100644 --- a/infer/src/IR/Procname.rei +++ b/infer/src/IR/Procname.rei @@ -236,6 +236,11 @@ 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; + + /** Pretty print a proc name. */ let pp: Format.formatter => t => unit; diff --git a/infer/src/IR/Pvar.re b/infer/src/IR/Pvar.re index 7308f8f71..1bc527a72 100644 --- a/infer/src/IR/Pvar.re +++ b/infer/src/IR/Pvar.re @@ -340,3 +340,15 @@ let mk_abduced_ref_param (proc_name: Procname.t) (pv: t) (loc: Location.t) :t => let name = Mangled.from_string ("$REF_PARAM_" ^ Procname.to_unique_id proc_name); {pv_name: name, pv_kind: Abduced_ref_param proc_name pv loc} }; + +let get_source_file pvar => + switch pvar.pv_kind { + | Global_var f => Some f + | _ => None + }; + +let module Set = PrettyPrintable.MakePPSet { + type nonrec t = t; + let compare = compare; + let pp_element = pp pe_text; +}; diff --git a/infer/src/IR/Pvar.rei b/infer/src/IR/Pvar.rei index 8717dd12e..7fec94c36 100644 --- a/infer/src/IR/Pvar.rei +++ b/infer/src/IR/Pvar.rei @@ -133,3 +133,7 @@ let to_seed: t => t; /** Convert a pvar to string. */ let to_string: t => string; + +let get_source_file: t => option DB.source_file; + +let module Set: PrettyPrintable.PPSet with type elt = t; diff --git a/infer/src/backend/specs.ml b/infer/src/backend/specs.ml index b503e4065..4d51f989a 100644 --- a/infer/src/backend/specs.ml +++ b/infer/src/backend/specs.ml @@ -324,8 +324,9 @@ type payload = typestate : unit TypeState.t option; (** final typestate *) calls: call_summary option; crashcontext_frame: Stacktree_j.stacktree option; - quandary : QuandarySummary.t option; (** Proc location and blame_range info for crashcontext analysis *) + quandary : QuandarySummary.t option; + globals_read: Pvar.Set.t option; } type summary = @@ -760,6 +761,7 @@ let empty_payload = calls = None; crashcontext_frame = None; quandary = None; + globals_read = None; } (** [init_summary (depend_list, nodes, diff --git a/infer/src/backend/specs.mli b/infer/src/backend/specs.mli index d7b625ff8..ae7cbf480 100644 --- a/infer/src/backend/specs.mli +++ b/infer/src/backend/specs.mli @@ -130,6 +130,7 @@ type payload = crashcontext_frame: Stacktree_j.stacktree option; (** Procedure location and blame_range info for crashcontext analysis *) quandary : QuandarySummary.t option; + globals_read: Pvar.Set.t option; } (** Procedure summary *) diff --git a/infer/src/checkers/addressTaken.ml b/infer/src/checkers/addressTaken.ml index 06e44f501..a7dbb7365 100644 --- a/infer/src/checkers/addressTaken.ml +++ b/infer/src/checkers/addressTaken.ml @@ -9,13 +9,7 @@ open! Utils -module PvarSet = PrettyPrintable.MakePPSet(struct - type t = Pvar.t - let compare = Pvar.compare - let pp_element = (Pvar.pp pe_text) - end) - -module Domain = AbstractDomain.FiniteSet(PvarSet) +module Domain = AbstractDomain.FiniteSet(Pvar.Set) module TransferFunctions (CFG : ProcCfg.S) = struct module CFG = CFG diff --git a/infer/src/checkers/registerCheckers.ml b/infer/src/checkers/registerCheckers.ml index f42fbff58..a2d73b919 100644 --- a/infer/src/checkers/registerCheckers.ml +++ b/infer/src/checkers/registerCheckers.ml @@ -51,6 +51,7 @@ let active_procedure_checkers () = CheckDeadCode.callback_check_dead_code, false; Checkers.callback_print_access_to_globals, false; CppTaintAnalysis.checker, Config.quandary; + Siof.checker, checkers_enabled; ] in IList.map (fun (x, y) -> (x, y, Some Config.Clang)) l in diff --git a/infer/src/checkers/siof.ml b/infer/src/checkers/siof.ml new file mode 100644 index 000000000..0d4ce3ebb --- /dev/null +++ b/infer/src/checkers/siof.ml @@ -0,0 +1,112 @@ +(* + * Copyright (c) 2016 - 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. + *) + +open! Utils + +module F = Format +module L = Logging + +module Domain = AbstractDomain.FiniteSet(Pvar.Set) + +module Summary = Summary.Make (struct + type summary = Domain.astate + + let update_payload astate payload = + { payload with Specs.globals_read = Some astate } + + let read_from_payload payload = + match payload.Specs.globals_read with + | Some astate -> astate + | None -> Domain.initial + end) + +module TransferFunctions (CFG : ProcCfg.S) = struct + module CFG = CFG + module Domain = Domain + type extras = ProcData.no_extras + + let get_globals e = + Exp.get_vars e |> snd |> IList.filter Pvar.is_global |> Domain.of_list + + let exec_instr astate { ProcData.pdesc; tenv } _ (instr : Sil.instr) = match instr with + | Load (_, exp, _, _) + | Store (_, _, exp, _) + | Prune (exp, _, _, _) -> + let globals = get_globals exp in + Domain.union astate globals + | Call (_, Const (Cfun callee_pname), params, _, _) -> + let param_globals = + IList.map fst params + |> IList.map get_globals + |> IList.fold_left Domain.union astate in + let callee_globals = + Option.default Domain.initial + @@ Summary.read_summary tenv pdesc callee_pname in + Domain.union callee_globals param_globals + | Call (_, _, params, _, _) -> + IList.map fst params + |> IList.map get_globals + |> IList.fold_left Domain.union astate + | Declare_locals _ | Remove_temps _ | Abstract _ | Nullify _ -> + astate +end + +module Analyzer = + AbstractInterpreter.Make + (ProcCfg.Backward(ProcCfg.Exceptional)) + (Scheduler.ReversePostorder) + (TransferFunctions) + +module Interprocedural = Analyzer.Interprocedural (Summary) + +let report_siof pname loc bad_globals = + let pp_desc fmt () = + let pp_var f v = + 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 -> + Format.fprintf f "" + | None -> + Format.fprintf f "" + | Some source_file -> + Format.fprintf f " from file %s" (DB.source_file_to_string source_file) in + Format.fprintf f "%s%a" (Pvar.get_simplified_name v) pp_source v in + let pp_set f s = pp_seq pp_var f (Pvar.Set.elements s) in + Format.fprintf fmt + "This global variable initializer accesses the following globals in another translation \ + unit: %a" + pp_set bad_globals in + let description = pp_to_string pp_desc () in + let exn = Exceptions.Checkers + ("STATIC_INITIALIZATION_ORDER_FIASCO", Localise.verbatim_desc description) in + Reporting.log_error pname ~loc exn + + +let siof_check pdesc = function + | Some post -> + let attrs = Cfg.Procdesc.get_attributes pdesc in + let is_orig_file f = match attrs.ProcAttributes.translation_unit with + | Some orig_file -> + let orig_path = DB.source_file_to_abs_path orig_file in + string_equal orig_path @@ DB.source_file_to_abs_path f + | None -> false in + let is_foreign v = Option.map_default + (fun f -> not @@ is_orig_file f) false (Pvar.get_source_file v) in + let foreign_globals = Domain.filter is_foreign post in + if not (Domain.is_empty foreign_globals) then + report_siof (Cfg.Procdesc.get_proc_name pdesc) attrs.ProcAttributes.loc foreign_globals; + | None -> () + +let checker callback = + let pdesc = callback.Callbacks.proc_desc in + let post = Interprocedural.checker callback ProcData.empty_extras in + let pname = Cfg.Procdesc.get_proc_name pdesc in + match pname with + | Procname.C c when Procname.is_globals_initializer c -> + siof_check pdesc post + | _ -> () diff --git a/infer/src/checkers/siof.mli b/infer/src/checkers/siof.mli new file mode 100644 index 000000000..06800b4a8 --- /dev/null +++ b/infer/src/checkers/siof.mli @@ -0,0 +1,10 @@ +(* + * Copyright (c) 2016 - 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. + *) + +val checker: Callbacks.proc_callback_args -> unit diff --git a/infer/src/clang/cFrontend_utils.ml b/infer/src/clang/cFrontend_utils.ml index 4736e6539..c3ffa94f6 100644 --- a/infer/src/clang/cFrontend_utils.ml +++ b/infer/src/clang/cFrontend_utils.ml @@ -756,7 +756,13 @@ struct if var_decl_info.Clang_ast_t.vdi_is_static_local then Mangled.from_string ((Procname.to_string outer_procname) ^ "_" ^ name_string) else simple_name in - Pvar.mk_global global_mangled_name source_file + let translation_unit = + match var_decl_info.Clang_ast_t.vdi_storage_class with + | Some "extern" -> + DB.source_file_empty + | _ -> + source_file in + Pvar.mk_global global_mangled_name translation_unit else if not should_be_mangled then Pvar.mk simple_name procname else let start_location = fst decl_info.Clang_ast_t.di_source_range in diff --git a/infer/src/clang/cMethod_trans.ml b/infer/src/clang/cMethod_trans.ml index 05947012d..2aabf03fe 100644 --- a/infer/src/clang/cMethod_trans.ml +++ b/infer/src/clang/cMethod_trans.ml @@ -414,6 +414,7 @@ let create_local_procdesc trans_unit_ctx cfg tenv ms fbody captured is_objc_inst is_objc_instance_method = is_objc_inst_method; is_cpp_instance_method = is_cpp_inst_method; loc = loc_start; + translation_unit = Some trans_unit_ctx.CFrontend_config.source_file; method_annotation; ret_type; } in diff --git a/infer/tests/codetoanalyze/cpp/checkers/Makefile b/infer/tests/codetoanalyze/cpp/checkers/Makefile new file mode 100644 index 000000000..b0b0c6b20 --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/checkers/Makefile @@ -0,0 +1,26 @@ +# Copyright (c) 2016 - 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. + +include ../../Makefile.clang + +OPTIONS = -x c++ -std=c++11 -isystem$(MODELS_DIR)/cpp/include -isystem$(CLANG_INCLUDES)/c++/v1/ -c + +ANALYZER = checkers +INFERPRINT_OPTIONS = --issues-txt + +FILES = \ + siof/pod_across_translation_units-1.cpp \ + siof/pod_across_translation_units-2.cpp \ + siof/pod_same_translation_unit.cpp \ + siof/siof_across_translation_units-1.cpp \ + +compile: + clang $(OPTIONS) $(FILES) + +analyze: + $(INFER_BIN) -a $(ANALYZER) --cxx --ml-buckets cpp --check-duplicate-symbols -- clang $(OPTIONS) $(FILES) >/dev/null 2>duplicates.txt + grep "DUPLICATE_SYMBOLS" duplicates.txt; test $$? -ne 0 diff --git a/infer/tests/codetoanalyze/cpp/checkers/issues.exp b/infer/tests/codetoanalyze/cpp/checkers/issues.exp new file mode 100644 index 000000000..64dfb0c0b --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/checkers/issues.exp @@ -0,0 +1,2 @@ +siof/pod_across_translation_units-1.cpp:12: 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 +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 diff --git a/infer/tests/codetoanalyze/cpp/checkers/siof/pod_across_translation_units-1.cpp b/infer/tests/codetoanalyze/cpp/checkers/siof/pod_across_translation_units-1.cpp new file mode 100644 index 000000000..dac91c1cb --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/checkers/siof/pod_across_translation_units-1.cpp @@ -0,0 +1,14 @@ +/* + * Copyright (c) 2016 - 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. + */ + +extern int foo(); + +static int x = foo(); // BAD: report SIOF here +static int x1 = x; // do not report here +static int x2 = x1; // do not report here diff --git a/infer/tests/codetoanalyze/cpp/checkers/siof/pod_across_translation_units-2.cpp b/infer/tests/codetoanalyze/cpp/checkers/siof/pod_across_translation_units-2.cpp new file mode 100644 index 000000000..a96c6bad3 --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/checkers/siof/pod_across_translation_units-2.cpp @@ -0,0 +1,12 @@ +/* + * Copyright (c) 2016 - 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. + */ + +static int y = 42; + +int foo() { return y; } diff --git a/infer/tests/codetoanalyze/cpp/checkers/siof/pod_same_translation_unit.cpp b/infer/tests/codetoanalyze/cpp/checkers/siof/pod_same_translation_unit.cpp new file mode 100644 index 000000000..84150275a --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/checkers/siof/pod_same_translation_unit.cpp @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2016 - 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. + */ + +#include + +int same_tu_foo(); +int same_tu_goo(); + +// BAD: report SIOF here +// This may not get the initialized value for y. +// Infer doesn't yet report here because it only looks across translation units. +int same_tu_x = same_tu_foo(); +int same_tu_y = same_tu_goo(); + +int same_tu_foo() { return same_tu_y; } + +int same_tu_goo() { return 42; } diff --git a/infer/tests/codetoanalyze/cpp/checkers/siof/siof_across_translation_units-1.cpp b/infer/tests/codetoanalyze/cpp/checkers/siof/siof_across_translation_units-1.cpp new file mode 100644 index 000000000..ec08d2f3f --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/checkers/siof/siof_across_translation_units-1.cpp @@ -0,0 +1,21 @@ +/* + * Copyright (c) 2016 - 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. + */ + +struct SomeObject { + void some_method(); +}; + +extern SomeObject global_object; + +struct SomeOtherObject { + SomeOtherObject() { global_object.some_method(); }; +}; + +// BAD: report SIOF here +SomeOtherObject another_global_object;