diff --git a/infer/src/checkers/callbackChecker.ml b/infer/src/checkers/callbackChecker.ml index 778456090..78e465154 100644 --- a/infer/src/checkers/callbackChecker.ml +++ b/infer/src/checkers/callbackChecker.ml @@ -16,22 +16,6 @@ module IdSet = Ident.IdentSet module FldSet = Ident.FieldSet open Utils -(** Automatically create a harness method to exercise code under test *) - -(** return the set of instance fields that are assigned to a null literal in [procdesc] *) -let get_fields_nullified procdesc = - (* walk through the instructions and look for instance fields that are assigned to null *) - let collect_nullified_flds (nullified_flds, this_ids) _ = function - | Sil.Set (Sil.Lfield (Sil.Var lhs, fld, _), typ, rhs, loc) - when Sil.exp_is_null_literal rhs && IdSet.mem lhs this_ids -> - (FldSet.add fld nullified_flds, this_ids) - | Sil.Letderef (id, rhs, _, _) when Sil.exp_is_this rhs -> - (nullified_flds, IdSet.add id this_ids) - | _ -> (nullified_flds, this_ids) in - let (nullified_flds, _) = - Cfg.Procdesc.fold_instrs collect_nullified_flds (FldSet.empty, IdSet.empty) procdesc in - nullified_flds - (** set of instance fields belonging to the current file that are assigned to null literals *) let fields_nullified = ref FldSet.empty @@ -112,7 +96,8 @@ let callback_checker_main all_procs get_procdesc idenv tenv proc_name proc_desc let _ = if AndroidFramework.is_destroy_method proc_name then (* compute the set of fields nullified by this procedure *) (* TODO (t4959422): get fields that are nullified in callees of the destroy method *) - fields_nullified := FldSet.union (get_fields_nullified proc_desc) !fields_nullified in + fields_nullified := + FldSet.union (PatternMatch.get_fields_nullified proc_desc) !fields_nullified in if done_checking (IList.length def_methods) then do_eradicate_check all_procs get_procdesc idenv tenv | _ -> () diff --git a/infer/src/checkers/callbackChecker.mli b/infer/src/checkers/callbackChecker.mli index 26c283f6b..12845b556 100644 --- a/infer/src/checkers/callbackChecker.mli +++ b/infer/src/checkers/callbackChecker.mli @@ -9,7 +9,4 @@ (** Make sure callbacks are always unregistered. drive the point home by reporting possible NPE's *) -(** return the set of instance fields that are assigned to a null literal in [procdesc] *) -val get_fields_nullified : Cfg.Procdesc.t -> Ident.FieldSet.t - val callback_checker_main : Callbacks.proc_callback_t diff --git a/infer/src/checkers/fragmentRetainsViewChecker.ml b/infer/src/checkers/fragmentRetainsViewChecker.ml new file mode 100644 index 000000000..642973142 --- /dev/null +++ b/infer/src/checkers/fragmentRetainsViewChecker.ml @@ -0,0 +1,62 @@ +(* + * Copyright (c) 2013 - 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. + *) + +(** Make sure callbacks are always unregistered. drive the point home by reporting possible NPE's *) + +module P = Printf +open Utils + +let report_error fld fld_typ pname pdesc = + let retained_view = "CHECKERS_FRAGMENT_RETAINS_VIEW" in + let fld_decl_class, fld_name = + Ident.java_fieldname_get_class fld, Ident.java_fieldname_get_field fld in + let description = + P.sprintf + "Fragment %s does not nullify View field %s (type %s) in onDestroyView. If the Fragment is placed on the back stack, a reference to the View may be retained." + fld_decl_class + fld_name + (Sil.typ_to_string (Sil.typ_strip_ptr fld_typ)) in + let loc = Cfg.Procdesc.get_loc pdesc in + let exn = Exceptions.Checkers (retained_view, Localise.verbatim_desc description) in + Reporting.log_error pname ~loc:(Some loc) exn + +let callback_fragment_retains_view all_procs get_procdesc idenv tenv pname pdesc = + (* TODO: complain if onDestroyView is not defined, yet the Fragment has View fields *) + (* TODO: handle fields nullified in callees in the same file *) + let is_on_destroy_view = Procname.java_get_method pname = "onDestroyView" in + (* this is needlessly complicated because field types are Tvars instead of Tstructs *) + let fld_typ_is_view = function + | Sil.Tptr (Sil.Tvar tname, _) -> + begin + match Sil.tenv_lookup tenv tname with + | Some typ -> AndroidFramework.is_view typ tenv + | None -> false + end + | _ -> false in + (* is [fldname] a View type declared by [class_typename]? *) + let is_declared_view_typ class_typename (fldname, fld_typ, _) = + let fld_classname = Typename.Java.from_string (Ident.java_fieldname_get_class fldname) in + Typename.equal fld_classname class_typename && fld_typ_is_view fld_typ in + if is_on_destroy_view then + begin + let class_typename = Typename.Java.from_string (Procname.java_get_class pname) in + match Sil.tenv_lookup tenv class_typename with + | Some (Sil.Tstruct { Sil.csu; struct_name = Some class_name; def_methods; instance_fields } + as typ) when AndroidFramework.is_fragment typ tenv -> + let declared_view_fields = + IList.filter (is_declared_view_typ class_typename) instance_fields in + let fields_nullified = PatternMatch.get_fields_nullified pdesc in + (* report if a field is declared by C, but not nulled out in C.onDestroyView *) + IList.iter + (fun (fname, typ, _) -> + if not (Ident.FieldSet.mem fname fields_nullified) then + report_error fname typ pname pdesc) + declared_view_fields + | _ -> () + end diff --git a/infer/src/checkers/patternMatch.ml b/infer/src/checkers/patternMatch.ml index 7e44e5ebc..eb1ae2f5e 100644 --- a/infer/src/checkers/patternMatch.ml +++ b/infer/src/checkers/patternMatch.ml @@ -328,3 +328,18 @@ let proc_iter_overridden_methods f tenv proc_name = | Some curr_type -> IList.iter (do_super_type tenv) (type_get_direct_supertypes curr_type) | None -> () + +(** return the set of instance fields that are assigned to a null literal in [procdesc] *) +let get_fields_nullified procdesc = + (* walk through the instructions and look for instance fields that are assigned to null *) + let collect_nullified_flds (nullified_flds, this_ids) _ = function + | Sil.Set (Sil.Lfield (Sil.Var lhs, fld, _), typ, rhs, loc) + when Sil.exp_is_null_literal rhs && Ident.IdentSet.mem lhs this_ids -> + (Ident.FieldSet.add fld nullified_flds, this_ids) + | Sil.Letderef (id, rhs, _, _) when Sil.exp_is_this rhs -> + (nullified_flds, Ident.IdentSet.add id this_ids) + | _ -> (nullified_flds, this_ids) in + let (nullified_flds, _) = + Cfg.Procdesc.fold_instrs + collect_nullified_flds (Ident.FieldSet.empty, Ident.IdentSet.empty) procdesc in + nullified_flds diff --git a/infer/src/checkers/patternMatch.mli b/infer/src/checkers/patternMatch.mli index 38b9340d5..24696c965 100644 --- a/infer/src/checkers/patternMatch.mli +++ b/infer/src/checkers/patternMatch.mli @@ -80,3 +80,6 @@ val type_is_nested_in_type : Sil.typ -> Mangled.t -> bool (** Is the type java.lang.Object *) val type_is_object : Sil.typ -> bool + +(** return the set of instance fields that are assigned to a null literal in [procdesc] *) +val get_fields_nullified : Cfg.Procdesc.t -> Ident.FieldSet.t diff --git a/infer/src/checkers/registerCheckers.ml b/infer/src/checkers/registerCheckers.ml index 15d665558..2ca424bb8 100644 --- a/infer/src/checkers/registerCheckers.ml +++ b/infer/src/checkers/registerCheckers.ml @@ -28,6 +28,7 @@ let active_procedure_checkers () = Checkers.callback_check_write_to_parcel, false; Checkers.callback_find_deserialization, false; Dataflow.callback_test_dataflow, false; + FragmentRetainsViewChecker.callback_fragment_retains_view, checkers_enabled; SqlChecker.callback_sql, false; Eradicate.callback_eradicate, !Config.eradicate; CodeQuery.code_query_callback, !CodeQuery.query <> None; diff --git a/infer/src/harness/androidFramework.ml b/infer/src/harness/androidFramework.ml index 0bf7c96d8..6ed8d92e6 100644 --- a/infer/src/harness/androidFramework.ml +++ b/infer/src/harness/androidFramework.ml @@ -289,6 +289,10 @@ let is_activity typ tenv = let is_view typ tenv = is_subtype_package_class typ "android.view" "View" tenv +let is_fragment typ tenv = + is_subtype_package_class typ "android.app" "Fragment" tenv || + is_subtype_package_class typ "android.support.v4.app" "Fragment" tenv + (** return true if [class_name] is a known callback class name *) let is_callback_class_name class_name = Mangled.MangledSet.mem class_name android_callbacks diff --git a/infer/src/harness/androidFramework.mli b/infer/src/harness/androidFramework.mli index 042b095c9..1ed77a9cc 100644 --- a/infer/src/harness/androidFramework.mli +++ b/infer/src/harness/androidFramework.mli @@ -32,6 +32,8 @@ val is_activity : Sil.typ -> Sil.tenv -> bool (** return true if [typ] <: android.view.View *) val is_view : Sil.typ -> Sil.tenv -> bool +val is_fragment : Sil.typ -> Sil.tenv -> bool + (** return true if [procname] is a special lifecycle cleanup method *) val is_destroy_method : Procname.t -> bool diff --git a/infer/tests/codetoanalyze/java/checkers/FragmentDoesNotRetainViewExample.java b/infer/tests/codetoanalyze/java/checkers/FragmentDoesNotRetainViewExample.java new file mode 100644 index 000000000..cfc6cc281 --- /dev/null +++ b/infer/tests/codetoanalyze/java/checkers/FragmentDoesNotRetainViewExample.java @@ -0,0 +1,56 @@ +/* + * 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. + */ + +package codetoanalyze.java.checkers; + +import android.content.Context; +import android.os.Bundle; +import android.support.v4.app.Fragment; +import android.view.LayoutInflater; +import android.view.View; +import android.view.ViewGroup; +import android.widget.ListView; + +public class FragmentDoesNotRetainViewExample extends Fragment { + + class CustomView extends ListView { + + public CustomView(Context c) { + super(c); + } + + } + + View mView1; + View mView2; + ViewGroup mViewSubclass; + CustomView mCustomView; + + boolean b; + + @Override + public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle bundle) { + mView1 = inflater.inflate(-1, container, false); + mView2 = inflater.inflate(-1, container, false); + mViewSubclass = (ViewGroup) inflater.inflate(-1, container, false); + mCustomView = (CustomView) inflater.inflate(-1, container, false); + return container; + } + + @Override + public void onDestroyView() { + mView1 = null; + if (b) { + mView2 = null; // conditional nulling is still ok + } + mCustomView = null; + mViewSubclass = null; + } + +} diff --git a/infer/tests/codetoanalyze/java/checkers/FragmentRetainsViewExample.java b/infer/tests/codetoanalyze/java/checkers/FragmentRetainsViewExample.java new file mode 100644 index 000000000..539c79638 --- /dev/null +++ b/infer/tests/codetoanalyze/java/checkers/FragmentRetainsViewExample.java @@ -0,0 +1,47 @@ +/* + * 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. + */ + +package codetoanalyze.java.checkers; + +import android.content.Context; +import android.os.Bundle; +import android.support.v4.app.Fragment; +import android.view.LayoutInflater; +import android.view.View; +import android.view.ViewGroup; +import android.widget.ListView; + +public class FragmentRetainsViewExample extends Fragment { + + class CustomView extends ListView { + + public CustomView(Context c) { + super(c); + } + + } + + View mView; + ViewGroup mViewSubclass; + CustomView mCustomView; + + @Override + public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle bundle) { + mView = inflater.inflate(-1, container, false); + mViewSubclass = (ViewGroup) inflater.inflate(-1, container, false); + mCustomView = (CustomView) inflater.inflate(-1, container, false); + return container; + } + + @Override + public void onDestroyView() { + // not nulling out anything + } + +} diff --git a/infer/tests/endtoend/java/checkers/FragmentDoesNotRetainViewTest.java b/infer/tests/endtoend/java/checkers/FragmentDoesNotRetainViewTest.java new file mode 100644 index 000000000..3ef650924 --- /dev/null +++ b/infer/tests/endtoend/java/checkers/FragmentDoesNotRetainViewTest.java @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2015 - 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. + */ + +package endtoend.java.checkers; + +import static org.hamcrest.MatcherAssert.assertThat; +import static utils.matchers.ResultContainsNoErrorInMethod.doesNotContain; + +import org.junit.BeforeClass; +import org.junit.Test; + +import java.io.IOException; + +import utils.InferException; +import utils.InferResults; + +public class FragmentDoesNotRetainViewTest { + + public static final String SOURCE_FILE = + "infer/tests/codetoanalyze/java/checkers/FragmentDoesNotRetainViewExample.java"; + + public static final String FRAGMENT_RETAINS_VIEW = + "CHECKERS_FRAGMENT_RETAINS_VIEW"; + + private static InferResults inferResults; + + @BeforeClass + public static void loadResults() throws InterruptedException, IOException { + inferResults = + InferResults.loadCheckersResults(FragmentDoesNotRetainViewTest.class, SOURCE_FILE); + } + + @Test + public void matchNumberOfErrors() + throws IOException, InterruptedException, InferException { + assertThat( + "Results should contain 0 retained View errors", + inferResults, + doesNotContain( + FRAGMENT_RETAINS_VIEW, + SOURCE_FILE, + "onDestroyView" + ) + ); + } + +} diff --git a/infer/tests/endtoend/java/checkers/FragmentRetainsViewTest.java b/infer/tests/endtoend/java/checkers/FragmentRetainsViewTest.java new file mode 100644 index 000000000..943f396bc --- /dev/null +++ b/infer/tests/endtoend/java/checkers/FragmentRetainsViewTest.java @@ -0,0 +1,54 @@ +/* + * Copyright (c) 2015 - 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. + */ + +package endtoend.java.checkers; + +import static org.hamcrest.MatcherAssert.assertThat; +import static utils.matchers.ResultContainsNumberOfErrorsInMethod.containsNumberOfErrors; + +import org.junit.BeforeClass; +import org.junit.Test; + +import java.io.IOException; + +import utils.InferException; +import utils.InferResults; + +public class FragmentRetainsViewTest { + + public static final String SOURCE_FILE = + "infer/tests/codetoanalyze/java/checkers/FragmentRetainsViewExample.java"; + + public static final String FRAGMENT_RETAINS_VIEW = + "CHECKERS_FRAGMENT_RETAINS_VIEW"; + + private static InferResults inferResults; + + @BeforeClass + public static void loadResults() throws InterruptedException, IOException { + inferResults = + InferResults.loadCheckersResults(FragmentRetainsViewTest.class, SOURCE_FILE); + } + + @Test + public void matchNumberOfErrors() + throws IOException, InterruptedException, InferException { + assertThat( + "Results should contain 3 retained View errors", + inferResults, + containsNumberOfErrors( + FRAGMENT_RETAINS_VIEW, + SOURCE_FILE, + "onDestroyView", + 3 + ) + ); + } + +}