From b770235a860fc08dc07759c59ab0dc8fcb96a0a3 Mon Sep 17 00:00:00 2001
From: Rubin Xu <rubinxu@google.com>
Date: Fri, 7 Feb 2020 12:02:00 +0000
Subject: [PATCH] Mitigate race conditions in PacService

There are some design limitations in PacService (one-way aidl calls
from ConnectivityService) that causes it to be racy when PAC proxy are
set and cleared in quick succession. Attempt to mitigate them with the
following changes:

1. Make PacNative a singleton instead of one instance per binder. The
underlying v8 engine is singleton so it makes little sense to have
multiple instances of the PacNative wrapper.
2. Remove the startPacSystem and stopPacSystem API and bind the
PacNative lifecycle to the PacService. Otherwise the one-way
stopPacSystem() binder call could have raced with a next
startPacSystem() call when PAC proxy is cleared and then set.

For this change, startPacSystem() and stopPacSystem() and made no-op
only. They will be fully removed in the next change.

Test: atest --iterations 200 com.android.cts.devicepolicy.DeviceOwnerTest#testProxyPacProxyTest
Bug: 147359729
Change-Id: Ie3ce098167694421f8bd2a6dec85d7c437cfb0be
EDIT
---
 .../com/android/pacprocessor/PacNative.java   |  8 +++-
 .../com/android/pacprocessor/PacService.java  | 38 ++++---------------
 2 files changed, 14 insertions(+), 32 deletions(-)

diff --git a/packages/services/PacProcessor/src/com/android/pacprocessor/PacNative.java b/packages/services/PacProcessor/src/com/android/pacprocessor/PacNative.java
index c67fe9ffa9166..1e8109cb393c5 100644
--- a/packages/services/PacProcessor/src/com/android/pacprocessor/PacNative.java
+++ b/packages/services/PacProcessor/src/com/android/pacprocessor/PacNative.java
@@ -23,6 +23,8 @@ import android.util.Log;
 public class PacNative {
     private static final String TAG = "PacProxy";
 
+    private static final PacNative sInstance = new PacNative();
+
     private String mCurrentPac;
 
     private boolean mIsActive;
@@ -39,10 +41,14 @@ public class PacNative {
         System.loadLibrary("jni_pacprocessor");
     }
 
-    PacNative() {
+    private PacNative() {
 
     }
 
+    public static PacNative getInstance() {
+        return sInstance;
+    }
+
     public synchronized boolean startPacSupport() {
         if (createV8ParserNativeLocked()) {
             Log.e(TAG, "Unable to Create v8 Proxy Parser.");
diff --git a/packages/services/PacProcessor/src/com/android/pacprocessor/PacService.java b/packages/services/PacProcessor/src/com/android/pacprocessor/PacService.java
index 74391eb676d1f..b006d6e1fa7bf 100644
--- a/packages/services/PacProcessor/src/com/android/pacprocessor/PacService.java
+++ b/packages/services/PacProcessor/src/com/android/pacprocessor/PacService.java
@@ -31,43 +31,27 @@ import java.net.URL;
 public class PacService extends Service {
     private static final String TAG = "PacService";
 
-    private PacNative mPacNative;
-    private ProxyServiceStub mStub;
+    private PacNative mPacNative = PacNative.getInstance();
+    private ProxyServiceStub mStub = new ProxyServiceStub();
 
     @Override
     public void onCreate() {
         super.onCreate();
-        if (mPacNative == null) {
-            mPacNative = new PacNative();
-            mStub = new ProxyServiceStub(mPacNative);
-        }
+        mPacNative.startPacSupport();
     }
 
     @Override
     public void onDestroy() {
+        mPacNative.stopPacSupport();
         super.onDestroy();
-        if (mPacNative != null) {
-            mPacNative.stopPacSupport();
-            mPacNative = null;
-            mStub = null;
-        }
     }
 
     @Override
     public IBinder onBind(Intent intent) {
-        if (mPacNative == null) {
-            mPacNative = new PacNative();
-            mStub = new ProxyServiceStub(mPacNative);
-        }
         return mStub;
     }
 
-    private static class ProxyServiceStub extends IProxyService.Stub {
-        private final PacNative mPacNative;
-
-        public ProxyServiceStub(PacNative pacNative) {
-            mPacNative = pacNative;
-        }
+    private class ProxyServiceStub extends IProxyService.Stub {
 
         @Override
         public String resolvePacFile(String host, String url) throws RemoteException {
@@ -102,20 +86,12 @@ public class PacService extends Service {
 
         @Override
         public void startPacSystem() throws RemoteException {
-            if (Binder.getCallingUid() != Process.SYSTEM_UID) {
-                Log.e(TAG, "Only system user is allowed to call startPacSystem");
-                throw new SecurityException();
-            }
-            mPacNative.startPacSupport();
+            //TODO: remove
         }
 
         @Override
         public void stopPacSystem() throws RemoteException {
-            if (Binder.getCallingUid() != Process.SYSTEM_UID) {
-                Log.e(TAG, "Only system user is allowed to call stopPacSystem");
-                throw new SecurityException();
-            }
-            mPacNative.stopPacSupport();
+            //TODO: remove
         }
     }
 }