Improve AsyncTask data flow support

Model the life-cycle described here: https://developer.android.com/reference/android/os/AsyncTask\#the-4-steps

Author: atorralba

java/ql/lib/semmle/code/java/frameworks/android/AsyncTask.qll

@@ -4,49 +4,103 @@ import java
 private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.FlowSteps
 
+/*
+ * The following flow steps aim to model the life-cycle of `AsyncTask`s described here:
+ * https://developer.android.com/reference/android/os/AsyncTask#the-4-steps
+ */
+
 /**
- * Models the value-preserving step from `asyncTask.execute(params)` to `AsyncTask::doInBackground(params)`.
+ * A taint step from the vararg arguments of `AsyncTask::execute` and `AsyncTask::executeOnExecutor`
+ * to the parameter of `AsyncTask::doInBackground`.
  */
-private class AsyncTaskAdditionalValueStep extends AdditionalValueStep {
+private class AsyncTaskExecuteAdditionalValueStep extends AdditionalTaintStep {
   override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
     exists(ExecuteAsyncTaskMethodAccess ma, AsyncTaskRunInBackgroundMethod m |
-      DataFlow::getInstanceArgument(ma).getType() = m.getDeclaringType() and
+      DataFlow::getInstanceArgument(ma).getType() = m.getDeclaringType()
+    |
       node1.asExpr() = ma.getParamsArgument() and
       node2.asParameter() = m.getParameter(0)
     )
   }
 }
 
+/**
+ * A value-preserving step from the return value of `AsyncTask::doInBackground`
+ * to the parameter of `AsyncTask::onPostExecute`.
+ */
+private class AsyncTaskOnPostExecuteAdditionalValueStep extends AdditionalValueStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(
+      AsyncTaskRunInBackgroundMethod runInBackground, AsyncTaskOnPostExecuteMethod onPostExecute
+    |
+      onPostExecute.getDeclaringType() = runInBackground.getDeclaringType()
+    |
+      node1.asExpr() = any(ReturnStmt r | r.getEnclosingCallable() = runInBackground).getResult() and
+      node2.asParameter() = onPostExecute.getParameter(0)
+    )
+  }
+}
+
+/**
+ * A value-preserving step from field initializers in `AsyncTask`'s constructor or initializer method
+ * to the instance parameter of `AsyncTask::runInBackground` and `AsyncTask::onPostExecute`.
+ */
+private class AsyncTaskFieldInitQualifierToInstanceParameterStep extends AdditionalValueStep {
+  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(AsyncTaskInit init, Callable receiver |
+      n1.(DataFlow::PostUpdateNode).getPreUpdateNode() =
+        DataFlow::getFieldQualifier(any(FieldWrite f | f.getEnclosingCallable() = init)) and
+      n2.(DataFlow::InstanceParameterNode).getCallable() = receiver and
+      receiver.getDeclaringType() = init.getDeclaringType() and
+      (
+        receiver instanceof AsyncTaskRunInBackgroundMethod or
+        receiver instanceof AsyncTaskOnPostExecuteMethod
+      )
+    )
+  }
+}
+
 /**
  * The Android class `android.os.AsyncTask`.
  */
 private class AsyncTask extends RefType {
   AsyncTask() { this.hasQualifiedName("android.os", "AsyncTask") }
 }
 
+/** The constructor or initializer method of the `android.os.AsyncTask` class. */
+private class AsyncTaskInit extends Callable {
+  AsyncTaskInit() {
+    this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
+    (this instanceof Constructor or this instanceof InitializerMethod)
+  }
+}
+
 /** A call to the `execute` or `executeOnExecutor` methods of the `android.os.AsyncTask` class. */
 private class ExecuteAsyncTaskMethodAccess extends MethodAccess {
   Argument paramsArgument;
 
   ExecuteAsyncTaskMethodAccess() {
-    exists(Method m |
-      this.getMethod() = m and
-      m.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask
-    |
-      m.getName() = "execute" and not m.isStatic() and paramsArgument = this.getArgument(0)
-      or
-      m.getName() = "executeOnExecutor" and paramsArgument = this.getArgument(1)
-    )
+    this.getMethod().hasName(["execute", "executeOnExecutor"]) and
+    this.getMethod().getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof
+      AsyncTask
   }
 
   /** Returns the `params` argument of this call. */
-  Argument getParamsArgument() { result = paramsArgument }
+  Argument getParamsArgument() { result = this.getAnArgument() and result.isVararg() }
 }
 
 /** The `doInBackground` method of the `android.os.AsyncTask` class. */
 private class AsyncTaskRunInBackgroundMethod extends Method {
   AsyncTaskRunInBackgroundMethod() {
     this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
-    this.getName() = "doInBackground"
+    this.hasName("doInBackground")
+  }
+}
+
+/** The `onPostExecute` method of the `android.os.AsyncTask` class. */
+private class AsyncTaskOnPostExecuteMethod extends Method {
+  AsyncTaskOnPostExecuteMethod() {
+    this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
+    this.hasName("onPostExecute")
   }
 }

java/ql/test/library-tests/frameworks/android/asynctask/Test.java

@@ -10,25 +10,54 @@ private static void sink(Object o) {}
 
     public void test() {
         TestAsyncTask t = new TestAsyncTask();
-        t.execute(source("execute"));
-        t.executeOnExecutor(null, source("executeOnExecutor"));
+        t.execute(source("execute"), null);
+        t.executeOnExecutor(null, source("executeOnExecutor"), null);
         SafeAsyncTask t2 = new SafeAsyncTask();
         t2.execute("safe");
+        TestConstructorTask t3 = new TestConstructorTask(source("constructor"), "safe");
+        t3.execute(source("params"));
     }
 
     private class TestAsyncTask extends AsyncTask<Object, Object, Object> {
         @Override
         protected Object doInBackground(Object... params) {
-            sink(params); // $ hasValueFlow=execute hasValueFlow=executeOnExecutor
+            sink(params[0]); // $ hasTaintFlow=execute hasTaintFlow=executeOnExecutor
+            sink(params[1]); // $ SPURIOUS: hasTaintFlow=execute hasTaintFlow=executeOnExecutor
             return null;
         }
     }
 
     private class SafeAsyncTask extends AsyncTask<Object, Object, Object> {
         @Override
         protected Object doInBackground(Object... params) {
-            sink(params); // Safe
+            sink(params[0]); // Safe
             return null;
         }
     }
+
+    class TestConstructorTask extends AsyncTask<Object, Object, Object> {
+        private Object field;
+        private Object safeField;
+
+        public TestConstructorTask(Object field, Object safeField) {
+            this.field = field;
+            this.safeField = safeField;
+        }
+
+        @Override
+        protected Object doInBackground(Object... params) {
+            sink(params[0]); // $ hasTaintFlow=params
+            sink(field); // $ hasValueFlow=constructor
+            sink(safeField); // Safe
+            return params[0];
+        }
+
+        @Override
+        protected void onPostExecute(Object param) {
+            sink(param); // $ hasTaintFlow=params
+            sink(field); // $ hasValueFlow=constructor
+            sink(safeField); // Safe
+        }
+
+    }
 }

java/ql/test/library-tests/frameworks/android/asynctask/test.ql

@@ -1,6 +1,3 @@
 import java
 import TestUtilities.InlineFlowTest
-
-class AsyncTaskTest extends InlineFlowTest {
-  override TaintTracking::Configuration getTaintFlowConfig() { none() }
-}
+import semmle.code.java.dataflow.FlowSteps