6 files changed, 65 insertions, 80 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<std::string> 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 <frac> <secs>
-  //            fill up the next <frac> part of of the progress bar
-  //            over <secs> seconds.  If <secs> is zero, use
-  //            set_progress commands to manually control the
-  //            progress of this segment of the bar.
+  //   progress <frac> <secs>
+  //       fill up the next <frac> part of of the progress bar over <secs> seconds. If <secs> is
+  //       zero, use `set_progress` commands to manually control the progress of this segment of the
+  //       bar.
   //
-  //        set_progress <frac>
-  //            <frac> should be between 0.0 and 1.0; sets the
-  //            progress bar within the segment defined by the most
-  //            recent progress command.
+  //   set_progress <frac>
+  //       <frac> should be between 0.0 and 1.0; sets the progress bar within the segment defined by
+  //       the most recent progress command.
   //
-  //        ui_print <string>
-  //            display <string> on the screen.
+  //   ui_print <string>
+  //       display <string> 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 <string>
-  //            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 <string>
+  //       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<std::string> 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<bool> 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/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;
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<Device::BuiltinActi
 }
 
 static void redirect_stdio(const char* filename) {
-  int pipefd[2];
-  if (pipe(pipefd) == -1) {
-    PLOG(ERROR) << "pipe failed";
+  android::base::unique_fd pipe_read, pipe_write;
+  // Create a pipe that allows parent process sending logs over.
+  if (!android::base::Pipe(&pipe_read, &pipe_write)) {
+    PLOG(ERROR) << "Failed to create pipe for redirecting stdio";
 
     // Fall back to traditional logging mode without timestamps. If these fail, there's not really
     // anywhere to complain...
@@ -233,7 +234,7 @@ static void redirect_stdio(const char* filename) {
 
   pid_t pid = fork();
   if (pid == -1) {
-    PLOG(ERROR) << "fork failed";
+    PLOG(ERROR) << "Failed to fork for redirecting stdio";
 
     // Fall back to traditional logging mode without timestamps. If these fail, there's not really
     // anywhere to complain...
@@ -246,8 +247,8 @@ static void redirect_stdio(const char* filename) {
   }
 
   if (pid == 0) {
-    /// Close the unused write end.
-    close(pipefd[1]);
+    // Child process reads the incoming logs and doesn't write to the pipe.
+    pipe_write.reset();
 
     auto start = std::chrono::steady_clock::now();
 
@@ -255,15 +256,13 @@ static void redirect_stdio(const char* filename) {
     FILE* log_fp = fopen(filename, "ae");
     if (log_fp == nullptr) {
       PLOG(ERROR) << "fopen \"" << filename << "\" failed";
-      close(pipefd[0]);
       _exit(EXIT_FAILURE);
     }
 
-    FILE* pipe_fp = fdopen(pipefd[0], "r");
+    FILE* pipe_fp = android::base::Fdopen(std::move(pipe_read), "r");
     if (pipe_fp == nullptr) {
       PLOG(ERROR) << "fdopen failed";
       check_and_fclose(log_fp, filename);
-      close(pipefd[0]);
       _exit(EXIT_FAILURE);
     }
 
@@ -283,25 +282,23 @@ static void redirect_stdio(const char* filename) {
 
     PLOG(ERROR) << "getline failed";
 
+    fclose(pipe_fp);
     free(line);
     check_and_fclose(log_fp, filename);
-    close(pipefd[0]);
     _exit(EXIT_FAILURE);
   } else {
     // Redirect stdout/stderr to the logger process. Close the unused read end.
-    close(pipefd[0]);
+    pipe_read.reset();
 
     setbuf(stdout, nullptr);
     setbuf(stderr, nullptr);
 
-    if (dup2(pipefd[1], STDOUT_FILENO) == -1) {
+    if (dup2(pipe_write.get(), STDOUT_FILENO) == -1) {
       PLOG(ERROR) << "dup2 stdout failed";
     }
-    if (dup2(pipefd[1], STDERR_FILENO) == -1) {
+    if (dup2(pipe_write.get(), STDERR_FILENO) == -1) {
       PLOG(ERROR) << "dup2 stderr failed";
     }
-
-    close(pipefd[1]);
   }
 }