Close the ZipFileSystem and the underlying ZipFile appropriately after we finished extracting the FDO profile.

Also fix a truly embarrassing infinite recursion bug introduced by Yours Truly in unknown commit .

This avoids a failure mode where, when two profiles at the same path are used in two builds close one after the other, the file handle would get erroneously re-used.

RELNOTES: None.
PiperOrigin-RevId: 163063976

2 files changed, 28 insertions, 15 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
index f763749b95..180d687f87 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
@@ -354,20 +354,24 @@ public class FdoSupport {
         FileSystemUtils.ensureSymbolicLink(
             execRoot.getRelative(getAutoProfilePath(fdoProfile, fdoRootExecPath)), fdoProfile);
       } else {
-        Path zipFilePath = new ZipFileSystem(fdoProfile).getRootDirectory();
-        String outputSymlinkName = productName + "-out";
-        if (!zipFilePath.getRelative(outputSymlinkName).isDirectory()) {
-          throw new ZipException(
-              "FDO zip files must be zipped directly above '"
-                  + outputSymlinkName
-                  + "' for the compiler to find the profile");
+        // Path objects referring to inside the zip file are only valid within this try block.
+        // FdoZipContents doesn't reference any of them, so we are fine.
+        try (ZipFileSystem zipFileSystem = new ZipFileSystem(fdoProfile)) {
+          Path zipFilePath = zipFileSystem.getRootDirectory();
+          String outputSymlinkName = productName + "-out";
+          if (!zipFilePath.getRelative(outputSymlinkName).isDirectory()) {
+            throw new ZipException(
+                "FDO zip files must be zipped directly above '"
+                    + outputSymlinkName
+                    + "' for the compiler to find the profile");
+          }
+          ImmutableSet.Builder<PathFragment> gcdaFilesBuilder = ImmutableSet.builder();
+          ImmutableMultimap.Builder<PathFragment, PathFragment> importsBuilder =
+              ImmutableMultimap.builder();
+          extractFdoZipDirectory(zipFilePath, fdoDirPath, gcdaFilesBuilder, importsBuilder);
+          gcdaFiles = gcdaFilesBuilder.build();
+          imports = importsBuilder.build();
         }
-        ImmutableSet.Builder<PathFragment> gcdaFilesBuilder = ImmutableSet.builder();
-        ImmutableMultimap.Builder<PathFragment, PathFragment> importsBuilder =
-            ImmutableMultimap.builder();
-        extractFdoZipDirectory(zipFilePath, fdoDirPath, gcdaFilesBuilder, importsBuilder);
-        gcdaFiles = gcdaFilesBuilder.build();
-        imports = importsBuilder.build();
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/ZipFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/ZipFileSystem.java
index 5419f7a1f8..aa9ad1ae29 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/ZipFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/ZipFileSystem.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.vfs;
 
+import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.Path.PathFactory;
@@ -26,6 +27,7 @@ import java.io.OutputStream;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.logging.Logger;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
 
@@ -33,8 +35,9 @@ import java.util.zip.ZipFile;
  * A FileSystem that provides a read-only filesystem view on a zip file.
  * Inherits the constraints imposed by ReadonlyFileSystem.
  */
-@ThreadSafe
+@ThreadCompatible  // Can only be accessed from one thread at a time (including its Path objects)
 public class ZipFileSystem extends ReadonlyFileSystem implements Closeable {
+  private static final Logger log = Logger.getLogger(ZipFileSystem.class.getName());
 
   private final File tempFile;  // In case this needs to be written to the file system
   private final ZipFile zipFile;
@@ -305,7 +308,13 @@ public class ZipFileSystem extends ReadonlyFileSystem implements Closeable {
   @Override
   public void close() {
     if (open) {
-      close();
+      try {
+        zipFile.close();
+      } catch (IOException e) {
+        // Not a lot can be done about this. Log an error and move on.
+        log.warning(String.format(
+            "Error while closing zip file '%s': %s", zipFile.getName(), e.getMessage()));
+      }
       if (tempFile != null) {
         tempFile.delete();
       }