Merge "Disallow starting new system services after apex services are started."

services/core/java/com/android/server/SystemServiceManager.java

@@ -45,6 +45,8 @@ import java.io.File;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.InvocationTargetException;
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
@@ -91,7 +93,7 @@ public final class SystemServiceManager implements Dumpable {
     private long mRuntimeStartUptime;
 
     // Services that should receive lifecycle events.
-    private final ArrayList<SystemService> mServices = new ArrayList<SystemService>();
+    private List<SystemService> mServices;
 
     private int mCurrentPhase = -1;
 
@@ -113,11 +115,12 @@ public final class SystemServiceManager implements Dumpable {
 
     SystemServiceManager(Context context) {
         mContext = context;
+        mServices = new ArrayList<>();
         // Disable using the thread pool for low ram devices
         sUseLifecycleThreadPool = sUseLifecycleThreadPool
-                                    && !ActivityManager.isLowRamDeviceStatic();
+                && !ActivityManager.isLowRamDeviceStatic();
         mNumUserPoolThreads = Math.min(Runtime.getRuntime().availableProcessors(),
-                                       DEFAULT_MAX_USER_POOL_THREADS);
+                DEFAULT_MAX_USER_POOL_THREADS);
     }
 
     /**
@@ -220,11 +223,16 @@ public final class SystemServiceManager implements Dumpable {
         warnIfTooLong(SystemClock.elapsedRealtime() - time, service, "onStart");
     }
 
+    /** Disallow starting new services after this call. */
+    void sealStartedServices() {
+        mServices = Collections.unmodifiableList(mServices);
+    }
+
     /**
      * Starts the specified boot phase for all system services that have been started up to
      * this point.
      *
-     * @param t trace logger
+     * @param t     trace logger
      * @param phase The boot phase to start.
      */
     public void startBootPhase(@NonNull TimingsTraceAndSlog t, int phase) {
@@ -398,8 +406,8 @@ public final class SystemServiceManager implements Dumpable {
         // Limit the lifecycle parallelization to all users other than the system user
         // and only for the user start lifecycle phase for now.
         final boolean useThreadPool = sUseLifecycleThreadPool
-                                        && curUserId != UserHandle.USER_SYSTEM
-                                        && onWhat.equals(USER_STARTING);
+                && curUserId != UserHandle.USER_SYSTEM
+                && onWhat.equals(USER_STARTING);
         final ExecutorService threadPool =
                 useThreadPool ? Executors.newFixedThreadPool(mNumUserPoolThreads) : null;
         for (int i = 0; i < serviceLen; i++) {
@@ -419,7 +427,7 @@ public final class SystemServiceManager implements Dumpable {
                             + serviceName + " because it's not supported (curUser: "
                             + curUser + ", prevUser:" + prevUser + ")");
                 } else {
-                    Slog.i(TAG,  "Skipping " + onWhat + "User-" + curUserId + " on "
+                    Slog.i(TAG, "Skipping " + onWhat + "User-" + curUserId + " on "
                             + serviceName);
                 }
                 continue;
@@ -516,6 +524,7 @@ public final class SystemServiceManager implements Dumpable {
 
     /**
      * Returns whether we are booting into safe mode.
+     *
      * @return safe mode flag
      */
     public boolean isSafeMode() {
@@ -559,9 +568,10 @@ public final class SystemServiceManager implements Dumpable {
 
     /**
      * Ensures that the system directory exist creating one if needed.
+     *
+     * @return The system directory.
      * @deprecated Use {@link Environment#getDataSystemCeDirectory()}
      * or {@link Environment#getDataSystemDeDirectory()} instead.
-     * @return The system directory.
      */
     @Deprecated
     public static File ensureSystemDir() {
@@ -578,7 +588,9 @@ public final class SystemServiceManager implements Dumpable {
         pw.printf("Current phase: %d\n", mCurrentPhase);
         synchronized (mTargetUsers) {
             if (mCurrentUser != null) {
-                pw.print("Current user: "); mCurrentUser.dump(pw); pw.println();
+                pw.print("Current user: ");
+                mCurrentUser.dump(pw);
+                pw.println();
             } else {
                 pw.println("Current user not set!");
             }

services/java/com/android/server/SystemServer.java

@@ -920,12 +920,6 @@ public final class SystemServer implements Dumpable {
             startBootstrapServices(t);
             startCoreServices(t);
             startOtherServices(t);
-            // Apex services must be the last category of services to start. No other service must
-            // be starting after this point. This is to prevent unnessary stability issues when
-            // these apexes are updated outside of OTA; and to avoid breaking dependencies from
-            // system into apexes.
-            // TODO(satayev): lock mSystemServiceManager.startService to stop accepting new services
-            // after this step
             startApexServices(t);
         } catch (Throwable ex) {
             Slog.e("System", "******************************************");
@@ -3051,6 +3045,10 @@ public final class SystemServer implements Dumpable {
 
     /**
      * Starts system services defined in apexes.
+     *
+     * <p>Apex services must be the last category of services to start. No other service must be
+     * starting after this point. This is to prevent unnecessary stability issues when these apexes
+     * are updated outside of OTA; and to avoid breaking dependencies from system into apexes.
      */
     private void startApexServices(@NonNull TimingsTraceAndSlog t) {
         t.traceBegin("startApexServices");
@@ -3067,6 +3065,10 @@ public final class SystemServer implements Dumpable {
             }
             t.traceEnd();
         }
+
+        // make sure no other services are started after this point
+        mSystemServiceManager.sealStartedServices();
+
         t.traceEnd(); // startApexServices
     }
 

services/tests/servicestests/src/com/android/server/SystemServiceManagerTest.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.server;
+
+import static org.junit.Assert.assertThrows;
+
+import android.test.AndroidTestCase;
+
+import org.junit.Test;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+
+/**
+ * Tests for {@link SystemServiceManager}.
+ */
+public class SystemServiceManagerTest extends AndroidTestCase {
+
+    private static final String TAG = "SystemServiceManagerTest";
+
+    @Test
+    public void testSealStartedServices() throws Exception {
+        SystemServiceManager manager = new SystemServiceManager(getContext());
+        // must be effectively final, since it's changed from inner class below
+        AtomicBoolean serviceStarted = new AtomicBoolean(false);
+        SystemService service = new SystemService(getContext()) {
+            @Override
+            public void onStart() {
+                serviceStarted.set(true);
+            }
+        };
+
+        // started services have their #onStart methods called
+        manager.startService(service);
+        assertTrue(serviceStarted.get());
+
+        // however, after locking started services, it is not possible to start a new service
+        manager.sealStartedServices();
+        assertThrows(UnsupportedOperationException.class, () -> manager.startService(service));
+    }
+}