From dd2c56da063e938bfba712c84d0aa104f4beadce Mon Sep 17 00:00:00 2001
From: Jules Villard <jul@fb.com>
Date: Tue, 11 Apr 2017 12:16:51 -0700
Subject: [PATCH] be more careful about handling invalid source files

Summary:
Limit the use of `SourceFile.invalid` (renamed from `SourceFile.empty`) as much
as possible. In particular, do not generate bogus procnames for external global
variables: their translation unit was set to the invalid source file, now we
distinguish between extern/non-extern global variables more explicitly.

`SourceFile.invalid` is still used in too many places to actually remove it, often as a dummy initial value that never gets used, but sometimes as an actual value... Worse, we cannot fail on all operations on `SourceFile.Invalid` yet: the `SourceFile.to_string` method is used in too many places where it could get `SourceFile.Invalid` as argument. It's easy to see where it's used by making it raise in the code, then running the tests. This results in spaghetti backtraces that are hard to trace back to a root cause.

Reviewed By: akotulski, jeremydubreil

Differential Revision: D4860019

fbshipit-source-id: 45be040
---
 infer/src/IR/Cg.re                  | 11 ++--------
 infer/src/IR/Cg.rei                 |  2 +-
 infer/src/IR/Location.re            |  2 +-
 infer/src/IR/ProcAttributes.re      |  2 +-
 infer/src/IR/Pvar.re                | 34 +++++++++++++++++++++--------
 infer/src/IR/Pvar.rei               | 18 ++++++++++++---
 infer/src/IR/Sil.re                 |  2 +-
 infer/src/backend/exe_env.ml        |  2 +-
 infer/src/backend/ondemand.ml       |  2 +-
 infer/src/base/DB.ml                |  5 +++--
 infer/src/base/SourceFile.ml        | 10 ++++++++-
 infer/src/base/SourceFile.mli       |  7 ++++--
 infer/src/checkers/Siof.ml          | 13 ++++++-----
 infer/src/checkers/SiofTrace.ml     |  6 ++---
 infer/src/checkers/Stacktrace.ml    |  6 +++--
 infer/src/clang/cFrontend.ml        |  2 +-
 infer/src/clang/cGeneral_utils.ml   |  4 ++--
 infer/src/java/jFrontend.ml         |  4 ++--
 infer/src/java/jTrans.ml            |  4 ++--
 infer/src/quandary/TaintAnalysis.ml |  2 +-
 20 files changed, 86 insertions(+), 52 deletions(-)

diff --git a/infer/src/IR/Cg.re b/infer/src/IR/Cg.re
index 8d8a61939..c4b50437d 100644
--- a/infer/src/IR/Cg.re
+++ b/infer/src/IR/Cg.re
@@ -46,14 +46,7 @@ type t = {
   node_map: Typ.Procname.Hash.t node_info /** map from node to node_info */
 };
 
