diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml index 3c591bea7..d779f424f 100644 --- a/infer/src/concurrency/starvation.ml +++ b/infer/src/concurrency/starvation.ml @@ -275,36 +275,37 @@ end module Analyzer = LowerHil.MakeAbstractInterpreter (TransferFunctions (ProcCfg.Normal)) -(** Before analyzing Java instance methods (non-constructor, non-static), compute the attributes of - instance variables that all constructors agree on after termination. *) -let add_constructor_attributes tenv procname (astate : Domain.t) = +(** Compute the attributes (of static variables) set up by the class initializer. *) +let set_class_init_attributes procname (astate : Domain.t) = let open Domain in - let skip_constructor_analysis = - match procname with - | Typ.Procname.Java jname -> - (* constructor attributes affect instance fields, which static methods cannot access; - ditto for the class initializer; finally, don't recurse if already in constructor *) - Typ.Procname.Java.(is_static jname || is_constructor jname || is_class_initializer jname) - | _ -> - (* don't do anything for non-Java code *) - true + let attributes = + Typ.Procname.get_class_type_name procname + |> Option.map ~f:(fun tname -> Typ.Procname.(Java (Java.get_class_initializer tname))) + |> Option.bind ~f:Payload.read_toplevel_procedure + |> Option.value_map ~default:AttributeDomain.top ~f:(fun summary -> summary.attributes) in - if skip_constructor_analysis then astate - else - (* make a local [this] variable, for replacing all constructor attribute map keys' roots *) - let local_this = Pvar.mk Mangled.this procname |> Var.of_pvar in - let make_local exp = - let var, typ = HilExp.AccessExpression.get_base exp in - let newvar = - assert (Var.is_this var) ; - local_this - in - HilExp.AccessExpression.replace_base ~remove_deref_after_base:false (newvar, typ) exp - in - let localize_attrs attributes = - AttributeDomain.(fold (fun exp attr acc -> add (make_local exp) attr acc) attributes empty) - in - (* get class of current procedure *) + ({astate with attributes} : t) + + +(** Compute the attributes of instance variables that all constructors agree on. *) +let set_constructor_attributes tenv procname (astate : Domain.t) = + let open Domain in + (* make a local [this] variable, for replacing all constructor attribute map keys' roots *) + let local_this = Pvar.mk Mangled.this procname |> Var.of_pvar in + let make_local exp = + (* contract here matches that of [StarvationDomain.summary_of_astate] *) + let var, typ = HilExp.AccessExpression.get_base exp in + if Var.is_global var then + (* let expressions rooted at globals unchanged, these are probably from class initialiser *) + exp + else ( + assert (Var.is_this var) ; + HilExp.AccessExpression.replace_base ~remove_deref_after_base:false (local_this, typ) exp ) + in + let localize_attrs attributes = + AttributeDomain.(fold (fun exp attr acc -> add (make_local exp) attr acc) attributes empty) + in + let attributes = Typ.Procname.get_class_type_name procname (* retrieve its definition *) |> Option.bind ~f:(Tenv.lookup tenv) @@ -319,7 +320,26 @@ let add_constructor_attributes tenv procname (astate : Domain.t) = (* join all the attribute maps together *) |> List.reduce ~f:AttributeDomain.join |> Option.value ~default:AttributeDomain.top - |> fun attributes -> {astate with attributes} + in + {astate with attributes} + + +let set_initial_attributes tenv procname astate = + match procname with + | Typ.Procname.Java java_pname when Typ.Procname.Java.is_class_initializer java_pname -> + (* we are analyzing the class initializer, don't go through on-demand again *) + astate + | Typ.Procname.Java java_pname + when Typ.Procname.Java.(is_constructor java_pname || is_static java_pname) -> + (* analyzing a constructor or static method, so we need the attributes established by the + class initializer *) + set_class_init_attributes procname astate + | Typ.Procname.Java _ -> + (* we are analyzing an instance method, so we need constructor-established attributes + which will include those by the class initializer *) + set_constructor_attributes tenv procname astate + | _ -> + astate let analyze_procedure {Callbacks.exe_env; summary} = @@ -362,8 +382,8 @@ let analyze_procedure {Callbacks.exe_env; summary} = in let initial = Domain.bottom - (* set the attributes of instance variables to those achieved by all constructors *) - |> add_constructor_attributes tenv procname + (* set the attributes of instance variables set up by all constructors or the class initializer *) + |> set_initial_attributes tenv procname |> set_lock_state_for_synchronized_proc |> set_thread_status_by_annotation in Analyzer.compute_post proc_data ~initial diff --git a/infer/src/concurrency/starvationDomain.ml b/infer/src/concurrency/starvationDomain.ml index f826b00b8..a2b014919 100644 --- a/infer/src/concurrency/starvationDomain.ml +++ b/infer/src/concurrency/starvationDomain.ml @@ -690,19 +690,22 @@ let integrate_summary ?tenv ?lhs callsite (astate : t) (summary : summary) = let summary_of_astate : Procdesc.t -> t -> summary = fun proc_desc astate -> let proc_name = Procdesc.get_proc_name proc_desc in - let is_java_constr = - match (proc_name : Typ.Procname.t) with - | Java jname -> - Typ.Procname.Java.is_constructor jname - | _ -> - false - in let attributes = - if is_java_constr then - (* only keep attributes that have [this] as their root *) - let filter_this exp _ = HilExp.AccessExpression.get_base exp |> fst |> Var.is_this in - AttributeDomain.filter filter_this astate.attributes - else AttributeDomain.empty + let var_predicate = + match proc_name with + | Typ.Procname.Java jname when Typ.Procname.Java.is_class_initializer jname -> + (* only keep static attributes for the class initializer *) + fun v -> Var.is_global v + | Typ.Procname.Java jname when Typ.Procname.Java.is_constructor jname -> + (* only keep static attributes or ones that have [this] as their root *) + fun v -> Var.is_this v || Var.is_global v + | _ -> + (* non-constructor/class initializer or non-java, don't keep any attributes *) + Fn.const false + in + AttributeDomain.filter + (fun exp _ -> HilExp.AccessExpression.get_base exp |> fst |> var_predicate) + astate.attributes in let return_attribute = let return_var_exp = diff --git a/infer/tests/codetoanalyze/java/starvation-whole-program/StaticInitAttributes.java b/infer/tests/codetoanalyze/java/starvation-whole-program/StaticInitAttributes.java new file mode 100644 index 000000000..4d552bbee --- /dev/null +++ b/infer/tests/codetoanalyze/java/starvation-whole-program/StaticInitAttributes.java @@ -0,0 +1,66 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import android.os.Binder; +import android.os.Handler; +import android.os.Looper; +import android.os.RemoteException; +import java.util.concurrent.Executor; + +class StaticInitAttributes { + static Binder binder; + + private static void doTransact() { + try { + binder.transact(0, null, null, 0); + } catch (RemoteException e) { + } + } + + @ForUiThread static Executor mUiThreadExecutor = null; + + static Executor mUiExecutor; + static Executor mNonUiExecutor; + static Handler mUiHandler; + static Runnable mBadRunnable; + static Runnable mOkRunnable; + + static { + mUiExecutor = mUiThreadExecutor; + mNonUiExecutor = Executors.getBackgroundExecutor(); + mUiHandler = new Handler(Looper.getMainLooper()); + mBadRunnable = + new Runnable() { + @Override + public void run() { + doTransact(); + } + }; + + mOkRunnable = + new Runnable() { + @Override + public void run() {} + }; + } + + public void postBlockingCallToUIExecutorBad() { + mUiExecutor.execute(mBadRunnable); + } + + public void postNoopCallToUIExecutorOk() { + mUiExecutor.execute(mOkRunnable); + } + + public void postBlockingCallToNonUIExecutorOk() { + mNonUiExecutor.execute(mBadRunnable); + } + + public void postBlockingCallToUIHandlerBad() { + mUiHandler.post(mBadRunnable); + } +} diff --git a/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp b/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp index 9e4874bc2..82b71b2b0 100644 --- a/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp @@ -32,6 +32,8 @@ codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceC codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onNullBinding(android.content.ComponentName):void, 31, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onNullBinding(ComponentName)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onServiceConnected(android.content.ComponentName,android.os.IBinder):void, 36, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onServiceConnected(ComponentName,IBinder)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onServiceDisconnected(android.content.ComponentName):void, 41, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onServiceDisconnected(ComponentName)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] +codetoanalyze/java/starvation-whole-program/StaticInitAttributes.java, StaticInitAttributes.postBlockingCallToUIExecutorBad():void, 52, STARVATION, no_bucket, ERROR, [`void StaticInitAttributes.postBlockingCallToUIExecutorBad()`,Method call: `void StaticInitAttributes$1.run()`,Method call: `void StaticInitAttributes.doTransact()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] +codetoanalyze/java/starvation-whole-program/StaticInitAttributes.java, StaticInitAttributes.postBlockingCallToUIHandlerBad():void, 64, STARVATION, no_bucket, ERROR, [`void StaticInitAttributes.postBlockingCallToUIHandlerBad()`,Method call: `void StaticInitAttributes$1.run()`,Method call: `void StaticInitAttributes.doTransact()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/ThreadScheduling.java, ThreadScheduling.scheduleBlockingCallOnContendedLockBad():void, 36, STARVATION, no_bucket, ERROR, [[Trace 1] `void ThreadScheduling.scheduleBlockingCallOnContendedLockBad()`,Method call: `void ThreadScheduling$2.run()`, locks `this.monitorA` in `class ThreadScheduling`,[Trace 2] `void ThreadScheduling.scheduleBlockingCallOnContendedLockBad()`,Method call: `void ThreadScheduling$1.run()`, locks `this.monitorA` in `class ThreadScheduling`,Method call: `void ThreadScheduling.doTransact()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/ThreadScheduling.java, ThreadScheduling.scheduleBlockingCallOnContendedLockViaInheritanceBad():void, 89, STARVATION, no_bucket, ERROR, [[Trace 1] `void ThreadScheduling.scheduleBlockingCallOnContendedLockViaInheritanceBad()`,Method call: `void ThreadScheduling$5.run()`, locks `this.monitorD` in `class ThreadScheduling`,[Trace 2] `void ThreadScheduling.scheduleBlockingCallOnContendedLockViaInheritanceBad()`,Method call: `void ThreadScheduling$BadThread.run()`, locks `this.monitorD` in `class ThreadScheduling`,Method call: `void ThreadScheduling.doTransact()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/ThreadScheduling.java, ThreadScheduling.scheduleDeadlockBad():void, 60, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadScheduling.scheduleDeadlockBad()`,Method call: `void ThreadScheduling$3.run()`, locks `this.monitorB` in `class ThreadScheduling`, locks `this.monitorC` in `class ThreadScheduling`,[Trace 2] `void ThreadScheduling.scheduleDeadlockBad()`,Method call: `void ThreadScheduling$4.run()`, locks `this.monitorC` in `class ThreadScheduling`, locks `this.monitorB` in `class ThreadScheduling`]