From 6fcd208db40c53fa933dee0b210363f439340ffa Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 16 Jan 2019 09:29:17 -0800 Subject: Use android::base::Pipe. Also clean up the comments regarding updater/recovery protocol. Test: Sideload a package on taimen. Check that /tmp/recovery.log contains the log from update_engine_sideload. Test: Factory reset on taimen. Check that log file has the info from mke2fs. Change-Id: If560269ce7007769fc85a63bd228813bdd75bebd --- install.cpp | 110 ++++++++++++++++++++++++------------------------------ recovery_main.cpp | 27 ++++++-------- 2 files changed, 61 insertions(+), 76 deletions(-) diff --git a/install.cpp b/install.cpp index 9d8943f7d..a7868fa1d 100644 --- a/install.cpp +++ b/install.cpp @@ -292,6 +292,11 @@ int SetUpNonAbUpdateCommands(const std::string& package, ZipArchiveHandle zip, i return INSTALL_ERROR; } + // When executing the update binary contained in the package, the arguments passed are: + // - the version number for this interface + // - an FD to which the program can write in order to update the progress bar. + // - the name of the package zip file. + // - an optional argument "retry" if this update is a retry of a failed update attempt. *cmd = { binary_path, std::to_string(kRecoveryApiVersion), @@ -334,75 +339,56 @@ static int try_update_binary(const std::string& package, ZipArchiveHandle zip, b ReadSourceTargetBuild(metadata, log_buffer); - int pipefd[2]; - pipe(pipefd); - std::vector args; - if (int update_status = - is_ab ? SetUpAbUpdateCommands(package, zip, pipefd[1], &args) - : SetUpNonAbUpdateCommands(package, zip, retry_count, pipefd[1], &args); - update_status != 0) { - close(pipefd[0]); - close(pipefd[1]); - log_buffer->push_back(android::base::StringPrintf("error: %d", kUpdateBinaryCommandFailure)); - return update_status; + // The updater in child process writes to the pipe to communicate with recovery. + android::base::unique_fd pipe_read, pipe_write; + if (!android::base::Pipe(&pipe_read, &pipe_write)) { + PLOG(ERROR) << "Failed to create pipe for updater-recovery communication"; + return INSTALL_CORRUPT; } - // When executing the update binary contained in the package, the - // arguments passed are: - // - // - the version number for this interface - // - // - an FD to which the program can write in order to update the - // progress bar. The program can write single-line commands: + // The updater-recovery communication protocol. // - // progress - // fill up the next part of of the progress bar - // over seconds. If is zero, use - // set_progress commands to manually control the - // progress of this segment of the bar. + // progress + // fill up the next part of of the progress bar over seconds. If is + // zero, use `set_progress` commands to manually control the progress of this segment of the + // bar. // - // set_progress - // should be between 0.0 and 1.0; sets the - // progress bar within the segment defined by the most - // recent progress command. + // set_progress + // should be between 0.0 and 1.0; sets the progress bar within the segment defined by + // the most recent progress command. // - // ui_print - // display on the screen. + // ui_print + // display on the screen. // - // wipe_cache - // a wipe of cache will be performed following a successful - // installation. + // wipe_cache + // a wipe of cache will be performed following a successful installation. // - // clear_display - // turn off the text display. + // clear_display + // turn off the text display. // - // enable_reboot - // packages can explicitly request that they want the user - // to be able to reboot during installation (useful for - // debugging packages that don't exit). + // enable_reboot + // packages can explicitly request that they want the user to be able to reboot during + // installation (useful for debugging packages that don't exit). // - // retry_update - // updater encounters some issue during the update. It requests - // a reboot to retry the same package automatically. + // retry_update + // updater encounters some issue during the update. It requests a reboot to retry the same + // package automatically. // - // log - // updater requests logging the string (e.g. cause of the - // failure). - // - // - the name of the package zip file. - // - // - an optional argument "retry" if this update is a retry of a failed - // update attempt. + // log + // updater requests logging the string (e.g. cause of the failure). // - // Convert the std::string vector to a NULL-terminated char* vector suitable for execv. - auto chr_args = StringVectorToNullTerminatedArray(args); + std::vector args; + if (int update_status = + is_ab ? SetUpAbUpdateCommands(package, zip, pipe_write.get(), &args) + : SetUpNonAbUpdateCommands(package, zip, retry_count, pipe_write.get(), &args); + update_status != 0) { + log_buffer->push_back(android::base::StringPrintf("error: %d", kUpdateBinaryCommandFailure)); + return update_status; + } pid_t pid = fork(); - if (pid == -1) { - close(pipefd[0]); - close(pipefd[1]); PLOG(ERROR) << "Failed to fork update binary"; log_buffer->push_back(android::base::StringPrintf("error: %d", kForkUpdateBinaryFailure)); return INSTALL_ERROR; @@ -410,16 +396,18 @@ static int try_update_binary(const std::string& package, ZipArchiveHandle zip, b if (pid == 0) { umask(022); - close(pipefd[0]); + pipe_read.reset(); + + // Convert the std::string vector to a NULL-terminated char* vector suitable for execv. + auto chr_args = StringVectorToNullTerminatedArray(args); execv(chr_args[0], chr_args.data()); - // Bug: 34769056 - // We shouldn't use LOG/PLOG in the forked process, since they may cause - // the child process to hang. This deadlock results from an improperly - // copied mutex in the ui functions. + // We shouldn't use LOG/PLOG in the forked process, since they may cause the child process to + // hang. This deadlock results from an improperly copied mutex in the ui functions. + // (Bug: 34769056) fprintf(stdout, "E:Can't run %s (%s)\n", chr_args[0], strerror(errno)); _exit(EXIT_FAILURE); } - close(pipefd[1]); + pipe_write.reset(); std::atomic logger_finished(false); std::thread temperature_logger(log_max_temperature, max_temperature, std::ref(logger_finished)); @@ -428,7 +416,7 @@ static int try_update_binary(const std::string& package, ZipArchiveHandle zip, b bool retry_update = false; char buffer[1024]; - FILE* from_child = fdopen(pipefd[0], "r"); + FILE* from_child = android::base::Fdopen(std::move(pipe_read), "r"); while (fgets(buffer, sizeof(buffer), from_child) != nullptr) { std::string line(buffer); size_t space = line.find_first_of(" \n"); diff --git a/recovery_main.cpp b/recovery_main.cpp index 7fb46163e..935d69815 100644 --- a/recovery_main.cpp +++ b/recovery_main.cpp @@ -217,9 +217,10 @@ static void ListenRecoverySocket(RecoveryUI* ui, std::atomic Date: Wed, 16 Jan 2019 09:29:17 -0800 Subject: Use O_CLOEXEC at a few places. Found these FDs unintentionally propagated into forked processes (e.g. updater) during my local debugging. Use O_CLOEXEC to avoid that. Test: Dump open FDs in updater process. Test: Sideload a package on taimen (which uses framebuffer). Test: Run graphics test on taimen. Change-Id: I1fb65dd4a103c47d28b15973def9e9359c251ca3 --- minui/events.cpp | 2 +- minui/graphics_adf.cpp | 2 +- minui/graphics_drm.cpp | 2 +- minui/graphics_fbdev.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/minui/events.cpp b/minui/events.cpp index d94e97723..30f8d50a2 100644 --- a/minui/events.cpp +++ b/minui/events.cpp @@ -66,7 +66,7 @@ int ev_init(ev_callback input_cb, bool allow_touch_inputs) { dirent* de; while ((de = readdir(dir))) { if (strncmp(de->d_name, "event", 5)) continue; - int fd = openat(dirfd(dir), de->d_name, O_RDONLY); + int fd = openat(dirfd(dir), de->d_name, O_RDONLY | O_CLOEXEC); if (fd == -1) continue; // Use unsigned long to match ioctl's parameter type. diff --git a/minui/graphics_adf.cpp b/minui/graphics_adf.cpp index 9eea497d6..10cd60709 100644 --- a/minui/graphics_adf.cpp +++ b/minui/graphics_adf.cpp @@ -101,7 +101,7 @@ int MinuiBackendAdf::DeviceInit(adf_device* dev) { err = adf_device_attach(dev, eng_id, intf_id); if (err < 0 && err != -EALREADY) return err; - intf_fd = adf_interface_open(dev, intf_id, O_RDWR); + intf_fd = adf_interface_open(dev, intf_id, O_RDWR | O_CLOEXEC); if (intf_fd < 0) return intf_fd; err = InterfaceInit(); diff --git a/minui/graphics_drm.cpp b/minui/graphics_drm.cpp index 765e2625a..7b2eed15d 100644 --- a/minui/graphics_drm.cpp +++ b/minui/graphics_drm.cpp @@ -285,7 +285,7 @@ GRSurface* MinuiBackendDrm::Init() { /* Consider DRM devices in order. */ for (int i = 0; i < DRM_MAX_MINOR; i++) { auto dev_name = android::base::StringPrintf(DRM_DEV_NAME, DRM_DIR_NAME, i); - android::base::unique_fd fd(open(dev_name.c_str(), O_RDWR)); + android::base::unique_fd fd(open(dev_name.c_str(), O_RDWR | O_CLOEXEC)); if (fd == -1) continue; /* We need dumb buffers. */ diff --git a/minui/graphics_fbdev.cpp b/minui/graphics_fbdev.cpp index 8d9c9741d..2584017d6 100644 --- a/minui/graphics_fbdev.cpp +++ b/minui/graphics_fbdev.cpp @@ -56,7 +56,7 @@ void MinuiBackendFbdev::SetDisplayedFramebuffer(size_t n) { } GRSurface* MinuiBackendFbdev::Init() { - android::base::unique_fd fd(open("/dev/graphics/fb0", O_RDWR)); + android::base::unique_fd fd(open("/dev/graphics/fb0", O_RDWR | O_CLOEXEC)); if (fd == -1) { perror("cannot open fb0"); return nullptr; -- cgit v1.2.3