From 7760121d6a5690c4bd1aaa3cce0d383883469e67 Mon Sep 17 00:00:00 2001 From: Benjamin Peterson Date: Wed, 27 Sep 2023 11:34:58 -0700 Subject: [PATCH] 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. --- .../build/lib/actions/FileArtifactValue.java | 2 +- .../analysis/ConfiguredRuleClassProvider.java | 4 +- .../build/lib/exec/RunfilesTreeUpdater.java | 4 +- .../build/lib/exec/SpawnLogContext.java | 3 +- .../lib/remote/RemoteActionFileSystem.java | 4 +- .../build/lib/remote/util/DigestUtil.java | 3 +- .../skyframe/ActionOutputMetadataStore.java | 2 +- .../lib/testing/vfs/SpiedFileSystem.java | 4 +- .../build/lib/unix/UnixFileSystem.java | 4 +- .../devtools/build/lib/vfs/DigestUtils.java | 33 +++++++++++--- .../devtools/build/lib/vfs/FileSystem.java | 29 +++++++++--- .../build/lib/vfs/JavaIoFileSystem.java | 4 +- .../google/devtools/build/lib/vfs/Path.java | 24 ++++++++-- .../PathTransformingDelegateFileSystem.java | 4 +- .../analysis/StubbableFSBuildViewTest.java | 2 +- .../lib/exec/SingleBuildFileCacheTest.java | 4 +- .../lib/exec/SpawnLogContextTestBase.java | 4 +- .../remote/RemoteActionFileSystemTest.java | 17 +++---- .../remote/RemoteExecutionServiceTest.java | 45 +++++++++++-------- .../lib/skyframe/ArtifactFunctionTest.java | 6 ++- .../skyframe/ArtifactFunctionTestCase.java | 2 +- .../lib/skyframe/FileArtifactValueTest.java | 2 +- .../build/lib/skyframe/FileFunctionTest.java | 6 +-- .../unix/UnixDigestHashAttributeNameTest.java | 4 +- .../build/lib/vfs/DigestUtilsTest.java | 35 ++++++++------- .../build/lib/vfs/FileSystemTest.java | 5 ++- 26 files changed, 165 insertions(+), 91 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index b40c6426b41e6c..f4aa37f4895263 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/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); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java index e7e3ff8e3031ed..42c2a4f57b3bdd 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java +++ b/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(); } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java b/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java index 33b73f430fec47..3694e65ec48bd1 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java +++ b/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; } diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java index 30a5ba3277fde9..886c77c5626e89 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java +++ b/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(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index 4fd13a1abf9e71..f178fac495aa2d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/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 diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java index 781adf4abb0a42..271b3f534b782d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java +++ b/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 { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java index 13697b03fcfc4d..a112964bac4d2f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java +++ b/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) { diff --git a/src/main/java/com/google/devtools/build/lib/testing/vfs/SpiedFileSystem.java b/src/main/java/com/google/devtools/build/lib/testing/vfs/SpiedFileSystem.java index 3487cf6f47118f..9eadd9623217f5 100644 --- a/src/main/java/com/google/devtools/build/lib/testing/vfs/SpiedFileSystem.java +++ b/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 diff --git a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java index 249fe92706bb78..75d2cd73731847 100644 --- a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java +++ b/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); } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java b/src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java index 9716dfe872a675..b4347ec2568a50 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java +++ b/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. + * + *

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); } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java index ce45f0020c4092..b6b7b8556aa01c 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java +++ b/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(); } /** diff --git a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java index 9311eeba1ed92b..387c53b29da17d 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java +++ b/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); } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/Path.java b/src/main/java/com/google/devtools/build/lib/vfs/Path.java index f0258243612bfa..b84c4b2f26af80 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/Path.java +++ b/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()); diff --git a/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java index 3e427af8b1ca2e..9074cf39736499 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/PathTransformingDelegateFileSystem.java +++ b/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 diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java index b7783a59db988d..e9d8121b41df1b 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StubbableFSBuildViewTest.java +++ b/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); } } } diff --git a/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java b/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java index aa698adecb8066..aaf32540e03ace 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SingleBuildFileCacheTest.java +++ b/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 diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java index 81ea2772cfc2b8..59d7e3ec1df1ab 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java +++ b/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(); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index fe2af660aac9b9..0fef7f9fd37291 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/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 diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 8a4d43c69efc8e..3ab9f324ccc962 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -385,7 +385,7 @@ public void downloadOutputs_outputFiles() throws Exception { // assert assertThat(inMemoryOutput).isNull(); RemoteActionFileSystem actionFs = context.getActionFileSystem(); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file1"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file1"), -1)) .isEqualTo(toBinaryDigest(d1)); assertThat(readContent(execRoot.getRelative("outputs/file1"), UTF_8)).isEqualTo("content1"); assertThat(context.isLockOutputFilesCalled()).isTrue(); @@ -422,9 +422,10 @@ public void downloadOutputs_outputDirectories() throws Exception { // assert RemoteActionFileSystem actionFs = context.getActionFileSystem(); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/a/dir/foo"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/a/dir/foo"), -1)) .isEqualTo(toBinaryDigest(fooDigest)); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/a/dir/subdir/bar"))) + assertThat( + actionFs.getDigest(execRoot.asFragment().getRelative("outputs/a/dir/subdir/bar"), -1)) .isEqualTo(toBinaryDigest(barDigest)); assertThat(readContent(execRoot.getRelative("outputs/a/dir/foo"), UTF_8)) .isEqualTo("foo-contents"); @@ -1150,9 +1151,9 @@ public void downloadOutputs_outputFiles_partialDownload() throws Exception { // assert assertThat(inMemoryOutput).isNull(); RemoteActionFileSystem actionFs = context.getActionFileSystem(); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file1"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file1"), -1)) .isEqualTo(toBinaryDigest(d1)); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file2"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file2"), -1)) .isEqualTo(toBinaryDigest(d2)); assertThat(execRoot.getRelative("outputs/file1").exists()).isTrue(); assertThat(execRoot.getRelative("outputs/file2").exists()).isFalse(); @@ -1184,9 +1185,9 @@ public void downloadOutputs_outputFiles_noDownload() throws Exception { // assert assertThat(inMemoryOutput).isNull(); RemoteActionFileSystem actionFs = context.getActionFileSystem(); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file1"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file1"), -1)) .isEqualTo(toBinaryDigest(d1)); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file2"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file2"), -1)) .isEqualTo(toBinaryDigest(d2)); assertThat(execRoot.getRelative("outputs/file1").exists()).isFalse(); assertThat(execRoot.getRelative("outputs/file2").exists()).isFalse(); @@ -1234,9 +1235,9 @@ public void downloadOutputs_outputDirectories_partialDownload() throws Exception // assert assertThat(inMemoryOutput).isNull(); RemoteActionFileSystem actionFs = context.getActionFileSystem(); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/dir/file1"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/dir/file1"), -1)) .isEqualTo(toBinaryDigest(d1)); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/dir/a/file2"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/dir/a/file2"), -1)) .isEqualTo(toBinaryDigest(d2)); assertThat(execRoot.getRelative("outputs/dir/file1").exists()).isTrue(); assertThat(execRoot.getRelative("outputs/dir/a").exists()).isFalse(); @@ -1283,9 +1284,9 @@ public void downloadOutputs_outputDirectories_noDownload() throws Exception { // assert assertThat(inMemoryOutput).isNull(); RemoteActionFileSystem actionFs = context.getActionFileSystem(); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/dir/file1"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/dir/file1"), -1)) .isEqualTo(toBinaryDigest(d1)); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/dir/a/file2"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/dir/a/file2"), -1)) .isEqualTo(toBinaryDigest(d2)); assertThat(execRoot.getRelative("outputs/dir/file1").exists()).isFalse(); assertThat(execRoot.getRelative("outputs/dir/a").exists()).isFalse(); @@ -1366,8 +1367,10 @@ public void downloadOutputs_nonInlinedStdoutAndStderr_alwaysDownload() throws Ex // assert assertThat(inMemoryOutput).isNull(); RemoteActionFileSystem actionFs = context.getActionFileSystem(); - assertThat(actionFs.getDigest(outErr.getOutputPathFragment())).isEqualTo(toBinaryDigest(dOut)); - assertThat(actionFs.getDigest(outErr.getErrorPathFragment())).isEqualTo(toBinaryDigest(dErr)); + assertThat(actionFs.getDigest(outErr.getOutputPathFragment(), -1)) + .isEqualTo(toBinaryDigest(dOut)); + assertThat(actionFs.getDigest(outErr.getErrorPathFragment(), -1)) + .isEqualTo(toBinaryDigest(dErr)); assertThat(outErr.outAsLatin1()).isEqualTo("stdout"); assertThat(outErr.errAsLatin1()).isEqualTo("stderr"); Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); @@ -1400,8 +1403,10 @@ public void downloadOutputs_inlinedStdoutAndStderr_alwaysDownload() throws Excep // assert assertThat(inMemoryOutput).isNull(); RemoteActionFileSystem actionFs = context.getActionFileSystem(); - assertThat(actionFs.getDigest(outErr.getOutputPathFragment())).isEqualTo(toBinaryDigest(dOut)); - assertThat(actionFs.getDigest(outErr.getErrorPathFragment())).isEqualTo(toBinaryDigest(dErr)); + assertThat(actionFs.getDigest(outErr.getOutputPathFragment(), -1)) + .isEqualTo(toBinaryDigest(dOut)); + assertThat(actionFs.getDigest(outErr.getErrorPathFragment(), -1)) + .isEqualTo(toBinaryDigest(dErr)); assertThat(inMemoryOutput).isNull(); assertThat(outErr.outAsLatin1()).isEqualTo("stdout"); assertThat(outErr.errAsLatin1()).isEqualTo("stderr"); @@ -1443,9 +1448,9 @@ public void downloadOutputs_inMemoryOutput_doNotDownload() throws Exception { assertThat(inMemoryOutput.getOutput()) .isEqualTo(ActionsTestUtil.createArtifact(artifactRoot, "file1")); RemoteActionFileSystem actionFs = context.getActionFileSystem(); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file1"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file1"), -1)) .isEqualTo(toBinaryDigest(d1)); - assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file2"))) + assertThat(actionFs.getDigest(execRoot.asFragment().getRelative("outputs/file2"), -1)) .isEqualTo(toBinaryDigest(d2)); assertThat(execRoot.getRelative("outputs/file1").exists()).isFalse(); assertThat(execRoot.getRelative("outputs/file2").exists()).isFalse(); @@ -1554,9 +1559,11 @@ public void downloadOutputs_pathUnmapped() throws Exception { assertThat(inMemoryOutput).isNull(); RemoteActionFileSystem actionFs = context.getActionFileSystem(); - assertThat(actionFs.getDigest(output1.getPath().asFragment())).isEqualTo(toBinaryDigest(d1)); + assertThat(actionFs.getDigest(output1.getPath().asFragment(), -1)) + .isEqualTo(toBinaryDigest(d1)); assertThat(readContent(output1.getPath(), UTF_8)).isEqualTo("content1"); - assertThat(actionFs.getDigest(output2.getPath().asFragment())).isEqualTo(toBinaryDigest(d2)); + assertThat(actionFs.getDigest(output2.getPath().asFragment(), -1)) + .isEqualTo(toBinaryDigest(d2)); assertThat(readContent(output2.getPath(), UTF_8)).isEqualTo("content2"); assertThat(context.isLockOutputFilesCalled()).isTrue(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java index 77e43a5d1c5eae..2d9f06572147c5 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java @@ -119,8 +119,10 @@ public void testUnreadableInputWithFsWithAvailableDigest() throws Throwable { setupRoot( new CustomInMemoryFs() { @Override - public byte[] getDigest(PathFragment path) throws IOException { - return path.getBaseName().equals("unreadable") ? expectedDigest : super.getDigest(path); + public byte[] getDigest(PathFragment path, long expectedSize) throws IOException { + return path.getBaseName().equals("unreadable") + ? expectedDigest + : super.getDigest(path, expectedSize); } }); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java index f67a9a06326e30..920666934db15e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java @@ -180,7 +180,7 @@ protected class CustomInMemoryFs extends InMemoryFileSystem { @Override @SuppressWarnings("UnsynchronizedOverridesSynchronized") protected byte[] getFastDigest(PathFragment path) throws IOException { - return fastDigest ? getDigest(path) : null; + return fastDigest ? getDigest(path, -1) : null; } } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java index c0a393dcd18679..a3432cc0cbcd68 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java @@ -186,7 +186,7 @@ public void testIOException() throws Exception { FileSystem fs = new InMemoryFileSystem(DigestHashFunction.SHA256) { @Override - public byte[] getDigest(PathFragment path) throws IOException { + public byte[] getDigest(PathFragment path, long expectedSize) throws IOException { throw exception; } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java index 7294283a2168bb..e9b2689f8aea9d 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java @@ -895,9 +895,9 @@ public void testDigest() throws Exception { fs = new CustomInMemoryFs(manualClock) { @Override - protected byte[] getDigest(PathFragment path) throws IOException { + protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException { digestCalls.incrementAndGet(); - return super.getDigest(path); + return super.getDigest(path, expectedSize); } }; pkgRoot = Root.fromPath(fs.getPath("/root")); @@ -1816,7 +1816,7 @@ protected byte[] getFastDigest(PathFragment path) throws IOException { if (stubbedFastDigestErrors.containsKey(path)) { throw stubbedFastDigestErrors.get(path); } - return fastDigest ? getDigest(path) : null; + return fastDigest ? getDigest(path, -1) : null; } void stubStat(Path path, @Nullable FileStatus stubbedResult) { diff --git a/src/test/java/com/google/devtools/build/lib/unix/UnixDigestHashAttributeNameTest.java b/src/test/java/com/google/devtools/build/lib/unix/UnixDigestHashAttributeNameTest.java index 357a16ae9a680b..97001c1ed4bb36 100644 --- a/src/test/java/com/google/devtools/build/lib/unix/UnixDigestHashAttributeNameTest.java +++ b/src/test/java/com/google/devtools/build/lib/unix/UnixDigestHashAttributeNameTest.java @@ -42,7 +42,9 @@ public void testFoo() throws Exception { // Instead of actually trying to access this file, a call to getxattr() should be made. We // intercept this call and return a fake extended attribute value, thereby causing the checksum // computation to be skipped entirely. - assertThat(DigestUtils.getDigestWithManualFallback(absolutize("myfile"), SyscallCache.NO_CACHE)) + assertThat( + DigestUtils.getDigestWithManualFallback( + absolutize("myfile"), 123, SyscallCache.NO_CACHE)) .isEqualTo(FAKE_DIGEST); } diff --git a/src/test/java/com/google/devtools/build/lib/vfs/DigestUtilsTest.java b/src/test/java/com/google/devtools/build/lib/vfs/DigestUtilsTest.java index a3ef020445fc86..def866af785f11 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/DigestUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/DigestUtilsTest.java @@ -49,7 +49,7 @@ private static void assertDigestCalculationConcurrency( FileSystem myfs = new InMemoryFileSystem(hf) { @Override - protected byte[] getDigest(PathFragment path) throws IOException { + protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException { try { barrierLatch.countDown(); readyLatch.countDown(); @@ -59,12 +59,12 @@ protected byte[] getDigest(PathFragment path) throws IOException { } catch (Exception e) { throw new IOException(e); } - return super.getDigest(path); + return super.getDigest(path, expectedSize); } @Override protected byte[] getFastDigest(PathFragment path) throws IOException { - return fastDigest ? super.getDigest(path) : null; + return fastDigest ? super.getDigest(path, -1) : null; } }; @@ -75,14 +75,12 @@ protected byte[] getFastDigest(PathFragment path) throws IOException { TestThread thread1 = new TestThread( - () -> { - var unused = DigestUtils.getDigestWithManualFallback(myFile1, SyscallCache.NO_CACHE); - }); + () -> + DigestUtils.getDigestWithManualFallback(myFile1, fileSize1, SyscallCache.NO_CACHE)); TestThread thread2 = new TestThread( - () -> { - var unused = DigestUtils.getDigestWithManualFallback(myFile2, SyscallCache.NO_CACHE); - }); + () -> + DigestUtils.getDigestWithManualFallback(myFile2, fileSize2, SyscallCache.NO_CACHE)); thread1.start(); thread2.start(); if (!expectConcurrent) { // Synchronized case. @@ -111,9 +109,9 @@ protected byte[] getFastDigest(PathFragment path) { } @Override - protected byte[] getDigest(PathFragment path) throws IOException { + protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException { getDigestCounter.incrementAndGet(); - return super.getDigest(path); + return super.getDigest(path, expectedSize); } }; @@ -122,18 +120,23 @@ protected byte[] getDigest(PathFragment path) throws IOException { Path file = tracingFileSystem.getPath("/file.txt"); FileSystemUtils.writeContentAsLatin1(file, "some contents"); - byte[] digest = DigestUtils.getDigestWithManualFallback(file, SyscallCache.NO_CACHE); + byte[] digest = + DigestUtils.getDigestWithManualFallback(file, file.getFileSize(), SyscallCache.NO_CACHE); assertThat(getFastDigestCounter.get()).isEqualTo(1); assertThat(getDigestCounter.get()).isEqualTo(1); - assertThat(DigestUtils.getDigestWithManualFallback(file, SyscallCache.NO_CACHE)) + assertThat( + DigestUtils.getDigestWithManualFallback( + file, file.getFileSize(), SyscallCache.NO_CACHE)) .isEqualTo(digest); assertThat(getFastDigestCounter.get()).isEqualTo(2); assertThat(getDigestCounter.get()).isEqualTo(1); // Cached. DigestUtils.clearCache(); - assertThat(DigestUtils.getDigestWithManualFallback(file, SyscallCache.NO_CACHE)) + assertThat( + DigestUtils.getDigestWithManualFallback( + file, file.getFileSize(), SyscallCache.NO_CACHE)) .isEqualTo(digest); assertThat(getFastDigestCounter.get()).isEqualTo(3); assertThat(getDigestCounter.get()).isEqualTo(2); // Not cached. @@ -150,13 +153,13 @@ protected byte[] getFastDigest(PathFragment path) { } @Override - protected byte[] getDigest(PathFragment path) { + protected byte[] getDigest(PathFragment path, long expectedSize) { return digest; } }; Path file = noDigestFileSystem.getPath("/f.txt"); FileSystemUtils.writeContentAsLatin1(file, "contents"); - assertThat(DigestUtils.manuallyComputeDigest(file)).isEqualTo(digest); + assertThat(DigestUtils.manuallyComputeDigest(file, /* fileSize= */ 8)).isEqualTo(digest); } } diff --git a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java index 379185572c0314..5364ed36bc40f7 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java @@ -1794,11 +1794,14 @@ public void testGetDigestForEmptyFile() throws Exception { @Test public void testGetDigest() throws Exception { - byte[] buffer = new byte[500000]; + var size = 500000; + byte[] buffer = new byte[size]; for (int i = 0; i < buffer.length; ++i) { buffer[i] = 1; } FileSystemUtils.writeContent(xFile, buffer); + assertThrows(IOException.class, () -> xFile.getDigest(size + 1)); + assertThrows(IOException.class, () -> xFile.getDigest(size - 1)); Fingerprint fp = new Fingerprint(digestHashFunction); fp.addBytes(buffer); assertThat(fp.hexDigestAndReset())