From c2bcf3a5eebacf225538c1cb9e44f5dabf91f752 Mon Sep 17 00:00:00 2001 From: Tyler French Date: Mon, 11 May 2026 01:18:18 -0700 Subject: [PATCH 1/2] Verify the whole blob when downloading chunks (https://github.com/bazelbuild/bazel/pull/29435) This is a bug fix for the chunking implementation, where we should verify the hash of the full blob if verify downloads is specified, so we don't blindly trust SplitBlob responses. Closes #29435. PiperOrigin-RevId: 913555501 Change-Id: I7ffd600e53e585ad946f15c89201382343992d7e --- .../lib/remote/ChunkedBlobDownloader.java | 21 ++++++- .../build/lib/remote/CombinedCache.java | 2 +- .../build/lib/remote/GrpcCacheClient.java | 4 ++ .../lib/remote/ChunkedBlobDownloaderTest.java | 56 ++++++++++++++++++- 4 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/ChunkedBlobDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/ChunkedBlobDownloader.java index c2fd63e6bfa4e5..771e229981d429 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ChunkedBlobDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ChunkedBlobDownloader.java @@ -18,21 +18,29 @@ import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.SplitBlobResponse; +import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.util.DigestOutputStream; +import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.Utils; import java.io.IOException; import java.io.OutputStream; import java.util.List; +import javax.annotation.Nullable; /** Downloads blobs by sequentially fetching chunks via the SplitBlob API. */ public class ChunkedBlobDownloader { private final GrpcCacheClient grpcCacheClient; private final CombinedCache combinedCache; + private final DigestUtil digestUtil; - public ChunkedBlobDownloader(GrpcCacheClient grpcCacheClient, CombinedCache combinedCache) { + public ChunkedBlobDownloader( + GrpcCacheClient grpcCacheClient, CombinedCache combinedCache, DigestUtil digestUtil) { this.grpcCacheClient = grpcCacheClient; this.combinedCache = combinedCache; + this.digestUtil = digestUtil; } /** @@ -43,14 +51,23 @@ public ChunkedBlobDownloader(GrpcCacheClient grpcCacheClient, CombinedCache comb public void downloadChunked( RemoteActionExecutionContext context, Digest blobDigest, OutputStream out) throws IOException, InterruptedException { + @Nullable DigestOutputStream digestOut = null; + if (grpcCacheClient.shouldVerifyDownloads()) { + digestOut = digestUtil.newDigestOutputStream(out); + out = digestOut; + } + List chunkDigests = getChunkDigests(context, blobDigest); downloadAndReassembleChunks(context, chunkDigests, out); + if (digestOut != null) { + Utils.verifyBlobContents(blobDigest, digestOut.digest()); + } } private List getChunkDigests(RemoteActionExecutionContext context, Digest blobDigest) throws IOException, InterruptedException { if (blobDigest.getSizeBytes() == 0) { - return List.of(); + return ImmutableList.of(); } ListenableFuture splitResponseFuture = grpcCacheClient.splitBlob(context, blobDigest); diff --git a/src/main/java/com/google/devtools/build/lib/remote/CombinedCache.java b/src/main/java/com/google/devtools/build/lib/remote/CombinedCache.java index b77774d0f6329c..09760b7a7b4a93 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/CombinedCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/CombinedCache.java @@ -129,7 +129,7 @@ boolean supported() throws IOException { synchronized (this) { config = ChunkingConfig.fromServerCapabilities(getRemoteServerCapabilities()); if (config != null) { - downloader = new ChunkedBlobDownloader(grpcClient, CombinedCache.this); + downloader = new ChunkedBlobDownloader(grpcClient, CombinedCache.this, digestUtil); uploader = new ChunkedBlobUploader(grpcClient, CombinedCache.this, config, digestUtil); } initialized = true; diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java index 19ebe9723826c7..a5d96d1135c5c9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java @@ -97,6 +97,10 @@ public class GrpcCacheClient implements RemoteCacheClient, MissingDigestsFinder private final AtomicBoolean closed = new AtomicBoolean(); + boolean shouldVerifyDownloads() { + return options.getRemoteVerifyDownloads(); + } + @VisibleForTesting public GrpcCacheClient( ReferenceCountedChannel channel, diff --git a/src/test/java/com/google/devtools/build/lib/remote/ChunkedBlobDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ChunkedBlobDownloaderTest.java index c3c01b23cd00d8..c46a1bca7b32c1 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ChunkedBlobDownloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ChunkedBlobDownloaderTest.java @@ -24,6 +24,7 @@ import build.bazel.remote.execution.v2.SplitBlobResponse; import com.google.common.util.concurrent.Futures; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; +import com.google.devtools.build.lib.remote.common.OutputDigestMismatchException; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -56,7 +57,8 @@ public class ChunkedBlobDownloaderTest { @Before public void setUp() { - downloader = new ChunkedBlobDownloader(grpcCacheClient, combinedCache); + when(grpcCacheClient.shouldVerifyDownloads()).thenReturn(true); + downloader = new ChunkedBlobDownloader(grpcCacheClient, combinedCache, DIGEST_UTIL); } @Test @@ -184,4 +186,56 @@ public void downloadChunked_chunkFailsAfterPartialWrite_throwsIOException() thro ByteArrayOutputStream out = new ByteArrayOutputStream(); assertThrows(IOException.class, () -> downloader.downloadChunked(context, blobDigest, out)); } + + @Test + public void downloadChunked_blobDigestMismatch_throwsOutputDigestMismatch() throws Exception { + byte[] chunkData = new byte[] {1, 2, 3}; + Digest chunkDigest = DIGEST_UTIL.compute(chunkData); + Digest blobDigest = DIGEST_UTIL.compute(new byte[] {4, 5, 6}); + + SplitBlobResponse splitResponse = + SplitBlobResponse.newBuilder().addChunkDigests(chunkDigest).build(); + when(grpcCacheClient.splitBlob(any(), eq(blobDigest))) + .thenReturn(Futures.immediateFuture(splitResponse)); + when(combinedCache.downloadBlob(any(), eq(chunkDigest), any())) + .thenAnswer( + invocation -> { + OutputStream out = invocation.getArgument(2); + out.write(chunkData); + return Futures.immediateFuture(null); + }); + + OutputDigestMismatchException e = + assertThrows( + OutputDigestMismatchException.class, + () -> downloader.downloadChunked(context, blobDigest, new ByteArrayOutputStream())); + + assertThat(e).hasMessageThat().contains(blobDigest.getHash()); + assertThat(e).hasMessageThat().contains(chunkDigest.getHash()); + } + + @Test + public void downloadChunked_blobDigestMismatchVerificationDisabled_succeeds() throws Exception { + when(grpcCacheClient.shouldVerifyDownloads()).thenReturn(false); + byte[] chunkData = new byte[] {1, 2, 3}; + Digest chunkDigest = DIGEST_UTIL.compute(chunkData); + Digest blobDigest = DIGEST_UTIL.compute(new byte[] {4, 5, 6}); + + SplitBlobResponse splitResponse = + SplitBlobResponse.newBuilder().addChunkDigests(chunkDigest).build(); + when(grpcCacheClient.splitBlob(any(), eq(blobDigest))) + .thenReturn(Futures.immediateFuture(splitResponse)); + when(combinedCache.downloadBlob(any(), eq(chunkDigest), any())) + .thenAnswer( + invocation -> { + OutputStream out = invocation.getArgument(2); + out.write(chunkData); + return Futures.immediateFuture(null); + }); + + ByteArrayOutputStream out = new ByteArrayOutputStream(); + downloader.downloadChunked(context, blobDigest, out); + + assertThat(out.toByteArray()).isEqualTo(chunkData); + } } From b2c9d92524cd761b044220af6b6665015a52d6cd Mon Sep 17 00:00:00 2001 From: iancha1992 Date: Tue, 19 May 2026 12:14:03 -0700 Subject: [PATCH 2/2] Fix --- .../com/google/devtools/build/lib/remote/GrpcCacheClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java index a5d96d1135c5c9..9139250eaa6698 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java @@ -98,7 +98,7 @@ public class GrpcCacheClient implements RemoteCacheClient, MissingDigestsFinder private final AtomicBoolean closed = new AtomicBoolean(); boolean shouldVerifyDownloads() { - return options.getRemoteVerifyDownloads(); + return options.remoteVerifyDownloads; } @VisibleForTesting