Merge "Fixes two data races in USAP pool management." into qt-dev-plus-aosp

core/java/com/android/internal/os/Zygote.java

@@ -519,6 +519,9 @@ public final class Zygote {
             try {
                 sessionSocket = usapPoolSocket.accept();
 
+                // Block SIGTERM so we won't be killed if the Zygote flushes the USAP pool.
+                blockSigTerm();
+
                 BufferedReader usapReader =
                         new BufferedReader(new InputStreamReader(sessionSocket.getInputStream()));
                 usapOutputStream =
@@ -537,95 +540,116 @@ public final class Zygote {
                 } else {
                     Log.e("USAP", "Truncated command received.");
                     IoUtils.closeQuietly(sessionSocket);
+
+                    // Re-enable SIGTERM so the USAP can be flushed from the pool if necessary.
+                    unblockSigTerm();
                 }
             } catch (Exception ex) {
                 Log.e("USAP", ex.getMessage());
                 IoUtils.closeQuietly(sessionSocket);
+
+                // Re-enable SIGTERM so the USAP can be flushed from the pool if necessary.
+                unblockSigTerm();
             }
         }
 
-        setAppProcessName(args, "USAP");
-
-        applyUidSecurityPolicy(args, peerCredentials);
-        applyDebuggerSystemProperty(args);
-
-        int[][] rlimits = null;
-
-        if (args.mRLimits != null) {
-            rlimits = args.mRLimits.toArray(INT_ARRAY_2D);
-        }
-
-        // This must happen before the SELinux policy for this process is
-        // changed when specializing.
         try {
-            // Used by ZygoteProcess.zygoteSendArgsAndGetResult to fill in a
+            // SIGTERM is blocked on loop exit.  This prevents a USAP that is specializing from
-            // Process.ProcessStartResult object.
+            // being killed during a pool flush.
-            usapOutputStream.writeInt(pid);
+
-        } catch (IOException ioEx) {
+            applyUidSecurityPolicy(args, peerCredentials);
-            Log.e("USAP", "Failed to write response to session socket: " + ioEx.getMessage());
+            applyDebuggerSystemProperty(args);
-            System.exit(-1);
+
-        } finally {
+            int[][] rlimits = null;
-            IoUtils.closeQuietly(sessionSocket);
+
+            if (args.mRLimits != null) {
+                rlimits = args.mRLimits.toArray(INT_ARRAY_2D);
+            }
+
+            // This must happen before the SELinux policy for this process is
+            // changed when specializing.
+            try {
+                // Used by ZygoteProcess.zygoteSendArgsAndGetResult to fill in a
+                // Process.ProcessStartResult object.
+                usapOutputStream.writeInt(pid);
+            } catch (IOException ioEx) {
+                Log.e("USAP", "Failed to write response to session socket: "
+                        + ioEx.getMessage());
+                throw new RuntimeException(ioEx);
+            } finally {
+                IoUtils.closeQuietly(sessionSocket);
+
+                try {
+                    // This socket is closed using Os.close due to an issue with the implementation
+                    // of LocalSocketImp.close().  Because the raw FD is created by init and then
+                    // loaded from an environment variable (as opposed to being created by the
+                    // LocalSocketImpl itself) the current implementation will not actually close
+                    // the underlying FD.
+                    //
+                    // See b/130309968 for discussion of this issue.
+                    Os.close(usapPoolSocket.getFileDescriptor());
+                } catch (ErrnoException ex) {
+                    Log.e("USAP", "Failed to close USAP pool socket");
+                    throw new RuntimeException(ex);
+                }
+            }
 
             try {
-                // This socket is closed using Os.close due to an issue with the implementation of
+                ByteArrayOutputStream buffer =
-                // LocalSocketImp.close.  Because the raw FD is created by init and then loaded from
+                        new ByteArrayOutputStream(Zygote.USAP_MANAGEMENT_MESSAGE_BYTES);
-                // an environment variable (as opposed to being created by the LocalSocketImpl
+                DataOutputStream outputStream = new DataOutputStream(buffer);
-                // itself) the current implementation will not actually close the underlying FD.
+
-                //
+                // This is written as a long so that the USAP reporting pipe and USAP pool event FD
-                // See b/130309968 for discussion of this issue.
+                // handlers in ZygoteServer.runSelectLoop can be unified.  These two cases should
-                Os.close(usapPoolSocket.getFileDescriptor());
+                // both send/receive 8 bytes.
-            } catch (ErrnoException ex) {
+                outputStream.writeLong(pid);
-                Log.e("USAP", "Failed to close USAP pool socket: " + ex.getMessage());
+                outputStream.flush();
+
+                Os.write(writePipe, buffer.toByteArray(), 0, buffer.size());
+            } catch (Exception ex) {
+                Log.e("USAP",
+                        String.format("Failed to write PID (%d) to pipe (%d): %s",
+                                pid, writePipe.getInt$(), ex.getMessage()));
+                throw new RuntimeException(ex);
+            } finally {
+                IoUtils.closeQuietly(writePipe);
             }
-        }
 
