Check that digesting consumes the expected number of bytes.

In Bazel, the time a file is `stat`ed to find its size can be very far from the time the file's digest is computed, which creates a window for shenanigans. Allow passing the expected size into `Path.getDigest` and raise an error there if the number of digested bytes doesn't match the previously seen size.

Author: benjaminp

src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java

@@ -273,7 +273,7 @@ private static FileArtifactValue create(
       return new DirectoryArtifactValue(path.getLastModifiedTime());
     }
     if (digest == null) {
-      digest = DigestUtils.getDigestWithManualFallback(path, xattrProvider);
+      digest = DigestUtils.getDigestWithManualFallback(path, size, xattrProvider);
     }
     Preconditions.checkState(digest != null, path);
     return createForNormalFile(digest, proxy, size);

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java

@@ -126,11 +126,11 @@ public BundledFileSystem() {
 
     @Override
     protected synchronized byte[] getFastDigest(PathFragment path) {
-      return getDigest(path);
+      return getDigest(path, -1);
     }
 
     @Override
-    protected synchronized byte[] getDigest(PathFragment path) {
+    protected synchronized byte[] getDigest(PathFragment path, long expectedSize) {
       return getDigestFunction().getHashFunction().hashString(path.toString(), UTF_8).asBytes();
     }
   }

src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java

@@ -138,8 +138,8 @@ private void updateRunfilesTree(
       if (tree.getSymlinksMode() != SKIP
           && !outputManifest.isSymbolicLink()
           && Arrays.equals(
-              DigestUtils.getDigestWithManualFallback(outputManifest, xattrProvider),
-              DigestUtils.getDigestWithManualFallback(inputManifest, xattrProvider))
+              DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(outputManifest, xattrProvider),
+              DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(inputManifest, xattrProvider))
           && (OS.getCurrent() != OS.WINDOWS || isRunfilesDirectoryPopulated(runfilesDirPath))) {
         return;
       }

src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java

@@ -177,7 +177,8 @@ protected Digest computeDigest(
     // Try to obtain a digest from the filesystem.
     return builder
         .setHash(
-            HashCode.fromBytes(DigestUtils.getDigestWithManualFallback(path, xattrProvider))
+            HashCode.fromBytes(
+                    DigestUtils.getDigestWithManualFallback(path, fileSize, xattrProvider))
                 .toString())
         .setSizeBytes(fileSize)
         .build();

src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java

@@ -434,7 +434,7 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
   }
 
   @Override
-  protected byte[] getDigest(PathFragment path) throws IOException {
+  protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
     path = resolveSymbolicLinks(path).asFragment();
     // Try to obtain a fast digest through a stat. This is only possible for in-memory files.
     // The parent path has already been canonicalized by resolveSymbolicLinks, so FOLLOW_NONE is
@@ -443,7 +443,7 @@ protected byte[] getDigest(PathFragment path) throws IOException {
     if (status instanceof FileStatusWithDigest) {
       return ((FileStatusWithDigest) status).getDigest();
     }
-    return localFs.getPath(path).getDigest();
+    return localFs.getPath(path).getDigest(expectedSize);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java

@@ -72,7 +72,8 @@ public Digest compute(Path file) throws IOException {
   }
 
   public Digest compute(Path file, long fileSize) throws IOException {
-    return buildDigest(DigestUtils.getDigestWithManualFallback(file, xattrProvider), fileSize);
+    return buildDigest(
+        DigestUtils.getDigestWithManualFallback(file, fileSize, xattrProvider), fileSize);
   }
 
   public Digest compute(VirtualActionInput input) throws IOException {

src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java

@@ -557,7 +557,7 @@ private FileArtifactValue constructFileArtifactValue(
       // possible to hit the digest cache - we probably already computed the digest for the
       // target during previous action execution.
       Path pathToDigest = isResolvedSymlink ? statAndValue.realPath() : statAndValue.pathNoFollow();
-      actualDigest = DigestUtils.manuallyComputeDigest(pathToDigest);
+      actualDigest = DigestUtils.manuallyComputeDigest(pathToDigest, value.getSize());
     }
 
     if (!isResolvedSymlink) {

src/main/java/com/google/devtools/build/lib/testing/vfs/SpiedFileSystem.java

@@ -58,8 +58,8 @@ public boolean createWritableDirectory(PathFragment path) throws IOException {
   }
 
   @Override
-  public byte[] getDigest(PathFragment path) throws IOException {
-    return super.getDigest(path);
+  public byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
+    return super.getDigest(path, expectedSize);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java

@@ -446,11 +446,11 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
   }
 
   @Override
-  protected byte[] getDigest(PathFragment path) throws IOException {
+  protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
     String name = path.toString();
     long startTime = Profiler.nanoTimeMaybe();
     try {
-      return super.getDigest(path);
+      return super.getDigest(path, expectedSize);
     } finally {
       profiler.logSimpleTask(startTime, ProfilerTask.VFS_MD5, name);
     }

src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java

@@ -160,19 +160,40 @@ public static CacheStats getCacheStats() {
    * #manuallyComputeDigest} to skip an additional attempt to obtain the fast digest.
    *
    * @param path Path of the file.
+   * @param fileSize Size of the file. Used to determine if digest calculation should be done
+   *     serially or in parallel. Files larger than a certain threshold will be read serially, in
+   *     order to avoid excessive disk seeks.
    */
-  public static byte[] getDigestWithManualFallback(Path path, XattrProvider xattrProvider)
-      throws IOException {
+  public static byte[] getDigestWithManualFallback(
+      Path path, long fileSize, XattrProvider xattrProvider) throws IOException {
     byte[] digest = xattrProvider.getFastDigest(path);
-    return digest != null ? digest : manuallyComputeDigest(path);
+    return digest != null ? digest : manuallyComputeDigest(path, fileSize);
+  }
+
+  /**
+   * Gets the digest of {@code path}, using a constant-time xattr call if the filesystem supports
+   * it, and calculating the digest manually otherwise.
+   *
+   * <p>Unlike {@link #getDigestWithManualFallback}, will not rate-limit manual digesting of files,
+   * so only use this method if the file size is truly unknown and you don't expect many concurrent
+   * manual digests of large files.
+   *
+   * @param path Path of the file.
+   */
+  public static byte[] getDigestWithManualFallbackWhenSizeUnknown(
+      Path path, XattrProvider xattrProvider) throws IOException {
+    return getDigestWithManualFallback(path, -1, xattrProvider);
   }
 
   /**
    * Calculates the digest manually.
    *
    * @param path Path of the file.
+   * @param fileSize Size of the file. Used to determine if digest calculation should be done
+   *     serially or in parallel. Files larger than a certain threshold will be read serially, in
+   *     order to avoid excessive disk seeks.
    */
-  public static byte[] manuallyComputeDigest(Path path) throws IOException {
+  public static byte[] manuallyComputeDigest(Path path, long fileSize) throws IOException {
     byte[] digest;
 
     // Attempt a cache lookup if the cache is enabled.
@@ -186,9 +207,9 @@ public static byte[] manuallyComputeDigest(Path path) throws IOException {
       }
     }
 
-    digest = path.getDigest();
+    digest = path.getDigest(fileSize);
 
-    Preconditions.checkNotNull(digest, "Missing digest for %s", path);
+    Preconditions.checkNotNull(digest, "Missing digest for %s (size %s)", path, fileSize);
     if (cache != null) {
       cache.put(key, digest);
     }

src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java

@@ -20,6 +20,7 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Lists;
+import com.google.common.hash.Funnels;
 import com.google.common.io.ByteSource;
 import com.google.common.io.CharStreams;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
@@ -33,6 +34,7 @@
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Locale;
 import javax.annotation.Nullable;
 
 /** This interface models a file system. */
@@ -347,13 +349,26 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
    * @return a new byte array containing the file's digest
    * @throws IOException if the digest could not be computed for any reason
    */
-  protected byte[] getDigest(PathFragment path) throws IOException {
-    return new ByteSource() {
-      @Override
-      public InputStream openStream() throws IOException {
-        return getInputStream(path);
-      }
-    }.hash(digestFunction.getHashFunction()).asBytes();
+  protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
+    var bs =
+        new ByteSource() {
+          @Override
+          public InputStream openStream() throws IOException {
+            return getInputStream(path);
+          }
+        };
+    var hasher = digestFunction.getHashFunction().newHasher();
+    var copied = bs.copyTo(Funnels.asOutputStream(hasher));
+    if (expectedSize != -1 && copied != expectedSize) {
+      throw new IOException(
+          String.format(
+              Locale.US,
+              "digesting %s saw %s bytes rather than the expected %s",
+              path,
+              copied,
+              expectedSize));
+    }
+    return hasher.hash().asBytes();
   }
 
   /**

src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java

@@ -420,11 +420,11 @@ public void setLastModifiedTime(PathFragment path, long newTime) throws IOExcept
   }
 
   @Override
-  protected byte[] getDigest(PathFragment path) throws IOException {
+  protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
     String name = path.toString();
     long startTime = Profiler.nanoTimeMaybe();
     try {
-      return super.getDigest(path);
+      return super.getDigest(path, expectedSize);
     } finally {
       profiler.logSimpleTask(startTime, ProfilerTask.VFS_MD5, name);
     }

src/main/java/com/google/devtools/build/lib/vfs/Path.java

@@ -686,7 +686,23 @@ public byte[] getFastDigest() throws IOException {
    * @throws IOException if the digest could not be computed for any reason
    */
   public byte[] getDigest() throws IOException {
-    return fileSystem.getDigest(asFragment());
+    return getDigest(-1);
+  }
+
+  /**
+   * Returns the digest of the file denoted by the current path, following symbolic links. Is not
+   * guaranteed to call {@link #getFastDigest} internally, even if a fast digest is likely
+   * available. Callers should prefer {@link DigestUtils#getDigestWithManualFallback} to this method
+   * unless they know that a fast digest is unavailable and do not need the other features
+   * (disk-read rate-limiting, global cache) that {@link DigestUtils} provides.
+   *
+   * @param expectedSize If not -1, the expected number of bytes to digest. If this does not match
+   *     the number of bytes digested, an {@link IOException} may be raised.
+   * @return a new byte array containing the file's digest
+   * @throws IOException if the digest could not be computed for any reason
+   */
+  public byte[] getDigest(long expectedSize) throws IOException {
+    return fileSystem.getDigest(asFragment(), expectedSize);
   }
 
   /**
@@ -716,7 +732,8 @@ public String getDirectoryDigest(XattrProvider xattrProvider) throws IOException
         } else {
           hasher.putChar('-');
         }
-        hasher.putBytes(DigestUtils.getDigestWithManualFallback(path, xattrProvider));
+        hasher.putBytes(
+            DigestUtils.getDigestWithManualFallback(path, stat.getSize(), xattrProvider));
       } else if (stat.isDirectory()) {
         hasher.putChar('d').putUnencodedChars(path.getDirectoryDigest(xattrProvider));
       } else if (stat.isSymbolicLink()) {
@@ -730,7 +747,8 @@ public String getDirectoryDigest(XattrProvider xattrProvider) throws IOException
               } else {
                 hasher.putChar('-');
               }
-              hasher.putBytes(DigestUtils.getDigestWithManualFallback(resolved, xattrProvider));
+              hasher.putBytes(
+                  DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(resolved, xattrProvider));
             } else {
               // link to a non-file: include the link itself in the hash
               hasher.putChar('l').putUnencodedChars(link.toString());

src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java

@@ -227,8 +227,8 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
   }
 
   @Override
-  protected byte[] getDigest(PathFragment path) throws IOException {
-    return delegateFs.getDigest(toDelegatePath(path));
+  protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
+    return delegateFs.getDigest(toDelegatePath(path), expectedSize);
   }
 
   @Override

src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java

@@ -137,7 +137,7 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
       if (stubbedFastDigestErrors.containsKey(path)) {
         throw stubbedFastDigestErrors.get(path);
       }
-      return getDigest(path);
+      return getDigest(path, -1);
     }
   }
 }

src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java

@@ -61,9 +61,9 @@ protected InputStream getInputStream(PathFragment path) throws IOException {
           }
 
           @Override
-          protected byte[] getDigest(PathFragment path) throws IOException {
+          protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
             byte[] override = digestOverrides.get(path.getPathString());
-            return override != null ? override : super.getDigest(path);
+            return override != null ? override : super.getDigest(path, expectedSize);
           }
 
           @Override

src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java

@@ -86,11 +86,11 @@ protected static final class FakeActionFileSystem extends DelegateFileSystem {
 
     @Override
     protected byte[] getFastDigest(PathFragment path) throws IOException {
-      return super.getDigest(path);
+      return super.getDigest(path, -1);
     }
 
     @Override
-    protected byte[] getDigest(PathFragment path) throws IOException {
+    protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
       throw new UnsupportedOperationException();
     }
   }

src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java

@@ -21,6 +21,7 @@
 import static java.util.Arrays.stream;
 import static org.junit.Assert.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doReturn;
@@ -578,9 +579,9 @@ public void getDigest_fromInputArtifactData_forLocalArtifact() throws Exception
     // Verify that we don't fall back to a slow digest.
     reset(fs);
     assertThat(actionFs.getFastDigest(path)).isEqualTo(getDigest("local contents"));
-    verify(fs, never()).getDigest(any());
+    verify(fs, never()).getDigest(any(), anyLong());
 
-    assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("local contents"));
+    assertThat(actionFs.getDigest(path, -1)).isEqualTo(getDigest("local contents"));
   }
 
   @Test
@@ -593,9 +594,9 @@ public void getDigest_fromInputArtifactData_forRemoteArtifact() throws Exception
     // Verify that we don't fall back to a slow digest.
     reset(fs);
     assertThat(actionFs.getFastDigest(path)).isEqualTo(getDigest("remote contents"));
-    verify(fs, never()).getDigest(any());
+    verify(fs, never()).getDigest(any(), anyLong());
 
-    assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("remote contents"));
+    assertThat(actionFs.getDigest(path, -1)).isEqualTo(getDigest("remote contents"));
   }
 
   @Test
@@ -606,7 +607,7 @@ public void getDigest_fromRemoteOutputTree() throws Exception {
     injectRemoteFile(actionFs, artifact.getPath().asFragment(), "remote contents");
 
     assertThat(actionFs.getFastDigest(path)).isEqualTo(getDigest("remote contents"));
-    assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("remote contents"));
+    assertThat(actionFs.getDigest(path, -1)).isEqualTo(getDigest("remote contents"));
   }
 
   @Test
@@ -617,7 +618,7 @@ public void getDigest_fromLocalFilesystem() throws Exception {
     writeLocalFile(actionFs, artifact.getPath().asFragment(), "local contents");
 
     assertThat(actionFs.getFastDigest(path)).isNull();
-    assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("local contents"));
+    assertThat(actionFs.getDigest(path, -1)).isEqualTo(getDigest("local contents"));
   }
 
   @Test
@@ -627,7 +628,7 @@ public void getDigest_notFound() throws Exception {
     PathFragment path = artifact.getPath().asFragment();
 
     assertThrows(FileNotFoundException.class, () -> actionFs.getFastDigest(path));
-    assertThrows(FileNotFoundException.class, () -> actionFs.getDigest(path));
+    assertThrows(FileNotFoundException.class, () -> actionFs.getDigest(path, -1));
   }
 
   @Test
@@ -650,7 +651,7 @@ public void getDigest_followSymlinks(
       assertThat(actionFs.getFastDigest(linkPath)).isEqualTo(getDigest("content"));
     }
 
-    assertThat(actionFs.getDigest(linkPath)).isEqualTo(getDigest("content"));
+    assertThat(actionFs.getDigest(linkPath, -1)).isEqualTo(getDigest("content"));
   }
 
   @Test