[infer][java] prevent the closeable as resource approach to report resource leak when close() throws an exception

Summary:
@public
Previously, if the close() method was throwing an exception, then code overriding the file attribute with a mem attribute would be skipped, resulting in reporting a wrong resource leak. This diff fixes this.

Test Plan: Added new end-to-end tests which would previously have been failing

infer/src/backend/symExec.ml

@@ -1160,10 +1160,11 @@ and add_constraints_on_actuals_by_ref prop actuals_by_ref callee_pname =
 (** execute a call for an unknown or scan function *)
 and call_unknown_or_scan is_scan cfg pdesc tenv pre path
     ret_ids ret_type_option actual_pars callee_pname loc =
-  let remove_resource_att prop =
+  let remove_file_attribute prop =
     let do_exp p (e, t) =
       let do_attribute q = function
-        | Sil.Aresource _ as res ->
+        | Sil.Aresource res_action as res
+             when res_action.Sil.ra_res = Sil.Rfile ->
             Prop.remove_attribute res q
         | _ -> q in
       list_fold_left do_attribute p (Prop.get_exp_attributes p e) in
@@ -1175,7 +1176,7 @@ and call_unknown_or_scan is_scan cfg pdesc tenv pre path
         | _ -> false)
       actual_pars in
   (* in Java, assume that skip functions close resources passed as params *)
-  let pre' = if !Sil.curr_language = Sil.Java then remove_resource_att pre else pre in
+  let pre' = if !Sil.curr_language = Sil.Java then remove_file_attribute pre else pre in
   let pre'' = add_constraints_on_retval pdesc pre' ret_ids ret_type_option callee_pname in
   let pre''' = add_constraints_on_actuals_by_ref pre'' actuals_by_ref callee_pname in
   if is_scan (* if scan function, don't mark anything with undef attributes *)

infer/src/java/jTrans.ml

@@ -651,27 +651,30 @@ let method_invocation context loc pc var_opt cn ms sil_obj_opt expr_list invoke_
         let sil_var = JContext.set_pvar context var return_type in
         (call_ret_instrs sil_var) in
   let instrs =
-      match call_args with
+    match call_args with
-      (* modeling a class bypasses the treatment of Closeable *)
+    (* modeling a class bypasses the treatment of Closeable *)
-      | _ when Config.analyze_models || JClasspath.is_model callee_procname -> call_instrs
+    | _ when Config.analyze_models || JClasspath.is_model callee_procname -> call_instrs
-
+
-      (* add a file attribute when calling the constructor of a subtype of Closeable *)
+    (* add a file attribute when calling the constructor of a subtype of Closeable *)
-      | (var, typ) as exp :: _
+    | (var, typ) as exp :: _
-      when Procname.is_constructor callee_procname && JTransType.is_closeable program tenv typ ->
+         when Procname.is_constructor callee_procname && JTransType.is_closeable program tenv typ ->
-          let set_file_attr =
+       let set_file_attr =
-            let set_builtin = Sil.Const (Sil.Cfun SymExec.ModelBuiltins.__set_file_attribute) in
+         let set_builtin = Sil.Const (Sil.Cfun SymExec.ModelBuiltins.__set_file_attribute) in
-            Sil.Call ([], set_builtin, [exp], loc, Sil.cf_default) in
+         Sil.Call ([], set_builtin, [exp], loc, Sil.cf_default) in
-          call_instrs @ [set_file_attr]
+       (* Exceptions thrown in the constructor should prevent adding the resource attribute *)
-
+       call_instrs @ [set_file_attr]
-      (* remove file attribute when calling the close method of a subtype of Closeable *)
+
-      | (var, typ) as exp :: []
+    (* remove file attribute when calling the close method of a subtype of Closeable *)
-      when Procname.java_is_close callee_procname && JTransType.is_closeable program tenv typ ->
+    | (var, typ) as exp :: []
-          let set_mem_attr =
+         when Procname.java_is_close callee_procname && JTransType.is_closeable program tenv typ ->
-            let set_builtin = Sil.Const (Sil.Cfun SymExec.ModelBuiltins.__set_mem_attribute) in
+       let set_mem_attr =
-            Sil.Call ([], set_builtin, [exp], loc, Sil.cf_default) in
+         let set_builtin = Sil.Const (Sil.Cfun SymExec.ModelBuiltins.__set_mem_attribute) in
-          call_instrs @ [set_mem_attr]
+         Sil.Call ([], set_builtin, [exp], loc, Sil.cf_default) in
-
+       (* Exceptions thrown in the close method should not prevent the resource from being *)
-      | _ -> call_instrs in
+       (* considered as closed *)
+       [set_mem_attr] @ call_instrs
+
+    | _ -> call_instrs in
 
   (callee_procdesc, callee_procname, call_idl, instrs)
 

infer/tests/codetoanalyze/java/infer/CloseableAsResourceExample.java

@@ -11,17 +11,19 @@ import java.io.StringReader;
 
 public class CloseableAsResourceExample {
 
+  native boolean star();
+
   class LocalException extends IOException {
   }
 
   class SomeResource implements Closeable {
 
-      native boolean isValid();
       void doSomething() throws LocalException {
-        if (!isValid()) {
+        if (!star()) {
           throw new LocalException();
         }
       }
+
       public void close() {}
   }
 
@@ -50,15 +52,15 @@ public class CloseableAsResourceExample {
     res.close();
   } // should report a resource leak
 
-  class Res implements Closeable {
+  class Resource implements Closeable {
-    public Res() {
+    public Resource() {
     }
     public void close() {}
   }
 
   class Wrapper implements Closeable {
-    Res mR;
+    Resource mR;
-    public Wrapper(Res r) {
+    public Wrapper(Resource r) {
       mR = r;
     }
     public void close() {
@@ -67,19 +69,19 @@ public class CloseableAsResourceExample {
   }
 
   class Sub extends Wrapper {
-    public Sub(Res r) {
+    public Sub(Resource r) {
       super(r);
     }
   }
 
   void closingWrapper() {
-    Res r = new Res();
+    Resource r = new Resource();
     Sub s = new Sub(r);
     s.close();
   }
 
   void notClosingWrapper() {
-    Sub s = new Sub(new Res());
+    Sub s = new Sub(new Resource());
     s.mR.close();
   }  // should report a resource leak
 
@@ -115,4 +117,23 @@ public class CloseableAsResourceExample {
     }
   }
 
+  class ResourceWithException implements Closeable {
+
+    public void close() throws IOException {
+      if (star()) {
+        throw new IOException();
+      }
+    }
+  }
+
+  void noLeakwithExceptionOnClose() throws IOException {
+    ResourceWithException res = new ResourceWithException();
+    res.close();
+  }
+
+  void noLeakWithCloseQuietlyAndExceptionOnClose() {
+    ResourceWithException res = new ResourceWithException();
+    Utils.closeQuietly(res);
+  }
+
 }