-        try {
+            specializeAppProcess(args.mUid, args.mGid, args.mGids,
-            ByteArrayOutputStream buffer =
+                                 args.mRuntimeFlags, rlimits, args.mMountExternal,
-                    new ByteArrayOutputStream(Zygote.USAP_MANAGEMENT_MESSAGE_BYTES);
+                                 args.mSeInfo, args.mNiceName, args.mStartChildZygote,
-            DataOutputStream outputStream = new DataOutputStream(buffer);
+                                 args.mInstructionSet, args.mAppDataDir);
 
-            // This is written as a long so that the USAP reporting pipe and USAP pool event FD
+            disableExecuteOnly(args.mTargetSdkVersion);
-            // handlers in ZygoteServer.runSelectLoop can be unified.  These two cases should both
-            // send/receive 8 bytes.
-            outputStream.writeLong(pid);
-            outputStream.flush();
 
-            Os.write(writePipe, buffer.toByteArray(), 0, buffer.size());
+            if (args.mNiceName != null) {
-        } catch (Exception ex) {
+                Process.setArgV0(args.mNiceName);
-            Log.e("USAP",
+            }
-                    String.format("Failed to write PID (%d) to pipe (%d): %s",
+
-                            pid, writePipe.getInt$(), ex.getMessage()));
+            // End of the postFork event.
-            System.exit(-1);
+            Trace.traceEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER);
+
+            return ZygoteInit.zygoteInit(args.mTargetSdkVersion,
+                                         args.mRemainingArgs,
+                                         null /* classLoader */);
         } finally {
-            IoUtils.closeQuietly(writePipe);
+            // Unblock SIGTERM to restore the process to default behavior.
-        }
+            unblockSigTerm();
-
-        specializeAppProcess(args.mUid, args.mGid, args.mGids,
-                             args.mRuntimeFlags, rlimits, args.mMountExternal,
-                             args.mSeInfo, args.mNiceName, args.mStartChildZygote,
-                             args.mInstructionSet, args.mAppDataDir);
-
-        disableExecuteOnly(args.mTargetSdkVersion);
-
-        // End of the postFork event.
-        Trace.traceEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER);
-
-        return ZygoteInit.zygoteInit(args.mTargetSdkVersion,
-                                     args.mRemainingArgs,
-                                     null /* classLoader */);
-    }
-
-    static void setAppProcessName(ZygoteArguments args, String loggingTag) {
-        if (args.mNiceName != null) {
-            Process.setArgV0(args.mNiceName);
-        } else if (args.mPackageName != null) {
-            Process.setArgV0(args.mPackageName);
-        } else {
-            Log.w(loggingTag, "Unable to set package name.");
         }
     }
-    
+
+    private static void blockSigTerm() {
+        nativeBlockSigTerm();
+    }
+
+    private static native void nativeBlockSigTerm();
+
+    private static void unblockSigTerm() {
+        nativeUnblockSigTerm();
+    }
+
+    private static native void nativeUnblockSigTerm();
+
     private static final String USAP_ERROR_PREFIX = "Invalid command to USAP: ";
 
     /**

core/java/com/android/internal/os/ZygoteConnection.java

@@ -322,6 +322,7 @@ class ZygoteConnection {
             Runnable stateChangeCode) {
         try {
             if (zygoteServer.isUsapPoolEnabled()) {
+                Log.i(TAG, "Emptying USAP Pool due to state change.");
                 Zygote.emptyUsapPool();
             }
 
@@ -335,6 +336,8 @@ class ZygoteConnection {
                 if (fpResult != null) {
                     zygoteServer.setForkChild();
                     return fpResult;
+                } else {
+                    Log.i(TAG, "Finished refilling USAP Pool after state change.");
                 }
             }
 
@@ -467,8 +470,6 @@ class ZygoteConnection {
 
         closeSocket();
 
-        Zygote.setAppProcessName(parsedArgs, TAG);
-
         // End of the postFork event.
         Trace.traceEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER);
         if (parsedArgs.mInvokeWith != null) {

core/jni/com_android_internal_os_Zygote.cpp

@@ -198,7 +198,7 @@ class UsapTableEntry {
    * PIDs don't match nothing will happen.
    *
    * @param pid The ID of the process who's entry we want to clear.
-   * @return True if the entry was cleared; false otherwise
+   * @return True if the entry was cleared by this call; false otherwise
    */
   bool ClearForPID(int32_t pid) {
     EntryStorage storage = mStorage.load();
@@ -212,14 +212,16 @@ class UsapTableEntry {
        *   3) It fails and the new value isn't INVALID_ENTRY_VALUE, in which
        *      case the entry has already been cleared and re-used.
        *
-       * In all three cases the goal of the caller has been met and we can
+       * In all three cases the goal of the caller has been met, but only in
-       * return true.
+       * the first case do we need to decrement the pool count.
        */
       if (mStorage.compare_exchange_strong(storage, INVALID_ENTRY_VALUE)) {
         close(storage.read_pipe_fd);
+        return true;
+      } else {
+        return false;
       }
 
-      return true;
     } else {
       return false;
     }
