From 602b433c5cd922f66d2d2846d44d6734614a0d28 Mon Sep 17 00:00:00 2001
From: Ed Schouten <eschouten@apple.com>
Date: Thu, 31 Aug 2023 04:23:32 -0700
Subject: [PATCH] build-runfiles: remove temporary file prior to creating it

When building with Remote Output Service, bb_clientd has the ability to restore snapshots of previous bazel-out/ directories. Though the file contents of these snapshots are identical to what's created in the past, the files will be read-only. This is because the files may be shared by multiple snapshots.

We have noticed that most of Bazel is fine with that. Most of the times Bazel is a good citizen, where it removes any files before recreating them. We did notice a very rare case where build-runfiles tries to make in-place modifications to a temporary file that it maintains. This change ensures that build-runfiles stops doing this.

Closes #19241.

PiperOrigin-RevId: 561615005
Change-Id: I8ca95c7d35df8a53af8f632b10b4a6141d180631
---
 src/main/tools/build-runfiles.cc            |  6 ++++
 src/test/shell/integration/runfiles_test.sh | 31 +++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/src/main/tools/build-runfiles.cc b/src/main/tools/build-runfiles.cc
index 9a2ef97991353a..21a6b5a7efdb32 100644
--- a/src/main/tools/build-runfiles.cc
+++ b/src/main/tools/build-runfiles.cc
@@ -117,6 +117,12 @@ class RunfilesCreator {
 
   void ReadManifest(const std::string &manifest_file, bool allow_relative,
                     bool use_metadata) {
+    // Remove file left over from previous invocation. This ensures that
+    // opening succeeds if the existing file is read-only.
+    if (unlink(temp_filename_.c_str()) != 0 && errno != ENOENT) {
+      PDIE("removing temporary file at '%s/%s'", output_base_.c_str(),
+           temp_filename_.c_str());
+    }
     FILE *outfile = fopen(temp_filename_.c_str(), "w");
     if (!outfile) {
       PDIE("opening '%s/%s' for writing", output_base_.c_str(),
diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh
index f4481ce9f15aec..998e6b35112df0 100755
--- a/src/test/shell/integration/runfiles_test.sh
+++ b/src/test/shell/integration/runfiles_test.sh
@@ -397,4 +397,35 @@ EOF
 }
 
 
+function test_removal_of_old_tempfiles() {
+  cat > BUILD << EOF
+sh_binary(
+    name = "foo",
+    srcs = ["foo.sh"],
+)
+EOF
+  touch foo.sh
+  chmod +x foo.sh
+
+  # Build once to create a runfiles directory.
+  bazel build //:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "build failed"
+
+  # Remove the MANIFEST file that was created by the previous build.
+  # Create an inaccessible file in the place where build-runfiles writes
+  # its temporary results.
+  #
+  # This simulates the case where the runfiles creation process is
+  # interrupted and leaves the temporary file behind. The temporary file
+  # may become read-only if it was stored in a snapshot.
+  rm ${PRODUCT_NAME}-bin/foo${EXT}.runfiles/MANIFEST
+  touch ${PRODUCT_NAME}-bin/foo${EXT}.runfiles/MANIFEST.tmp
+  chmod 0 ${PRODUCT_NAME}-bin/foo${EXT}.runfiles/MANIFEST.tmp
+
+  # Even with the inaccessible temporary file in place, build-runfiles
+  # should complete successfully. The MANIFEST file should be recreated.
+  bazel build //:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "build failed"
+  [[ -f ${PRODUCT_NAME}-bin/foo${EXT}.runfiles/MANIFEST ]] \
+    || fail "MANIFEST file not recreated"
+}
+
 run_suite "runfiles"