Merge "Add a timeout for all installd operations." am: 9fc78a40b9 (bd236aa3) · Commits · e / os / android_frameworks_native

cmds/installd/dexopt.cpp

+69 −43

Original line number	Diff line number	Diff line
		@@ -74,10 +74,19 @@ using android::base::ReadFdToString;
		using android::base::ReadFully;
		using android::base::StringPrintf;
		using android::base::WriteFully;
		using android::base::borrowed_fd;
		using android::base::unique_fd;

		namespace {

		// Timeout for short operations, such as merging profiles.
		constexpr int kShortTimeoutMs = 60000; // 1 minute.

		// Timeout for long operations, such as compilation. This should be smaller than the Package Manager
		// watchdog (PackageManagerService.WATCHDOG_TIMEOUT, 10 minutes), so that the operation will be
		// aborted before that watchdog would take down the system server.
		constexpr int kLongTimeoutMs = 570000; // 9.5 minutes.

		class DexOptStatus {
		public:
		// Check if dexopt is cancelled and fork if it is not cancelled.
		@@ -486,6 +495,25 @@ static void open_profile_files(uid_t uid, const std::string& package_name,
		}
		}

		// Cleans up an output file specified by a file descriptor. This function should be called whenever
		// a subprocess that modifies a system-managed file crashes.
		// If the subprocess crashes while it's writing to the file, the file is likely corrupted, so we
		// should remove it.
		// If the subprocess times out and is killed while it's acquiring a flock on the file, there is
		// probably a deadlock, so it's also good to remove the file so that later operations won't
		// encounter the same problem. It's safe to do so because the process that is holding the flock will
		// still have access to the file until the file descriptor is closed.
		// Note that we can't do `clear_reference_profile` here even if the fd points to a reference profile
		// because that also requires a flock and is therefore likely to be stuck in the second case.
		static bool cleanup_output_fd(int fd) {
		std::string path;
		bool ret = remove_file_at_fd(fd, &path);
		if (ret) {
		LOG(INFO) << "Removed file at path " << path;
		}
		return ret;
		}

		static constexpr int PROFMAN_BIN_RETURN_CODE_SUCCESS = 0;
		static constexpr int PROFMAN_BIN_RETURN_CODE_COMPILE = 1;
		static constexpr int PROFMAN_BIN_RETURN_CODE_SKIP_COMPILATION_NOT_ENOUGH_DELTA = 2;
		@@ -497,9 +525,10 @@ static constexpr int PROFMAN_BIN_RETURN_CODE_SKIP_COMPILATION_EMPTY_PROFILES = 7

		class RunProfman : public ExecVHelper {
		public:
		void SetupArgs(const std::vector<unique_fd>& profile_fds,
		template <typename T, typename U>
		void SetupArgs(const std::vector<T>& profile_fds,
		const unique_fd& reference_profile_fd,
		const std::vector<unique_fd>& apk_fds,
		const std::vector<U>& apk_fds,
		const std::vector<std::string>& dex_locations,
		bool copy_and_update,
		bool for_snapshot,
		@@ -519,11 +548,11 @@ class RunProfman : public ExecVHelper {
		AddArg("--reference-profile-file-fd=" + std::to_string(reference_profile_fd.get()));
		}

		for (const unique_fd& fd : profile_fds) {
		for (const T& fd : profile_fds) {
		AddArg("--profile-file-fd=" + std::to_string(fd.get()));
		}

		for (const unique_fd& fd : apk_fds) {
		for (const U& fd : apk_fds) {
		AddArg("--apk-fd=" + std::to_string(fd.get()));
		}

		@@ -582,20 +611,14 @@ class RunProfman : public ExecVHelper {
		for_boot_image);
		}

		void SetupCopyAndUpdate(unique_fd&& profile_fd,
		unique_fd&& reference_profile_fd,
		unique_fd&& apk_fd,
		void SetupCopyAndUpdate(const unique_fd& profile_fd,
		const unique_fd& reference_profile_fd,
		const unique_fd& apk_fd,
		const std::string& dex_location) {
		// The fds need to stay open longer than the scope of the function, so put them into a local
		// variable vector.
		profiles_fd_.push_back(std::move(profile_fd));
		apk_fds_.push_back(std::move(apk_fd));
		reference_profile_fd_ = std::move(reference_profile_fd);
		std::vector<std::string> dex_locations = {dex_location};
		SetupArgs(profiles_fd_,
		reference_profile_fd_,
		apk_fds_,
		dex_locations,
		SetupArgs(std::vector<borrowed_fd>{profile_fd},
		reference_profile_fd,
		std::vector<borrowed_fd>{apk_fd},
		{dex_location},
		/copy_and_update=/true,
		/for_snapshot/false,
		/for_boot_image/false);
		@@ -621,11 +644,6 @@ class RunProfman : public ExecVHelper {
		void Exec() {
		ExecVHelper::Exec(DexoptReturnCodes::kProfmanExec);
		}

		private:
		unique_fd reference_profile_fd_;
		std::vector<unique_fd> profiles_fd_;
		std::vector<unique_fd> apk_fds_;
		};

		static int analyze_profiles(uid_t uid, const std::string& package_name,
		@@ -657,13 +675,14 @@ static int analyze_profiles(uid_t uid, const std::string& package_name,
		profman_merge.Exec();
		}
		/* parent */
		int return_code = wait_child(pid);
		int return_code = wait_child_with_timeout(pid, kShortTimeoutMs);
		bool need_to_compile = false;
		bool empty_profiles = false;
		bool should_clear_current_profiles = false;
		bool should_clear_reference_profile = false;
		if (!WIFEXITED(return_code)) {
		LOG(WARNING) << "profman failed for location " << location << ": " << return_code;
		cleanup_output_fd(reference_profile_fd.get());
		} else {
		return_code = WEXITSTATUS(return_code);
		switch (return_code) {
		@@ -797,10 +816,10 @@ bool dump_profiles(int32_t uid, const std::string& pkgname, const std::string& p
		profman_dump.Exec();
		}
		/* parent */
		int return_code = wait_child(pid);
		int return_code = wait_child_with_timeout(pid, kShortTimeoutMs);
		if (!WIFEXITED(return_code)) {
		LOG(WARNING) << "profman failed for package " << pkgname << ": "
		<< return_code;
		LOG(WARNING) << "profman failed for package " << pkgname << ": " << return_code;
		cleanup_output_fd(output_fd.get());
		return false;
		}
		return true;
		@@ -871,7 +890,11 @@ bool copy_system_profile(const std::string& system_profile,
		_exit(0);
		}
		/* parent */
		int return_code = wait_child(pid);
		int return_code = wait_child_with_timeout(pid, kShortTimeoutMs);
		if (!WIFEXITED(return_code)) {
		cleanup_output_fd(out_fd.get());
		return false;
		}
		return return_code == 0;
		}

		@@ -1521,7 +1544,7 @@ static bool get_class_loader_context_dex_paths(const char* class_loader_context,
		}
		pipe_read.reset();

		int return_code = wait_child(pid);
		int return_code = wait_child_with_timeout(pid, kShortTimeoutMs);
		if (!WIFEXITED(return_code)) {
		PLOG(ERROR) << "Error waiting for child dexoptanalyzer process";
		return false;
		@@ -1695,7 +1718,7 @@ static SecondaryDexOptProcessResult process_secondary_dex_dexopt(const std::stri
		}

		/* parent */
		int result = wait_child(pid);
		int result = wait_child_with_timeout(pid, kShortTimeoutMs);
		cancelled = dexopt_status_->check_if_killed_and_remove_dexopt_pid(pid);
		if (!WIFEXITED(result)) {
		if ((WTERMSIG(result) == SIGKILL) && cancelled) {
		@@ -1954,7 +1977,7 @@ int dexopt(const char* dex_path, uid_t uid, const char* pkgname, const char* ins

		runner.Exec(DexoptReturnCodes::kDex2oatExec);
		} else {
		int res = wait_child(pid);
		int res = wait_child_with_timeout(pid, kLongTimeoutMs);
		bool cancelled = dexopt_status_->check_if_killed_and_remove_dexopt_pid(pid);
		if (res == 0) {
		LOG(VERBOSE) << "DexInv: --- END '" << dex_path << "' (success) ---";
		@@ -2143,7 +2166,7 @@ bool reconcile_secondary_dex_file(const std::string& dex_path,
		_exit(result ? kReconcileSecondaryDexCleanedUp : kReconcileSecondaryDexAccessIOError);
		}

		int return_code = wait_child(pid);
		int return_code = wait_child_with_timeout(pid, kShortTimeoutMs);
		if (!WIFEXITED(return_code)) {
		LOG(WARNING) << "reconcile dex failed for location " << dex_path << ": " << return_code;
		} else {
		@@ -2261,7 +2284,7 @@ bool hash_secondary_dex_file(const std::string& dex_path, const std::string& pkg
		if (!ReadFully(pipe_read, out_secondary_dex_hash->data(), out_secondary_dex_hash->size())) {
		out_secondary_dex_hash->clear();
		}
		return wait_child(pid) == 0;
		return wait_child_with_timeout(pid, kShortTimeoutMs) == 0;
		}

		// Helper for move_ab, so that we can have common failure-case cleanup.
		@@ -2591,9 +2614,10 @@ static bool create_app_profile_snapshot(int32_t app_id,
		}

		/* parent */
		int return_code = wait_child(pid);
		int return_code = wait_child_with_timeout(pid, kShortTimeoutMs);
		if (!WIFEXITED(return_code)) {
		LOG(WARNING) << "profman failed for " << package_name << ":" << profile_name;
		cleanup_output_fd(snapshot_fd.get());
		return false;
		}

		@@ -2700,10 +2724,11 @@ static bool create_boot_image_profile_snapshot(const std::string& package_name,
		}

		/* parent */
		int return_code = wait_child(pid);
		int return_code = wait_child_with_timeout(pid, kShortTimeoutMs);

		if (!WIFEXITED(return_code)) {
		PLOG(WARNING) << "profman failed for " << package_name << ":" << profile_name;
		cleanup_output_fd(snapshot_fd.get());
		return false;
		}

		@@ -2774,9 +2799,9 @@ bool prepare_app_profile(const std::string& package_name,
		}

		RunProfman args;
		args.SetupCopyAndUpdate(std::move(dex_metadata_fd),
		std::move(ref_profile_fd),
		std::move(apk_fd),
		args.SetupCopyAndUpdate(dex_metadata_fd,
		ref_profile_fd,
		apk_fd,
		code_path);
		pid_t pid = fork();
		if (pid == 0) {
		@@ -2789,9 +2814,10 @@ bool prepare_app_profile(const std::string& package_name,
		}

		/* parent */
		int return_code = wait_child(pid);
		int return_code = wait_child_with_timeout(pid, kShortTimeoutMs);
		if (!WIFEXITED(return_code)) {
		PLOG(WARNING) << "profman failed for " << package_name << ":" << profile_name;
		cleanup_output_fd(ref_profile_fd.get());
		return false;
		}
		return true;

cmds/installd/tests/installd_dexopt_test.cpp

+4 −2

Original line number	Diff line number	Diff line
		@@ -50,6 +50,8 @@ using android::base::unique_fd;
		namespace android {
		namespace installd {

		constexpr int kTimeoutMs = 60000;

		// TODO(calin): try to dedup this code.
		#if defined(__arm__)
		static const std::string kRuntimeIsa = "arm";
		@@ -1072,7 +1074,7 @@ class ProfileTest : public DexoptTest {
		_exit(0);
		}
		/* parent */
		ASSERT_TRUE(WIFEXITED(wait_child(pid)));
		ASSERT_TRUE(WIFEXITED(wait_child_with_timeout(pid, kTimeoutMs)));
		}

		void mergePackageProfiles(const std::string& package_name,
		@@ -1376,7 +1378,7 @@ class BootProfileTest : public ProfileTest {
		_exit(0);
		}
		/* parent */
		ASSERT_TRUE(WIFEXITED(wait_child(pid)));
		ASSERT_TRUE(WIFEXITED(wait_child_with_timeout(pid, kTimeoutMs)));
		}
		protected:
		std::string intial_android_profiles_dir;

cmds/installd/tests/installd_utils_test.cpp

+42 −0

Original line number	Diff line number	Diff line
		@@ -14,8 +14,10 @@
		* limitations under the License.
		*/

		#include <errno.h>
		#include <stdlib.h>
		#include <string.h>
		#include <unistd.h>

		#include <android-base/logging.h>
		#include <android-base/scopeguard.h>
		@@ -656,5 +658,45 @@ TEST_F(UtilsTest, TestCreateDirIfNeeded) {
		ASSERT_NE(0, create_dir_if_needed("/data/local/tmp/user/0/bar/baz", 0700));
		}

		TEST_F(UtilsTest, WaitChild) {
		pid_t pid = fork();
		if (pid == 0) {
		/* child */
		// Do nothing.
		_exit(0);
		}
		/* parent */
		int return_code = wait_child_with_timeout(pid, /timeout_ms=/100);
		EXPECT_TRUE(WIFEXITED(return_code));
		EXPECT_EQ(WEXITSTATUS(return_code), 0);
		}

		TEST_F(UtilsTest, WaitChildTimeout) {
		pid_t pid = fork();
		if (pid == 0) {
		/* child */
		sleep(1);
		_exit(0);
		}
		/* parent */
		int return_code = wait_child_with_timeout(pid, /timeout_ms=/1);
		EXPECT_FALSE(WIFEXITED(return_code));
		EXPECT_EQ(WTERMSIG(return_code), SIGKILL);
		}

		TEST_F(UtilsTest, RemoveFileAtFd) {
		std::string filename = "/data/local/tmp/tempfile-XXXXXX";
		int fd = mkstemp(filename.data());
		ASSERT_GE(fd, 0);
		ASSERT_EQ(access(filename.c_str(), F_OK), 0);

		std::string actual_filename;
		remove_file_at_fd(fd, &actual_filename);
		EXPECT_NE(access(filename.c_str(), F_OK), 0);
		EXPECT_EQ(filename, actual_filename);

		close(fd);
		}

		} // namespace installd
		} // namespace android

cmds/installd/utils.cpp

+59 −19

Original line number	Diff line number	Diff line
		@@ -19,18 +19,21 @@
		#include <errno.h>
		#include <fcntl.h>
		#include <fts.h>
		#include <poll.h>
		#include <stdlib.h>
		#include <sys/capability.h>
		#include <sys/pidfd.h>
		#include <sys/stat.h>
		#include <sys/statvfs.h>
		#include <sys/wait.h>
		#include <sys/xattr.h>
		#include <unistd.h>
		#include <uuid/uuid.h>

		#include <android-base/file.h>
		#include <android-base/logging.h>
		#include <android-base/strings.h>
		#include <android-base/stringprintf.h>
		#include <android-base/strings.h>
		#include <android-base/unique_fd.h>
		#include <cutils/fs.h>
		#include <cutils/properties.h>
		@@ -1059,30 +1062,45 @@ int ensure_config_user_dirs(userid_t userid) {
		return fs_prepare_dir(path.c_str(), 0750, uid, gid);
		}

		int wait_child(pid_t pid)
		{
		static int wait_child(pid_t pid) {
		int status;
		pid_t got_pid;
		pid_t got_pid = TEMP_FAILURE_RETRY(waitpid(pid, &status, /options=/0));

		while (1) {
		got_pid = waitpid(pid, &status, 0);
		if (got_pid == -1 && errno == EINTR) {
		printf("waitpid interrupted, retrying\n");
		} else {
		break;
		if (got_pid != pid) {
		PLOG(ERROR) << "waitpid failed: wanted " << pid << ", got " << got_pid;
		return W_EXITCODE(/exit_code=/255, /signal_number=/0);
		}

		return status;
		}
		if (got_pid != pid) {
		ALOGW("waitpid failed: wanted %d, got %d: %s\n",
		(int) pid, (int) got_pid, strerror(errno));
		return 1;

		int wait_child_with_timeout(pid_t pid, int timeout_ms) {
		int pidfd = pidfd_open(pid, /flags=/0);
		if (pidfd < 0) {
		PLOG(ERROR) << "pidfd_open failed for pid " << pid;
		kill(pid, SIGKILL);
		return wait_child(pid);
		}

		if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
		return 0;
		} else {
		return status; /* always nonzero */
		struct pollfd pfd;
		pfd.fd = pidfd;
		pfd.events = POLLIN;
		int poll_ret = TEMP_FAILURE_RETRY(poll(&pfd, /nfds=/1, timeout_ms));

		close(pidfd);

		if (poll_ret < 0) {
		PLOG(ERROR) << "poll failed for pid " << pid;
		kill(pid, SIGKILL);
		return wait_child(pid);
		}
		if (poll_ret == 0) {
		LOG(WARNING) << "Child process " << pid << " timed out after " << timeout_ms
		<< "ms. Killing it";
		kill(pid, SIGKILL);
		return wait_child(pid);
		}
		return wait_child(pid);
		}

		/**
		@@ -1295,5 +1313,27 @@ void drop_capabilities(uid_t uid) {
		}
		}

		bool remove_file_at_fd(int fd, /out/ std::string* path) {
		char path_buffer[PATH_MAX + 1];
		std::string proc_path = android::base::StringPrintf("/proc/self/fd/%d", fd);
		ssize_t len = readlink(proc_path.c_str(), path_buffer, PATH_MAX);
		if (len < 0) {
		PLOG(WARNING) << "Could not remove file at fd " << fd << ": Failed to get file path";
		return false;
		}
		path_buffer[len] = '\0';
		if (path != nullptr) {
		*path = path_buffer;
		}
		if (unlink(path_buffer) != 0) {
		if (errno == ENOENT) {
		return true;
		}
		PLOG(WARNING) << "Could not remove file at path " << path_buffer;
		return false;
		}
		return true;
		}

		} // namespace installd
		} // namespace android

cmds/installd/utils.h

+6 −1

Original line number	Diff line number	Diff line
		@@ -153,7 +153,8 @@ int validate_apk_path_subdirs(const char *path);

		int ensure_config_user_dirs(userid_t userid);

		int wait_child(pid_t pid);
		// Waits for a child process, or kills it if it times out. Returns the exit code.
		int wait_child_with_timeout(pid_t pid, int timeout_ms);

		int prepare_app_cache_dir(const std::string& parent, const char* name, mode_t target_mode,
		uid_t uid, gid_t gid);
		@@ -170,6 +171,10 @@ bool collect_profiles(std::vector<std::string>* profiles_paths);

		void drop_capabilities(uid_t uid);

		// Removes a file specified by a file descriptor. Returns true on success. Reports the file path to
		// `path` if present.
		bool remove_file_at_fd(int fd, /out/ std::string* path = nullptr);

		} // namespace installd
		} // namespace android