@@ -331,11 +333,24 @@ static void SigChldHandler(int /*signal_number*/) {
     if (WIFEXITED(status)) {
       async_safe_format_log(ANDROID_LOG_INFO, LOG_TAG,
                             "Process %d exited cleanly (%d)", pid, WEXITSTATUS(status));
+
+      // Check to see if the PID is in the USAP pool and remove it if it is.
+      if (RemoveUsapTableEntry(pid)) {
+        ++usaps_removed;
+      }
     } else if (WIFSIGNALED(status)) {
       async_safe_format_log(ANDROID_LOG_INFO, LOG_TAG,
                             "Process %d exited due to signal %d (%s)%s", pid,
                             WTERMSIG(status), strsignal(WTERMSIG(status)),
                             WCOREDUMP(status) ? "; core dumped" : "");
+
+      // If the process exited due to a signal other than SIGTERM, check to see
+      // if the PID is in the USAP pool and remove it if it is.  If the process
+      // was closed by the Zygote using SIGTERM then the USAP pool entry will
+      // have already been removed (see nativeEmptyUsapPool()).
+      if (WTERMSIG(status) != SIGTERM && RemoveUsapTableEntry(pid)) {
+        ++usaps_removed;
+      }
     }
 
     // If the just-crashed process is the system_server, bring down zygote
@@ -346,11 +361,6 @@ static void SigChldHandler(int /*signal_number*/) {
                             "Exit zygote because system server (pid %d) has terminated", pid);
       kill(getpid(), SIGKILL);
     }
-
-    // Check to see if the PID is in the USAP pool and remove it if it is.
-    if (RemoveUsapTableEntry(pid)) {
-      ++usaps_removed;
-    }
   }
 
   // Note that we shouldn't consider ECHILD an error because
@@ -1648,7 +1658,13 @@ static void com_android_internal_os_Zygote_nativeEmptyUsapPool(JNIEnv* env, jcla
     auto entry_storage = entry.GetValues();
 
     if (entry_storage.has_value()) {
-      kill(entry_storage.value().pid, SIGKILL);
+      kill(entry_storage.value().pid, SIGTERM);
+
+      // Clean up the USAP table entry here.  This avoids a potential race
+      // where a newly created USAP might not be able to find a valid table
+      // entry if signal handler (which would normally do the cleanup) doesn't
+      // run between now and when the new process is created.
+
       close(entry_storage.value().read_pipe_fd);
 
       // Avoid a second atomic load by invalidating instead of clearing.
@@ -1678,6 +1694,16 @@ static jboolean com_android_internal_os_Zygote_nativeDisableExecuteOnly(JNIEnv*
   return dl_iterate_phdr(disable_execute_only, nullptr) == 0;
 }
 
+static void com_android_internal_os_Zygote_nativeBlockSigTerm(JNIEnv* env, jclass) {
+  auto fail_fn = std::bind(ZygoteFailure, env, "usap", nullptr, _1);
+  BlockSignal(SIGTERM, fail_fn);
+}
+
+static void com_android_internal_os_Zygote_nativeUnblockSigTerm(JNIEnv* env, jclass) {
+  auto fail_fn = std::bind(ZygoteFailure, env, "usap", nullptr, _1);
+  UnblockSignal(SIGTERM, fail_fn);
+}
+
 static const JNINativeMethod gMethods[] = {
     { "nativeForkAndSpecialize",
       "(II[II[[IILjava/lang/String;Ljava/lang/String;[I[IZLjava/lang/String;Ljava/lang/String;)I",
@@ -1708,7 +1734,11 @@ static const JNINativeMethod gMethods[] = {
     { "nativeEmptyUsapPool", "()V",
       (void *) com_android_internal_os_Zygote_nativeEmptyUsapPool },
     { "nativeDisableExecuteOnly", "()Z",
-      (void *) com_android_internal_os_Zygote_nativeDisableExecuteOnly }
+      (void *) com_android_internal_os_Zygote_nativeDisableExecuteOnly },
+    { "nativeBlockSigTerm", "()V",
+      (void* ) com_android_internal_os_Zygote_nativeBlockSigTerm },
+    { "nativeUnblockSigTerm", "()V",
+      (void* ) com_android_internal_os_Zygote_nativeUnblockSigTerm }
 };
 
 int register_com_android_internal_os_Zygote(JNIEnv* env) {