cgpt: Use subprocess_run to call flashrom with 1>/dev/nullstabilize-quickfix-13904.98.Bstabilize-13904.67.Bstabilize-13904.66.Bstabilize-13904.62.Bstabilize-13904.59.Bstabilize-13904.58.Bstabilize-13904.55.Bstabilize-13904.49.Bstabilize-13904.48.Bstabilize-13904.47.Bstabilize-13904.44.Bstabilize-13904.43.Bstabilize-13904.42.Bstabilize-13904.41.Bstabilize-13904.34.Brelease-R91-13904.B

Closing fd=1 is a bad idea, as flashrom will then reuse fd=1 for
the MTD device and directly write to it.

Luckily, we have a subprocess_run function in vboot that does
what we need. There will be some cleanup required after this,
but hopefully this is enough to clear the P0 on hand.

BUG=b:184559695
TEST=`cgpt find -t kernel` (fails?!), but at least does not
 corrupt flash:
 `flashrom -r /usr/local/x.bin && hexdump -C /usr/local/x.bin | head`
BRANCH=none

Change-Id: Ia82ed7966ea66274f72fe21eca5241633ffbdb5c
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2812630
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Jack Rosenthal <jrosenth@chromium.org>
Commit-Queue: Jack Rosenthal <jrosenth@chromium.org>

1 files changed, 56 insertions, 19 deletions

diff --git a/cgpt/cgpt_nor.c b/cgpt/cgpt_nor.c
index fd184463..40306676 100644
--- a/cgpt/cgpt_nor.c
+++ b/cgpt/cgpt_nor.c
@@ -24,6 +24,7 @@
 
 #include "cgpt.h"
 #include "cgpt_nor.h"
+#include "subprocess.h"
 
 static const char FLASHROM_PATH[] = "/usr/sbin/flashrom";
 
@@ -48,6 +49,7 @@ int GetMtdSize(const char *mtd_device, uint64_t *size) {
   return ret;
 }
 
+// TODO(b:184559695): Remove these functions and use subprocess_run everywhere.
 int ForkExecV(const char *cwd, const char *const argv[]) {
   pid_t pid = fork();
   if (pid == -1) {
@@ -212,27 +214,42 @@ int ReadNorFlash(char *temp_dir_template) {
 
   // Read RW_GPT section from NOR flash to "rw_gpt".
   ret++;
-  int fd_flags = fcntl(1, F_GETFD);
-  // Close stdout on exec so that flashrom does not muck up cgpt's output.
-  if (0 != fcntl(1, F_SETFD, FD_CLOEXEC))
-    Warning("Can't stop flashrom from mucking up our output\n");
-  if (ForkExecL(temp_dir_template, FLASHROM_PATH, "-i", "RW_GPT:rw_gpt", "-r",
-                NULL) != 0) {
+
+  // TODO(b:184559695): Add parameter to subprocess_run to change directory
+  // before exec. Also, NULL parameter is a glibc extension that _might_
+  // break FreeBSD.
+  char *cwd = getcwd(NULL, 0);
+  if (!cwd) {
+    Error("Cannot get current directory.\n");
+    return ret;
+  }
+  if (chdir(temp_dir_template) < 0) {
+    Error("Cannot change directory.\n");
+    goto out_free;
+  }
+  const char *const argv[] = {FLASHROM_PATH, "-i", "RW_GPT:rw_gpt", "-r"};
+  // Redirect stdout to /dev/null so that flashrom does not muck up cgpt's
+  // output.
+  if (subprocess_run(argv, &subprocess_null, &subprocess_null, NULL) != 0) {
     Error("Cannot exec flashrom to read from RW_GPT section.\n");
     RemoveDir(temp_dir_template);
   } else {
     ret = 0;
   }
+  if (chdir(cwd) < 0) {
+    Error("Cannot change directory back to original.\n");
+    goto out_free;
+  }
 
-  // Restore stdout flags
-  if (0 != fcntl(1, F_SETFD, fd_flags))
-    Warning("Can't restore stdout flags\n");
+out_free:
+  free(cwd);
   return ret;
 }
 
 // Write "rw_gpt" back to NOR flash. We write the file in two parts for safety.
 int WriteNorFlash(const char *dir) {
   int ret = 0;
+
   ret++;
   if (split_gpt(dir, "rw_gpt") != 0) {
     Error("Cannot split rw_gpt in two.\n");
@@ -240,26 +257,46 @@ int WriteNorFlash(const char *dir) {
   }
   ret++;
   int nr_fails = 0;
-  int fd_flags = fcntl(1, F_GETFD);
-  // Close stdout on exec so that flashrom does not muck up cgpt's output.
-  if (0 != fcntl(1, F_SETFD, FD_CLOEXEC))
-    Warning("Can't stop flashrom from mucking up our output\n");
-  if (ForkExecL(dir, FLASHROM_PATH, "-i", "RW_GPT_PRIMARY:rw_gpt_1",
-                "-w", "--fast-verify", NULL) != 0) {
+
+  // TODO(b:184559695): Add parameter to subprocess_run to change directory
+  // before exec. Also, NULL parameter is a glibc extension that _might_
+  // break FreeBSD.
+  char *cwd = getcwd(NULL, 0);
+  if (!cwd) {
+    Error("Cannot get current directory.\n");
+    return ret;
+  }
+  if (chdir(dir) < 0) {
+    Error("Cannot change directory.\n");
+    goto out_free;
+  }
+  const char *const argv1[] = {FLASHROM_PATH, "-i", "RW_GPT_PRIMARY:rw_gpt_1",
+                "-w", "--fast-verify"};
+  // Redirect stdout to /dev/null so that flashrom does not muck up cgpt's
+  // output.
+  if (subprocess_run(argv1, &subprocess_null, &subprocess_null, NULL) != 0) {
     Warning("Cannot write the 1st half of rw_gpt back with flashrom.\n");
     nr_fails++;
   }
-  if (ForkExecL(dir, FLASHROM_PATH, "-i", "RW_GPT_SECONDARY:rw_gpt_2",
-                "-w", "--fast-verify", NULL) != 0) {
+  const char *const argv2[] = {FLASHROM_PATH, "-i", "RW_GPT_SECONDARY:rw_gpt_2",
+                "-w", "--fast-verify"};
+  // Redirect stdout to /dev/null so that flashrom does not muck up cgpt's
+  // output.
+  if (subprocess_run(argv2, &subprocess_null, &subprocess_null, NULL) != 0) {
     Warning("Cannot write the 2nd half of rw_gpt back with flashrom.\n");
     nr_fails++;
   }
-  if (0 != fcntl(1, F_SETFD, fd_flags))
-    Warning("Can't restore stdout flags\n");
+  if (chdir(cwd) < 0) {
+    Error("Cannot change directory back to original.\n");
+    goto out_free;
+  }
   switch (nr_fails) {
     case 0: ret = 0; break;
     case 1: Warning("It might still be okay.\n"); break;
     case 2: Error("Cannot write both parts back with flashrom.\n"); break;
   }
+
+out_free:
+  free(cwd);
   return ret;
 }