diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java index 138bad34eb40e2..5898a9a9895d3a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java @@ -41,6 +41,7 @@ import com.google.protobuf.ByteString; import com.google.protobuf.ExtensionRegistryLite; import java.io.FileNotFoundException; +import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; @@ -270,9 +271,7 @@ public void close() { public ListenableFuture uploadFile(Digest digest, Path file) { return executorService.submit( () -> { - try (InputStream in = file.getInputStream()) { - saveFile(digest, Store.CAS, in); - } + saveFile(digest, Store.CAS, file); return null; }); } @@ -313,6 +312,51 @@ public Path toPath(String hash, Store store) { } public void saveFile(Digest digest, Store store, InputStream in) throws IOException { + save( + digest, + store, + temp -> { + try (OutputStream out = temp.getOutputStream()) { + ByteStreams.copy(in, out); + // Fsync temp before we rename it to avoid data loss in the case of machine + // crashes (the OS may reorder the writes and the rename). + if (out instanceof FileOutputStream fos) { + fos.getFD().sync(); + } + } + }); + } + + /** + * Saves an existing file into the cache. + * + *

The contents are copied through {@link FileSystemUtils#copyFile}, so a filesystem with + * copy-on-write support (clonefile on macOS, copy_file_range on Linux) can serve the copy as a + * clone, leaving the entry sharing its blocks with the file it was saved from. + */ + private void saveFile(Digest digest, Store store, Path file) throws IOException { + save( + digest, + store, + temp -> { + FileSystemUtils.copyFile(file, temp); + // copyFile preserves the source's permissions and mtime, neither of which suits a cache + // entry: an entry must remain readable by every user of a shared cache, and its mtime + // records when it was last stored or retrieved. + temp.chmod(0644); + temp.setLastModifiedTime(Path.NOW_SENTINEL_TIME); + // Fsync temp before we rename it to avoid data loss in the case of machine + // crashes (the OS may reorder the writes and the rename). + syncFile(temp); + }); + } + + /** Writes the contents of a cache entry into a temporary file. */ + private interface TempFileWriter { + void write(Path temp) throws IOException; + } + + private void save(Digest digest, Store store, TempFileWriter writer) throws IOException { Path path = toPath(digest, store); // CAS entries are content-addressed and thus automatically have the correct content if they @@ -325,14 +369,7 @@ public void saveFile(Digest digest, Store store, InputStream in) throws IOExcept Path temp = getTempPath(); try { - try (OutputStream out = temp.getOutputStream()) { - ByteStreams.copy(in, out); - // Fsync temp before we rename it to avoid data loss in the case of machine - // crashes (the OS may reorder the writes and the rename). - if (out instanceof FileOutputStream fos) { - fos.getFD().sync(); - } - } + writer.write(temp); path.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.renameToleratingConcurrentCreation(temp, path); } catch (IOException e) { @@ -344,4 +381,13 @@ public void saveFile(Digest digest, Store store, InputStream in) throws IOExcept throw e; } } + + /** Flushes a file's contents to stable storage, where the filesystem supports it. */ + private static void syncFile(Path path) throws IOException { + try (InputStream in = path.getInputStream()) { + if (in instanceof FileInputStream fileInputStream) { + fileInputStream.getFD().sync(); + } + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java index e4289358278db4..0b9429b5099156 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java @@ -154,6 +154,26 @@ public void uploadFile_whenPresent_updatesMtime() throws Exception { assertThat(path.getLastModifiedTime()).isNotEqualTo(0); } + @Test + public void uploadFile_whenMissing_doesNotInheritSourceMtimeOrPermissions() throws Exception { + Path file = fs.getPath("/file"); + FileSystemUtils.writeContent(file, UTF_8, "contents"); + // A build output is read-only, and may have been written long before it is uploaded. Neither + // property may leak into the cache entry: the mtime records when the entry was last stored or + // retrieved, and the entry must remain readable by every user of a shared cache. + file.chmod(0555); + file.setLastModifiedTime(1000); + Digest digest = getDigest("contents"); + + var unused = getFromFuture(client.uploadFile(digest, file)); + + Path path = getCasPath(digest); + assertThat(FileSystemUtils.readContent(path, UTF_8)).isEqualTo("contents"); + assertThat(path.getLastModifiedTime()).isNotEqualTo(1000); + assertThat(path.isReadable()).isTrue(); + assertThat(path.isWritable()).isTrue(); + } + @Test public void uploadBlob_whenMissing_populatesCas() throws Exception { ByteString blob = ByteString.copyFromUtf8("contents");