From dfb7c153039c3428cb5092e6c1b64e5a4c43781f Mon Sep 17 00:00:00 2001 From: Daiva Naudziuniene Date: Wed, 20 Jul 2016 09:01:15 -0700 Subject: [PATCH] Fixing Resource leak false positives assuming developers use raii. Reviewed By: jvillard Differential Revision: D3579581 fbshipit-source-id: 9c74d92 --- infer/src/IR/Sil.rei | 4 ++ infer/src/backend/interproc.ml | 9 +++ infer/src/backend/prop.ml | 33 +++++----- infer/src/backend/prop.mli | 2 + .../cpp/errors/resource_leaks/raii.cpp | 39 ++++++++++++ .../cpp/infer/ResourceLeakRaiiTest.java | 62 +++++++++++++++++++ 6 files changed, 135 insertions(+), 14 deletions(-) create mode 100644 infer/tests/codetoanalyze/cpp/errors/resource_leaks/raii.cpp create mode 100644 infer/tests/endtoend/cpp/infer/ResourceLeakRaiiTest.java diff --git a/infer/src/IR/Sil.rei b/infer/src/IR/Sil.rei index 7ff8a8df0..6c1542dd8 100644 --- a/infer/src/IR/Sil.rei +++ b/infer/src/IR/Sil.rei @@ -397,6 +397,10 @@ let binop_invert: Binop.t => exp => exp => exp; let mem_kind_compare: mem_kind => mem_kind => int; +let res_act_kind_compare: res_act_kind => res_act_kind => int; + +let resource_compare: resource => resource => int; + let attribute_compare: attribute => attribute => int; let attribute_equal: attribute => attribute => bool; diff --git a/infer/src/backend/interproc.ml b/infer/src/backend/interproc.ml index d12f8a62d..1fd37493f 100644 --- a/infer/src/backend/interproc.ml +++ b/infer/src/backend/interproc.ml @@ -767,6 +767,15 @@ let extract_specs tenv pdesc pathset : Prop.normal Specs.spec list = let collect_postconditions wl tenv pdesc : Paths.PathSet.t * Specs.Visitedset.t = let pname = Cfg.Procdesc.get_proc_name pdesc in let pathset = collect_analysis_result wl pdesc in + + (* Assuming C++ developers use RAII, remove resources from the constructor posts *) + let pathset = match pname with + | Procname.ObjC_Cpp _ -> + if (Procname.is_constructor pname) then + Paths.PathSet.map (Prop.remove_resource_attribute Sil.Racquire Sil.Rfile) pathset + else pathset + | _ -> pathset in + L.d_strln ("#### [FUNCTION " ^ Procname.to_string pname ^ "] Analysis result ####"); Propset.d Prop.prop_emp (Paths.PathSet.to_propset pathset); diff --git a/infer/src/backend/prop.ml b/infer/src/backend/prop.ml index 1920656f4..26bdaa034 100644 --- a/infer/src/backend/prop.ml +++ b/infer/src/backend/prop.ml @@ -1879,29 +1879,34 @@ let mark_vars_as_undefined prop vars_to_mark callee_pname ret_annots loc path_po | _ -> prop in IList.fold_left (fun prop id -> mark_var_as_undefined id prop) prop vars_to_mark -(** Remove an attribute from all the atoms in the heap *) -let remove_attribute att prop = +let remove_attribute_by_filter ~f prop = let atom_remove atom pi = match atom with - | Sil.Aneq (_, Sil.Attribute att_old) - | Sil.Aneq (Sil.Attribute att_old, _) -> - if Sil.attribute_equal att_old att then + | Sil.Aneq (e, Sil.Attribute att_old) + | Sil.Aneq (Sil.Attribute att_old, e) -> + if f att_old e then pi else atom:: pi | _ -> atom:: pi in let pi' = IList.fold_right atom_remove (get_pi prop) [] in replace_pi pi' prop +(** Remove an attribute from all the atoms in the heap *) +let remove_attribute att = + let f att_old _ = Sil.attribute_equal att_old att in + remove_attribute_by_filter ~f + +let remove_resource_attribute ra_kind ra_res = + let f att_old _ = match att_old with + | Sil.Aresource res_action -> + Sil.res_act_kind_compare res_action.Sil.ra_kind ra_kind == 0 + && Sil.resource_compare res_action.Sil.ra_res ra_res == 0 + | _ -> false in + remove_attribute_by_filter ~f + let remove_attribute_from_exp att prop exp = let nexp = exp_normalize_prop prop exp in - let atom_remove atom pi = match atom with - | Sil.Aneq (e, Sil.Attribute att_old) - | Sil.Aneq (Sil.Attribute att_old, e) -> - if Sil.attribute_equal att_old att && Sil.exp_equal nexp e then - pi - else atom:: pi - | _ -> atom:: pi in - let pi' = IList.fold_right atom_remove (get_pi prop) [] in - replace_pi pi' prop + let f att_old e = Sil.attribute_equal att_old att && Sil.exp_equal nexp e in + remove_attribute_by_filter ~f prop (* Replace an attribute OBJC_NULL($n1) with OBJC_NULL(var) when var = $n1, and also sets $n1 = 0 *) let replace_objc_null prop lhs_exp rhs_exp = diff --git a/infer/src/backend/prop.mli b/infer/src/backend/prop.mli index d5bf0410e..1358a58c4 100644 --- a/infer/src/backend/prop.mli +++ b/infer/src/backend/prop.mli @@ -326,6 +326,8 @@ val mark_vars_as_undefined : normal t -> Sil.exp list -> Procname.t -> Typ.item_ (** Remove an attribute from all the atoms in the heap *) val remove_attribute : Sil.attribute -> 'a t -> normal t +val remove_resource_attribute : Sil.res_act_kind -> Sil.resource -> 'a t -> normal t + (** [replace_objc_null lhs rhs]. If rhs has the objc_null attribute, replace the attribute and set the lhs = 0 *) val replace_objc_null : normal t -> exp -> exp -> normal t diff --git a/infer/tests/codetoanalyze/cpp/errors/resource_leaks/raii.cpp b/infer/tests/codetoanalyze/cpp/errors/resource_leaks/raii.cpp new file mode 100644 index 000000000..f56191cd8 --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/errors/resource_leaks/raii.cpp @@ -0,0 +1,39 @@ +/* + * 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 + +class OpenFile { + public: + OpenFile(const char* filename) { _filename = fopen(filename, "r"); } + + ~OpenFile() { fclose(_filename); } + + private: + FILE* _filename; +}; + +void resource_leak(const char* filename) { + char buffer[100]; + FILE* file = fopen(filename, "r"); + + if (file == NULL) + return; + else + fgets(buffer, 100, file); + + return; +} + +void no_resource_leak(const char* filename) { + OpenFile f(filename); + return; +} + +int main() {} diff --git a/infer/tests/endtoend/cpp/infer/ResourceLeakRaiiTest.java b/infer/tests/endtoend/cpp/infer/ResourceLeakRaiiTest.java new file mode 100644 index 000000000..1c954cc11 --- /dev/null +++ b/infer/tests/endtoend/cpp/infer/ResourceLeakRaiiTest.java @@ -0,0 +1,62 @@ +/* + * 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.cpp.infer; + +import static org.hamcrest.MatcherAssert.assertThat; +import static utils.matchers.ResultContainsExactly.containsExactly; + +import com.google.common.collect.ImmutableList; + +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; + +import java.io.IOException; + +import utils.DebuggableTemporaryFolder; +import utils.InferException; +import utils.InferResults; +import utils.InferRunner; + +public class ResourceLeakRaiiTest { + + public static final String FILE = + "infer/tests/codetoanalyze/cpp/errors/resource_leaks/raii.cpp"; + + private static ImmutableList inferCmd; + + public static final String RESOURCE_LEAK = "RESOURCE_LEAK"; + + @ClassRule + public static DebuggableTemporaryFolder folder = + new DebuggableTemporaryFolder(); + + @BeforeClass + public static void runInfer() throws InterruptedException, IOException { + inferCmd = InferRunner.createCPPInferCommandWithMLBuckets(folder, FILE, "cpp"); + } + + @Test + public void whenInferRunsOnObject_leakThenMLIsFound() + throws InterruptedException, IOException, InferException { + InferResults inferResults = InferRunner.runInferCPP(inferCmd); + String[] methods = { "resource_leak" }; + assertThat( + "Results should contain " + RESOURCE_LEAK, + inferResults, + containsExactly( + RESOURCE_LEAK, + FILE, + methods + ) + ); + } + +}