Race condition in JobServiceContext
BUG: 23981171
JobServiceContext has a reference to the running job which
1) needs to be copied before providing access to outside callers
2) was not guarded by a lock in one case which might result in an NPE
Change-Id: I7eb04052f3fe63e7b386c564a6bdebf9144e976a
diff --git a/services/core/java/com/android/server/job/JobServiceContext.java b/services/core/java/com/android/server/job/JobServiceContext.java
index d7fafe3..9aee588 100644
--- a/services/core/java/com/android/server/job/JobServiceContext.java
+++ b/services/core/java/com/android/server/job/JobServiceContext.java
@@ -109,7 +109,13 @@
int mVerb;
private AtomicBoolean mCancelled = new AtomicBoolean();
- /** All the information maintained about the job currently being executed. */
+ /**
+ * All the information maintained about the job currently being executed.
+ *
+ * Any reads (dereferences) not done from the handler thread must be synchronized on
+ * {@link #mLock}.
+ * Writes can only be done from the handler thread, or {@link #executeRunnableJob(JobStatus)}.
+ */
private JobStatus mRunningJob;
/** Binder to the client service. */
IJobService service;
@@ -194,7 +200,8 @@
*/
JobStatus getRunningJob() {
synchronized (mLock) {
- return mRunningJob;
+ return mRunningJob == null ?
+ null : new JobStatus(mRunningJob);
}
}
@@ -255,15 +262,22 @@
*/
@Override
public void onServiceConnected(ComponentName name, IBinder service) {
- if (!name.equals(mRunningJob.getServiceComponent())) {
+ JobStatus runningJob;
+ synchronized (mLock) {
+ // This isn't strictly necessary b/c the JobServiceHandler is running on the main
+ // looper and at this point we can't get any binder callbacks from the client. Better
+ // safe than sorry.
+ runningJob = mRunningJob;
+ }
+ if (runningJob == null || !name.equals(runningJob.getServiceComponent())) {
mCallbackHandler.obtainMessage(MSG_SHUTDOWN_EXECUTION).sendToTarget();
return;
}
this.service = IJobService.Stub.asInterface(service);
final PowerManager pm =
(PowerManager) mContext.getSystemService(Context.POWER_SERVICE);
- mWakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, mRunningJob.getTag());
- mWakeLock.setWorkSource(new WorkSource(mRunningJob.getUid()));
+ mWakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, runningJob.getTag());
+ mWakeLock.setWorkSource(new WorkSource(runningJob.getUid()));
mWakeLock.setReferenceCounted(false);
mWakeLock.acquire();
mCallbackHandler.obtainMessage(MSG_SERVICE_BOUND).sendToTarget();
@@ -281,13 +295,15 @@
* @return True if the binder calling is coming from the client we expect.
*/
private boolean verifyCallingUid() {
- if (mRunningJob == null || Binder.getCallingUid() != mRunningJob.getUid()) {
- if (DEBUG) {
- Slog.d(TAG, "Stale callback received, ignoring.");
+ synchronized (mLock) {
+ if (mRunningJob == null || Binder.getCallingUid() != mRunningJob.getUid()) {
+ if (DEBUG) {
+ Slog.d(TAG, "Stale callback received, ignoring.");
+ }
+ return false;
}
- return false;
+ return true;
}
- return true;
}
/**
diff --git a/services/core/java/com/android/server/job/controllers/JobStatus.java b/services/core/java/com/android/server/job/controllers/JobStatus.java
index 69c63f3..c02611f 100644
--- a/services/core/java/com/android/server/job/controllers/JobStatus.java
+++ b/services/core/java/com/android/server/job/controllers/JobStatus.java
@@ -82,6 +82,13 @@
this.numFailures = numFailures;
}
+ /** Copy constructor. */
+ public JobStatus(JobStatus jobStatus) {
+ this(jobStatus.getJob(), jobStatus.getUid(), jobStatus.getNumFailures());
+ this.earliestRunTimeElapsedMillis = jobStatus.getEarliestRunTime();
+ this.latestRunTimeElapsedMillis = jobStatus.getLatestRunTimeElapsed();
+ }
+
/** Create a newly scheduled job. */
public JobStatus(JobInfo job, int uId) {
this(job, uId, 0);