-let create source_opt => {
-  let source =
-    switch source_opt {
-    | None => SourceFile.empty
-    | Some source => source
-    };
-  {source, node_map: Typ.Procname.Hash.create 3}
-};
+let create source => {source, node_map: Typ.Procname.Hash.create 3};
 
 let add_node g n ::defined =>
   try {
@@ -360,7 +353,7 @@ let load_from_file (filename: DB.filename) :option t =>
   switch (Serialization.read_from_file callgraph_serializer filename) {
   | None => None
   | Some (source, (nodes, edges)) =>
-    let g = create (Some source);
+    let g = create source;
     List.iter
       f::(
         fun (node, defined) =>
diff --git a/infer/src/IR/Cg.rei b/infer/src/IR/Cg.rei
index f2514d953..3615d888a 100644
--- a/infer/src/IR/Cg.rei
+++ b/infer/src/IR/Cg.rei
@@ -40,7 +40,7 @@ let calls_recursively: t => Typ.Procname.t => Typ.Procname.t => bool;
 
 
 /** Create an empty call graph */
-let create: option SourceFile.t => t;
+let create: SourceFile.t => t;
 
 
 /** [extend cg1 gc2] extends [cg1] in place with nodes and edges from [gc2];
diff --git a/infer/src/IR/Location.re b/infer/src/IR/Location.re
index f884b76c7..498a387c2 100644
--- a/infer/src/IR/Location.re
+++ b/infer/src/IR/Location.re
@@ -29,7 +29,7 @@ let d (loc: t) => L.add_print_action (L.PTloc, Obj.repr loc);
 
 
 /** Dummy location */
-let dummy = {line: (-1), col: (-1), file: SourceFile.empty};
+let dummy = {line: (-1), col: (-1), file: SourceFile.invalid};
 
 
 /** Pretty print a location */
diff --git a/infer/src/IR/ProcAttributes.re b/infer/src/IR/ProcAttributes.re
index 66cf71272..822341875 100644
--- a/infer/src/IR/ProcAttributes.re
+++ b/infer/src/IR/ProcAttributes.re
@@ -100,5 +100,5 @@ let default proc_name language => {
   proc_flags: proc_flags_empty (),
   proc_name,
   ret_type: Typ.Tvoid,
-  source_file_captured: SourceFile.empty
+  source_file_captured: SourceFile.invalid
 };
diff --git a/infer/src/IR/Pvar.re b/infer/src/IR/Pvar.re
index b53ad1641..662f0525b 100644
--- a/infer/src/IR/Pvar.re
+++ b/infer/src/IR/Pvar.re
@@ -15,6 +15,11 @@ module L = Logging;
 
 module F = Format;
 
+type translation_unit =
+  | TUFile SourceFile.t
+  | TUExtern
+[@@deriving compare];
+
 
 /** Kind of global variables */
 type pvar_kind =
@@ -23,7 +28,7 @@ type pvar_kind =
   | Abduced_retvar Typ.Procname.t Location.t /** synthetic variable to represent return value */
   | Abduced_ref_param Typ.Procname.t t Location.t
   /** synthetic variable to represent param passed by reference */
-  | Global_var (SourceFile.t, bool, bool, bool)
+  | Global_var (translation_unit, bool, bool, bool)
   /** global variable: translation unit + is it compile constant? + is it POD? + is it a static
       local? */
   | Seed_var /** variable used to store the initial value of formal parameters */
@@ -36,6 +41,11 @@ let compare_alpha pv1 pv2 =>
 
 let equal = [%compare.equal : t];
 
+let pp_translation_unit fmt =>
+  fun
+  | TUFile fname => SourceFile.pp fmt fname
+  | TUExtern => Format.fprintf fmt "EXTERN";
+
 let rec _pp f pv => {
   let name = pv.pv_name;
   switch pv.pv_kind {
@@ -63,12 +73,12 @@ let rec _pp f pv => {
     } else {
       F.fprintf f "%a$%a%a|abducedRefParam" Typ.Procname.pp n Location.pp l Mangled.pp name
     }
-  | Global_var (fname, is_const, is_pod, _) =>
+  | Global_var (translation_unit, is_const, is_pod, _) =>
     F.fprintf
       f
       "#GB<%a%s%s>$%a"
-      SourceFile.pp
-      fname
+      pp_translation_unit
+      translation_unit
       (if is_const {"|const"} else {""})
       (
         if (not is_pod) {
@@ -301,10 +311,16 @@ let mk_callee (name: Mangled.t) (proc_name: Typ.Procname.t) :t => {
 
 
 /** create a global variable with the given name */
-let mk_global ::is_constexpr=false ::is_pod=true ::is_static_local=false (name: Mangled.t) fname :t => {
+let mk_global
+    ::is_constexpr=false
+    ::is_pod=true
+    ::is_static_local=false
+    (name: Mangled.t)
+    translation_unit
+    :t => {
   pv_hash: name_hash name,
   pv_name: name,
-  pv_kind: Global_var (fname, is_constexpr, is_pod, is_static_local)
+  pv_kind: Global_var (translation_unit, is_constexpr, is_pod, is_static_local)
 };
 
 
@@ -327,10 +343,10 @@ let mk_abduced_ref_param (proc_name: Typ.Procname.t) (pv: t) (loc: Location.t) :
   {pv_hash: name_hash name, pv_name: name, pv_kind: Abduced_ref_param proc_name pv loc}
 };
 
-let get_source_file pvar =>
+let get_translation_unit pvar =>
   switch pvar.pv_kind {
-  | Global_var (f, _, _, _) => Some f
-  | _ => None
+  | Global_var (tu, _, _, _) => tu
+  | _ => invalid_argf "Expected a global variable"
   };
 
 let is_compile_constant pvar =>
diff --git a/infer/src/IR/Pvar.rei b/infer/src/IR/Pvar.rei
index 72224e4a8..bc630e4fd 100644
--- a/infer/src/IR/Pvar.rei
+++ b/infer/src/IR/Pvar.rei
@@ -13,6 +13,11 @@ open! IStd;
 /** Program variables. */
 module F = Format;
 
+type translation_unit =
+  | TUFile SourceFile.t
+  | TUExtern
+[@@deriving compare];
+
 
 /** Type for program variables. There are 4 kinds of variables:
         1) local variables, used for local variables and formal parameters
@@ -106,7 +111,12 @@ let mk_callee: Mangled.t => Typ.Procname.t => t;
 
 /** create a global variable with the given name */
 let mk_global:
-  is_constexpr::bool? => is_pod::bool? => is_static_local::bool? => Mangled.t => SourceFile.t => t;
+  is_constexpr::bool? =>
+  is_pod::bool? =>
+  is_static_local::bool? =>
+  Mangled.t =>
+  translation_unit =>
+  t;
 
 
 /** create a fresh temporary variable local to procedure [pname]. for use in the frontends only! */
@@ -124,6 +134,8 @@ let pp_list: Pp.env => F.formatter => list t => unit;
 /** Pretty print a pvar which denotes a value, not an address */
 let pp_value: Pp.env => F.formatter => t => unit;
 
+let pp_translation_unit: F.formatter => translation_unit => unit;
+
 
 /** Turn an ordinary program variable into a callee program variable */
 let to_callee: Typ.Procname.t => t => t;
@@ -137,8 +149,8 @@ let to_seed: t => t;
 let to_string: t => string;
 
 
-/** Get the source file corresponding to a global, if known. Returns [None] if not a global. */
-let get_source_file: t => option SourceFile.t;
+/** Get the translation unit corresponding to a global. Raises Invalid_arg if not a global. */
+let get_translation_unit: t => translation_unit;
 
 
 /** Is the variable's value a compile-time constant? Always (potentially incorrectly) returns
diff --git a/infer/src/IR/Sil.re b/infer/src/IR/Sil.re
index af540f107..de7779204 100644
--- a/infer/src/IR/Sil.re
+++ b/infer/src/IR/Sil.re
@@ -2453,4 +2453,4 @@ let hpara_dll_instantiate (para: hpara_dll) cell blink flink elist => {
   (ids_evars, List.map f::(hpred_sub subst) para.body_dll)
 };
 
-let custom_error = Pvar.mk_global (Mangled.from_string "INFER_CUSTOM_ERROR") SourceFile.empty;
+let custom_error = Pvar.mk_global (Mangled.from_string "INFER_CUSTOM_ERROR") Pvar.TUExtern;
diff --git a/infer/src/backend/exe_env.ml b/infer/src/backend/exe_env.ml
index e4ff79090..ffc74a203 100644
--- a/infer/src/backend/exe_env.ml
+++ b/infer/src/backend/exe_env.ml
@@ -75,7 +75,7 @@ type initial = t
 
 (** create a new execution environment *)
 let create () =
-  { cg = Cg.create None;
+  { cg = Cg.create SourceFile.invalid;
     proc_map = Typ.Procname.Hash.create 17;
     file_map = FilenameHash.create 1;
     source_files = SourceFile.Set.empty;
diff --git a/infer/src/backend/ondemand.ml b/infer/src/backend/ondemand.ml
index e494fcb0a..2d93ad6d0 100644
--- a/infer/src/backend/ondemand.ml
+++ b/infer/src/backend/ondemand.ml
@@ -139,7 +139,7 @@ let run_proc_analysis ~propagate_exceptions analyze_proc curr_pdesc callee_pdesc
               failwith ("ERROR: "^(Typ.Procname.to_string callee_pname)
                         ^" not equal to "^(Typ.Procname.to_string attribute_pname));
             attributes.loc.file)
-        ~default:SourceFile.empty
+        ~default:SourceFile.invalid
         attributes_opt in
     let callee_pdesc_option =
       if Config.dynamic_dispatch = `Lazy
diff --git a/infer/src/base/DB.ml b/infer/src/base/DB.ml
index 2433bddf6..76ad62b68 100644
--- a/infer/src/base/DB.ml
+++ b/infer/src/base/DB.ml
@@ -222,12 +222,13 @@ module Results_dir = struct
 
   (** initialize the results directory *)
   let init source =
+    if SourceFile.is_invalid source then
+      invalid_arg "Invalid source file passed";
     Utils.create_dir Config.results_dir;
     Utils.create_dir specs_dir;
     Utils.create_dir (path_to_filename Abs_root [Config.attributes_dir_name]);
     Utils.create_dir (path_to_filename Abs_root [Config.captured_dir_name]);
-    if not (SourceFile.equal source SourceFile.empty) then
-      Utils.create_dir (path_to_filename (Abs_source_dir source) [])
+    Utils.create_dir (path_to_filename (Abs_source_dir source) [])
 
   let clean_specs_dir () =
     Utils.create_dir specs_dir; (* create dir just in case it doesn't exist to avoid errors *)
diff --git a/infer/src/base/SourceFile.ml b/infer/src/base/SourceFile.ml
index 62f55d9ca..2bedb4f47 100644
--- a/infer/src/base/SourceFile.ml
+++ b/infer/src/base/SourceFile.ml
@@ -15,6 +15,7 @@ let count_newlines (path: string): int =
   In_channel.with_file path ~f
 
 type t =
+  | Invalid
   | Absolute of string
   | RelativeProjectRoot of string (* relative to project root *)
   | RelativeInferModel of string (* relative to infer models *)
@@ -53,6 +54,7 @@ let from_abs_path fname =
 
 let to_string fname =
   match fname with
+  | Invalid -> "DUMMY"
   | RelativeInferModel path -> "INFER_MODEL/" ^ path
   | RelativeProjectRoot path
   | Absolute path -> path
@@ -63,6 +65,7 @@ let pp fmt fname =
 (* Checking if the path exists may be needed only in some cases, hence the flag check_exists *)
 let to_abs_path fname =
   match fname with
+  | Invalid -> invalid_arg "cannot be called with Invalid source file"
   | RelativeProjectRoot path -> Filename.concat Config.project_root path
   | RelativeInferModel path -> Filename.concat Config.models_src_dir path
   | Absolute path -> path
@@ -75,9 +78,13 @@ let to_rel_path fname =
   match fname with
   | RelativeProjectRoot path -> path
   | _ -> to_abs_path fname
-let empty = Absolute ""
+
+let invalid = Invalid
+
+let is_invalid = equal Invalid
 
 let is_infer_model source_file = match source_file with
+  | Invalid -> invalid_arg "cannot be called with Invalid source file"
   | RelativeProjectRoot _ | Absolute _ -> false
   | RelativeInferModel _ -> true
 
@@ -89,6 +96,7 @@ let is_cpp_model file =
   | _ -> false
 
 let is_under_project_root = function
+  | Invalid -> invalid_arg "cannot be called with Invalid source file"
   | RelativeProjectRoot _ -> true
   | Absolute _ | RelativeInferModel _ -> false
 
diff --git a/infer/src/base/SourceFile.mli b/infer/src/base/SourceFile.mli
index 3a4f7b65b..051876137 100644
--- a/infer/src/base/SourceFile.mli
+++ b/infer/src/base/SourceFile.mli
@@ -22,13 +22,16 @@ module UNSAFE : sig
   val from_string : string -> t
 end
 
+(** Is the source file the invalid source file? *)
+val is_invalid : t -> bool
+
 (** Set of files read from --changed-files-index file, None if option not specified
     NOTE: it may include extra source_files if --changed-files-index contains paths to
           header files *)
 val changed_files_set : Set.t option
 
-(** empty source file *)
-val empty : t
+(** Invalid source file *)
+val invalid : t
 
 (** equality of source files *)
 val equal : t -> t -> bool
diff --git a/infer/src/checkers/Siof.ml b/infer/src/checkers/Siof.ml
index 9dbe5cd67..67d8b1072 100644
--- a/infer/src/checkers/Siof.ml
+++ b/infer/src/checkers/Siof.ml
@@ -176,12 +176,13 @@ module Interprocedural = AbstractInterpreter.Interprocedural (Summary)
 
 
 let is_foreign tu_opt (v, _) =
-  let is_orig_file f = match tu_opt with
-    | Some orig_file ->
-        let orig_path = SourceFile.to_abs_path orig_file in
-        String.equal orig_path (SourceFile.to_abs_path f)
-    | None -> assert false in
-  Option.value_map ~f:(fun f -> not (is_orig_file f)) ~default:false (Pvar.get_source_file v)
+  match Pvar.get_translation_unit v, tu_opt with
+  | TUFile v_tu, Some current_tu ->
+      not (SourceFile.equal current_tu v_tu)
+  | TUExtern, Some _ ->
+      true
+  | _, None ->
+      invalid_arg "cannot be called with translation unit set to None"
 
 let report_siof summary trace pdesc gname loc =
   let tu_opt =
diff --git a/infer/src/checkers/SiofTrace.ml b/infer/src/checkers/SiofTrace.ml
index 5d5d3d5b4..bf6f06ba0 100644
--- a/infer/src/checkers/SiofTrace.ml
+++ b/infer/src/checkers/SiofTrace.ml
@@ -19,10 +19,8 @@ module GlobalsAccesses = PrettyPrintable.MakePPSet (struct
       (* compare by loc first to present reports in the right order *)
       [%compare : (Location.t * Pvar.t)] (l1, v1) (l2, v2)
     let pp fmt (v, _) =
-      F.fprintf fmt "%a" Mangled.pp (Pvar.get_name v);
-      match Pvar.get_source_file v with
-      | Some fname -> F.fprintf fmt "%a" SourceFile.pp fname
-      | None -> ()
+      F.fprintf fmt "%a|%a" Mangled.pp (Pvar.get_name v)
+        Pvar.pp_translation_unit (Pvar.get_translation_unit v)
   end)
 
 module TraceElem = struct
diff --git a/infer/src/checkers/Stacktrace.ml b/infer/src/checkers/Stacktrace.ml
index 1f70880ea..1cb5d24ad 100644
--- a/infer/src/checkers/Stacktrace.ml
+++ b/infer/src/checkers/Stacktrace.ml
@@ -43,8 +43,10 @@ let make_frame class_str method_str file_str line_num =
   { class_str; method_str; file_str; line_num; }
 
 let frame_matches_location frame_obj loc =
-  let lfname = SourceFile.to_string loc.Location.file in
-  let matches_file = String.is_suffix ~suffix:frame_obj.file_str lfname in
+  let lfname = if SourceFile.is_invalid loc.Location.file then None
+    else Some (SourceFile.to_string loc.Location.file) in
+  let matches_file = Option.value_map lfname ~default:false
+      ~f:(String.is_suffix ~suffix:frame_obj.file_str) in
   let matches_line = match frame_obj.line_num with
     | None -> false
     | Some line -> Int.equal line loc.Location.line in
diff --git a/infer/src/clang/cFrontend.ml b/infer/src/clang/cFrontend.ml
index bafe424e7..9267513a3 100644
--- a/infer/src/clang/cFrontend.ml
+++ b/infer/src/clang/cFrontend.ml
@@ -23,7 +23,7 @@ let compute_icfg trans_unit_ctx tenv ast =
   | Clang_ast_t.TranslationUnitDecl(_, decl_list, _, _) ->
       CFrontend_config.global_translation_unit_decls := decl_list;
       Logging.out_debug "@\n Start creating icfg@\n";
-      let cg = Cg.create (Some trans_unit_ctx.CFrontend_config.source_file) in
+      let cg = Cg.create trans_unit_ctx.CFrontend_config.source_file in
       let cfg = Cfg.create_cfg () in
       List.iter
         ~f:(CFrontend_declImpl.translate_one_declaration trans_unit_ctx tenv cg cfg `DeclTraversal)
diff --git a/infer/src/clang/cGeneral_utils.ml b/infer/src/clang/cGeneral_utils.ml
index 7ed4450a3..8bd310529 100644
--- a/infer/src/clang/cGeneral_utils.ml
+++ b/infer/src/clang/cGeneral_utils.ml
@@ -136,9 +136,9 @@ let mk_sil_global_var {CFrontend_config.source_file} ?(mk_name=fun _ x -> x)
     | Some "extern", None ->
         (* some compilers simply disregard "extern" when the global is given some initialisation
            code, which is why we make sure that [vdi_init_expr] is None here... *)
-        SourceFile.empty
+        Pvar.TUExtern
     | _ ->
-        source_file in
+        Pvar.TUFile source_file in
   let is_constexpr = var_decl_info.Clang_ast_t.vdi_is_const_expr in
   let is_pod =
     CAst_utils.get_desugared_type qt.Clang_ast_t.qt_type_ptr
diff --git a/infer/src/java/jFrontend.ml b/infer/src/java/jFrontend.ml
index 03afbec3c..9f282fbf5 100644
--- a/infer/src/java/jFrontend.ml
+++ b/infer/src/java/jFrontend.ml
@@ -190,7 +190,7 @@ let compute_source_icfg
     linereader classes program tenv
     source_basename package_opt source_file =
   let icfg =
-    { JContext.cg = Cg.create (Some source_file);
+    { JContext.cg = Cg.create source_file;
       JContext.cfg = Cfg.create_cfg ();
       JContext.tenv = tenv } in
   let select test procedure cn node =
@@ -210,7 +210,7 @@ let compute_source_icfg
 
 let compute_class_icfg source_file linereader program tenv node =
   let icfg =
-    { JContext.cg = Cg.create (Some source_file);
+    { JContext.cg = Cg.create source_file;
       JContext.cfg = Cfg.create_cfg ();
       JContext.tenv = tenv } in
   begin
diff --git a/infer/src/java/jTrans.ml b/infer/src/java/jTrans.ml
index 8be557f34..215f6748e 100644
--- a/infer/src/java/jTrans.ml
+++ b/infer/src/java/jTrans.ml
@@ -490,7 +490,7 @@ let rec expression (context : JContext.t) pc expr =
   | JBir.StaticField (cn, fs) ->
       let class_exp =
         let classname = Mangled.from_string (JBasics.cn_name cn) in
-        let var_name = Pvar.mk_global classname file in
+        let var_name = Pvar.mk_global classname (Pvar.TUFile file) in
         Exp.Lvar var_name in
       let (instrs, sil_expr) = [], class_exp in
       let field_name = get_field_name program true tenv cn fs in
@@ -768,7 +768,7 @@ let rec instruction (context : JContext.t) pc instr : translation =
     | JBir.AffectStaticField (cn, fs, e_rhs) ->
         let class_exp =
           let classname = Mangled.from_string (JBasics.cn_name cn) in
-          let var_name = Pvar.mk_global classname file in
+          let var_name = Pvar.mk_global classname (Pvar.TUFile file) in
           Exp.Lvar var_name in
         let (stml1, sil_expr_lhs) = [], class_exp in
         let (stml2, sil_expr_rhs, _) = expression context pc e_rhs in
diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml
index 547dfbc9f..768512830 100644
--- a/infer/src/quandary/TaintAnalysis.ml
+++ b/infer/src/quandary/TaintAnalysis.ml
@@ -611,7 +611,7 @@ module Make (TaintSpecification : TaintSpec.S) = struct
       then
         begin
           Preanal.do_liveness proc_data.pdesc proc_data.tenv;
-          Preanal.do_dynamic_dispatch proc_data.pdesc (Cg.create None) proc_data.tenv;
+          Preanal.do_dynamic_dispatch proc_data.pdesc (Cg.create SourceFile.invalid) proc_data.tenv;
         end;
       let initial = make_initial proc_data.pdesc in
       match Analyzer.compute_post proc_data ~initial with