[6.4.0] build-runfiles: remove temporary file prior to creating it (#…

…19386)

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.

Commit
357fc5f

PiperOrigin-RevId: 561615005
Change-Id: I8ca95c7d35df8a53af8f632b10b4a6141d180631

Co-authored-by: Ed Schouten <eschouten@apple.com>

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(),

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"