From a4a1a9c55e2b75ad7674e78ea97cdeef83b194c1 Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Fri, 20 Apr 2018 03:29:38 -0700 Subject: [PATCH] [starvation] Catch java IO blocking calls on UI thread Reviewed By: sblackshear Differential Revision: D7685918 fbshipit-source-id: 0ec46f2 --- infer/src/concurrency/RacerDConfig.ml | 38 ++++++++++ infer/src/concurrency/RacerDConfig.mli | 16 +++++ infer/src/concurrency/starvation.ml | 31 +------- .../codetoanalyze/java/starvation/JavaIO.java | 70 +++++++++++++++++++ .../codetoanalyze/java/starvation/issues.exp | 2 + 5 files changed, 129 insertions(+), 28 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/starvation/JavaIO.java diff --git a/infer/src/concurrency/RacerDConfig.ml b/infer/src/concurrency/RacerDConfig.ml index bcb655341..eac61ce14 100644 --- a/infer/src/concurrency/RacerDConfig.ml +++ b/infer/src/concurrency/RacerDConfig.ml @@ -540,4 +540,42 @@ module Models = struct false ) && Procdesc.get_access proc_desc <> PredSymb.Private && not (Annotations.pdesc_return_annot_ends_with proc_desc Annotations.visibleForTesting) + + + let is_blocking_java_io tenv pn = + match pn with + | Typ.Procname.Java java_pname -> + let classname = Typ.Procname.Java.get_class_type_name java_pname in + let mthd = Typ.Procname.Java.get_method java_pname in + let is_reader_or_inputstream = + let targets_str = ["java.io.Reader"; "java.io.InputStream"] in + let targets = List.map targets_str ~f:Typ.Name.Java.from_string in + fun tname _ -> List.mem targets tname ~equal:Typ.Name.equal + in + String.is_prefix mthd ~prefix:"read" + && PatternMatch.supertype_exists tenv is_reader_or_inputstream classname + | _ -> + false + + + let is_countdownlatch_await pn = + match pn with + | Typ.Procname.Java java_pname -> + let classname = Typ.Procname.Java.get_class_name java_pname in + let mthd = Typ.Procname.Java.get_method java_pname in + String.equal classname "java.util.concurrent.CountDownLatch" && String.equal mthd "await" + | _ -> + false + + + let is_two_way_binder_transact tenv actuals pn = + match pn with + | Typ.Procname.Java java_pname -> + let classname = Typ.Procname.Java.get_class_type_name java_pname in + let mthd = Typ.Procname.Java.get_method java_pname in + String.equal mthd "transact" + && PatternMatch.is_subtype_of_str tenv classname "android.os.IBinder" + && List.nth actuals 4 |> Option.value_map ~default:false ~f:HilExp.is_int_zero + | _ -> + false end diff --git a/infer/src/concurrency/RacerDConfig.mli b/infer/src/concurrency/RacerDConfig.mli index b91e203b1..4648cc33c 100644 --- a/infer/src/concurrency/RacerDConfig.mli +++ b/infer/src/concurrency/RacerDConfig.mli @@ -78,4 +78,20 @@ module Models : sig val should_report_on_proc : Procdesc.t -> Tenv.t -> bool (** return true if procedure is at an abstraction boundary or reporting has been explicitly requested via @ThreadSafe *) + + val is_blocking_java_io : Tenv.t -> Typ.Procname.t -> bool + (** It's surprisingly difficult making any sense of the official documentation on java.io classes + as to whether methods may block. We approximate these by calls in traditionally blocking + classes (non-channel based I/O), to methods `(Reader|InputStream).read*`. + Initially, (Writer|OutputStream).(flush|close) were also matched, but this generated too + many reports *) + + val is_countdownlatch_await : Typ.Procname.t -> bool + (** is the method called CountDownLath.await? *) + + val is_two_way_binder_transact : Tenv.t -> HilExp.t list -> Typ.Procname.t -> bool + (* an IBinder.transact call is an RPC. If the 4th argument (5th counting `this` as the first) + is int-zero then a reply is expected and returned from the remote process, thus potentially + blocking. If the 4th argument is anything else, we assume a one-way call which doesn't block. + *) end diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml index dfb358898..f81dc7222 100644 --- a/infer/src/concurrency/starvation.ml +++ b/infer/src/concurrency/starvation.ml @@ -20,32 +20,6 @@ let is_java_static pname = false -let is_countdownlatch_await pn = - match pn with - | Typ.Procname.Java java_pname -> - let classname = Typ.Procname.Java.get_class_name java_pname in - let mthd = Typ.Procname.Java.get_method java_pname in - String.equal classname "java.util.concurrent.CountDownLatch" && String.equal mthd "await" - | _ -> - false - - -(* an IBinder.transact call is an RPC. If the 4th argument (5th counting `this` as the first) - is int-zero then a reply is expected and returned from the remote process, thus potentially - blocking. If the 4th argument is anything else, we assume a one-way call which doesn't block. -*) -let is_two_way_binder_transact tenv actuals pn = - match pn with - | Typ.Procname.Java java_pname -> - let classname = Typ.Procname.Java.get_class_type_name java_pname in - let mthd = Typ.Procname.Java.get_method java_pname in - String.equal mthd "transact" - && PatternMatch.is_subtype_of_str tenv classname "android.os.IBinder" - && List.nth actuals 4 |> Option.value_map ~default:false ~f:HilExp.is_int_zero - | _ -> - false - - let is_on_main_thread pn = RacerDConfig.(match Models.get_thread pn with Models.MainThread -> true | _ -> false) @@ -106,8 +80,9 @@ module TransferFunctions (CFG : ProcCfg.S) = struct astate | NoEffect -> if - is_countdownlatch_await callee_pname - || is_two_way_binder_transact tenv actuals callee_pname + Models.is_countdownlatch_await callee_pname + || Models.is_two_way_binder_transact tenv actuals callee_pname + || Models.is_blocking_java_io tenv callee_pname then Domain.blocking_call callee_pname loc astate else if is_on_main_thread callee_pname then Domain.set_on_main_thread astate else diff --git a/infer/tests/codetoanalyze/java/starvation/JavaIO.java b/infer/tests/codetoanalyze/java/starvation/JavaIO.java new file mode 100644 index 000000000..b0091062a --- /dev/null +++ b/infer/tests/codetoanalyze/java/starvation/JavaIO.java @@ -0,0 +1,70 @@ +/* + * Copyright (c) 2018 - 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. + */ + +import java.io.FileReader; +import java.io.DataInputStream; +import java.io.FileWriter; +import java.io.DataOutputStream; +import java.io.IOException; +import android.support.annotation.UiThread; + +class JavaIO { + FileReader reader; + DataInputStream inputStream; + FileWriter writer; + DataOutputStream outputStream; + + int doFileRead() throws IOException { + return reader.read(); + } + + String doStreamRead() throws IOException { + return inputStream.readUTF(); + } + + @UiThread + void fileReadBad() throws IOException { + doFileRead(); + } + + @UiThread + void streamReadBad() throws IOException { + doStreamRead(); + } + + @UiThread + void writerMethodsOk() throws IOException { + writer = new FileWriter("bla"); + writer.write('a'); + writer.append('b'); + String enc = writer.getEncoding(); + } + + @UiThread + void readerMethodsOk() throws IOException { + reader = new FileReader("bla"); + String enc = reader.getEncoding(); + reader.markSupported(); + reader.reset(); + reader.close(); + } + + @UiThread + void outputStreamMethodsOk() throws IOException { + outputStream.write('a'); + outputStream.size(); + } + + @UiThread + void inputStreamMethodsOk() throws IOException { + inputStream.available(); + inputStream.reset(); + inputStream.close(); + } +} diff --git a/infer/tests/codetoanalyze/java/starvation/issues.exp b/infer/tests/codetoanalyze/java/starvation/issues.exp index 33bcaa410..6bac46d7f 100644 --- a/infer/tests/codetoanalyze/java/starvation/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation/issues.exp @@ -8,4 +8,6 @@ codetoanalyze/java/starvation/InnerClass.java, void InnerClass.outerInnerBad(Inn codetoanalyze/java/starvation/Interclass.java, void InterclassA.interclass2Bad(Interclass), 0, STARVATION, ERROR, [[Trace 1] Locks this in class InterclassA*,Method call: void Interclass.interclass2Bad(),Locks this in class Interclass*,[Trace 2] Locks this in class Interclass*,Method call: void InterclassA.interclass1Bad(),Locks this in class InterclassA*] codetoanalyze/java/starvation/Interproc.java, void Interproc.interproc1Bad(InterprocA), 0, STARVATION, ERROR, [[Trace 1] Locks this in class Interproc*,Method call: void Interproc.interproc2Bad(InterprocA),Locks b in class InterprocA*,[Trace 2] Locks this in class InterprocA*,Method call: void InterprocA.interproc2Bad(Interproc),Locks d in class Interproc*] codetoanalyze/java/starvation/Intraproc.java, void IntraprocA.intraBad(Intraproc), 0, STARVATION, ERROR, [[Trace 1] Method start: void IntraprocA.intraBad(Intraproc),Locks this in class IntraprocA*,Locks o in class Intraproc*,[Trace 2] Method start: void Intraproc.intraBad(IntraprocA),Locks this in class Intraproc*,Locks o in class IntraprocA*] +codetoanalyze/java/starvation/JavaIO.java, void JavaIO.fileReadBad(), 0, STARVATION, ERROR, [Method start: void JavaIO.fileReadBad(),Method call: int JavaIO.doFileRead(),Calls int InputStreamReader.read()] +codetoanalyze/java/starvation/JavaIO.java, void JavaIO.streamReadBad(), 0, STARVATION, ERROR, [Method start: void JavaIO.streamReadBad(),Method call: String JavaIO.doStreamRead(),Calls String DataInputStream.readUTF()] codetoanalyze/java/starvation/StaticLock.java, void StaticLock.lockOtherClassOneWayBad(), 0, STARVATION, ERROR, [[Trace 1] Method start: void StaticLock.lockOtherClassOneWayBad(),Locks StaticLock$0 in class java.lang.Class*,Locks this in class StaticLock*,[Trace 2] Locks this in class StaticLock*,Method call: void StaticLock.staticSynced(),Locks StaticLock$0 in class java.lang.Class*]