Merge "Fixes two data races in USAP pool management." into qt-dev am: 5e468cf4dd (2194be51) · Commits · e / os / android_frameworks_base

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

+91 −59

Original line number	Original line	Diff line number	Diff line
	@@ -519,6 +519,9 @@ public final class Zygote {
	try {		try {
	sessionSocket = usapPoolSocket.accept();		sessionSocket = usapPoolSocket.accept();

			// Block SIGTERM so we won't be killed if the Zygote flushes the USAP pool.
			blockSigTerm();

	BufferedReader usapReader =		BufferedReader usapReader =
	new BufferedReader(new InputStreamReader(sessionSocket.getInputStream()));		new BufferedReader(new InputStreamReader(sessionSocket.getInputStream()));
	usapOutputStream =		usapOutputStream =
	@@ -537,13 +540,23 @@ public final class Zygote {
	} else {		} else {
	Log.e("USAP", "Truncated command received.");		Log.e("USAP", "Truncated command received.");
	IoUtils.closeQuietly(sessionSocket);		IoUtils.closeQuietly(sessionSocket);

			// Re-enable SIGTERM so the USAP can be flushed from the pool if necessary.
			unblockSigTerm();
	}		}
	} catch (Exception ex) {		} catch (Exception ex) {
	Log.e("USAP", ex.getMessage());		Log.e("USAP", ex.getMessage());
	IoUtils.closeQuietly(sessionSocket);		IoUtils.closeQuietly(sessionSocket);

			// Re-enable SIGTERM so the USAP can be flushed from the pool if necessary.
			unblockSigTerm();
	}		}
	}		}

			try {
			// SIGTERM is blocked on loop exit. This prevents a USAP that is specializing from
			// being killed during a pool flush.

	applyUidSecurityPolicy(args, peerCredentials);		applyUidSecurityPolicy(args, peerCredentials);
	applyDebuggerSystemProperty(args);		applyDebuggerSystemProperty(args);

	@@ -560,21 +573,24 @@ public final class Zygote {
	// Process.ProcessStartResult object.		// Process.ProcessStartResult object.
	usapOutputStream.writeInt(pid);		usapOutputStream.writeInt(pid);
	} catch (IOException ioEx) {		} catch (IOException ioEx) {
	Log.e("USAP", "Failed to write response to session socket: " + ioEx.getMessage());		Log.e("USAP", "Failed to write response to session socket: "
	System.exit(-1);		+ ioEx.getMessage());
			throw new RuntimeException(ioEx);
	} finally {		} finally {
	IoUtils.closeQuietly(sessionSocket);		IoUtils.closeQuietly(sessionSocket);

	try {		try {
	// This socket is closed using Os.close due to an issue with the implementation of		// This socket is closed using Os.close due to an issue with the implementation
	// LocalSocketImp.close. Because the raw FD is created by init and then loaded from		// of LocalSocketImp.close(). Because the raw FD is created by init and then
	// an environment variable (as opposed to being created by the LocalSocketImpl		// loaded from an environment variable (as opposed to being created by the
	// itself) the current implementation will not actually close the underlying FD.		// LocalSocketImpl itself) the current implementation will not actually close
			// the underlying FD.
	//		//
	// See b/130309968 for discussion of this issue.		// See b/130309968 for discussion of this issue.
	Os.close(usapPoolSocket.getFileDescriptor());		Os.close(usapPoolSocket.getFileDescriptor());
	} catch (ErrnoException ex) {		} catch (ErrnoException ex) {
	Log.e("USAP", "Failed to close USAP pool socket: " + ex.getMessage());		Log.e("USAP", "Failed to close USAP pool socket");
			throw new RuntimeException(ex);
	}		}
	}		}

	@@ -584,8 +600,8 @@ public final class Zygote {
	DataOutputStream outputStream = new DataOutputStream(buffer);		DataOutputStream outputStream = new DataOutputStream(buffer);

	// This is written as a long so that the USAP reporting pipe and USAP pool event FD		// This is written as a long so that the USAP reporting pipe and USAP pool event FD
	// handlers in ZygoteServer.runSelectLoop can be unified. These two cases should both		// handlers in ZygoteServer.runSelectLoop can be unified. These two cases should
	// send/receive 8 bytes.		// both send/receive 8 bytes.
	outputStream.writeLong(pid);		outputStream.writeLong(pid);
	outputStream.flush();		outputStream.flush();

	@@ -594,7 +610,7 @@ public final class Zygote {
	Log.e("USAP",		Log.e("USAP",
	String.format("Failed to write PID (%d) to pipe (%d): %s",		String.format("Failed to write PID (%d) to pipe (%d): %s",
	pid, writePipe.getInt$(), ex.getMessage()));		pid, writePipe.getInt$(), ex.getMessage()));
	System.exit(-1);		throw new RuntimeException(ex);
	} finally {		} finally {
	IoUtils.closeQuietly(writePipe);		IoUtils.closeQuietly(writePipe);
	}		}
	@@ -616,8 +632,24 @@ public final class Zygote {
	return ZygoteInit.zygoteInit(args.mTargetSdkVersion,		return ZygoteInit.zygoteInit(args.mTargetSdkVersion,
	args.mRemainingArgs,		args.mRemainingArgs,
	null /* classLoader */);		null /* classLoader */);
			} finally {
			// Unblock SIGTERM to restore the process to default behavior.
			unblockSigTerm();
			}
	}		}

			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: ";		private static final String USAP_ERROR_PREFIX = "Invalid command to USAP: ";

	/**		/**

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

+3 −0

Original line number	Original line	Diff line number	Diff line
	@@ -338,6 +338,7 @@ class ZygoteConnection {
	Runnable stateChangeCode) {		Runnable stateChangeCode) {
	try {		try {
	if (zygoteServer.isUsapPoolEnabled()) {		if (zygoteServer.isUsapPoolEnabled()) {
			Log.i(TAG, "Emptying USAP Pool due to state change.");
	Zygote.emptyUsapPool();		Zygote.emptyUsapPool();
	}		}

	@@ -351,6 +352,8 @@ class ZygoteConnection {
	if (fpResult != null) {		if (fpResult != null) {
	zygoteServer.setForkChild();		zygoteServer.setForkChild();
	return fpResult;		return fpResult;
			} else {
			Log.i(TAG, "Finished refilling USAP Pool after state change.");
	}		}
	}		}

core/jni/com_android_internal_os_Zygote.cpp

+41 −11

Original line number	Original line	Diff line number	Diff line
	@@ -198,7 +198,7 @@ class UsapTableEntry {
	* PIDs don't match nothing will happen.		* PIDs don't match nothing will happen.
	*		*
	* @param pid The ID of the process who's entry we want to clear.		* @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) {		bool ClearForPID(int32_t pid) {
	EntryStorage storage = mStorage.load();		EntryStorage storage = mStorage.load();
	@@ -212,14 +212,16 @@ class UsapTableEntry {
	* 3) It fails and the new value isn't INVALID_ENTRY_VALUE, in which		* 3) It fails and the new value isn't INVALID_ENTRY_VALUE, in which
	* case the entry has already been cleared and re-used.		* 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)) {		if (mStorage.compare_exchange_strong(storage, INVALID_ENTRY_VALUE)) {
	close(storage.read_pipe_fd);		close(storage.read_pipe_fd);
			return true;
			} else {
			return false;
	}		}

	return true;
	} else {		} else {
	return false;		return false;
	}		}
	@@ -331,11 +333,24 @@ static void SigChldHandler(int /signal_number/) {
	if (WIFEXITED(status)) {		if (WIFEXITED(status)) {
	async_safe_format_log(ANDROID_LOG_INFO, LOG_TAG,		async_safe_format_log(ANDROID_LOG_INFO, LOG_TAG,
	"Process %d exited cleanly (%d)", pid, WEXITSTATUS(status));		"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)) {		} else if (WIFSIGNALED(status)) {
	async_safe_format_log(ANDROID_LOG_INFO, LOG_TAG,		async_safe_format_log(ANDROID_LOG_INFO, LOG_TAG,
	"Process %d exited due to signal %d (%s)%s", pid,		"Process %d exited due to signal %d (%s)%s", pid,
	WTERMSIG(status), strsignal(WTERMSIG(status)),		WTERMSIG(status), strsignal(WTERMSIG(status)),
	WCOREDUMP(status) ? "; core dumped" : "");		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		// 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);		"Exit zygote because system server (pid %d) has terminated", pid);
	kill(getpid(), SIGKILL);		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		// 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();		auto entry_storage = entry.GetValues();

	if (entry_storage.has_value()) {		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);		close(entry_storage.value().read_pipe_fd);

	// Avoid a second atomic load by invalidating instead of clearing.		// 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;		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[] = {		static const JNINativeMethod gMethods[] = {
	{ "nativeForkAndSpecialize",		{ "nativeForkAndSpecialize",
	"(II[II[[IILjava/lang/String;Ljava/lang/String;[I[IZLjava/lang/String;Ljava/lang/String;)I",		"(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",		{ "nativeEmptyUsapPool", "()V",
	(void *) com_android_internal_os_Zygote_nativeEmptyUsapPool },		(void *) com_android_internal_os_Zygote_nativeEmptyUsapPool },
	{ "nativeDisableExecuteOnly", "()Z",		{ "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) {		int register_com_android_internal_os_Zygote(JNIEnv* env) {