From ea7f6efd98146fa8aec42dcec83406e1676aba39 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 3 Jan 2025 01:59:17 -0800 Subject: [PATCH] Detect file reads that are longer than expected When Bazel knows the size of a file, it can also detect whether it would go past this size while reading the file. This is virtually free due to using a buffered stream and can catch cases of concurrent modifications or even bugs in Bazel. Work towards #24694 Closes #24708. PiperOrigin-RevId: 711678485 Change-Id: I85093c0d54dd365b1c301b6036f150dae3552c2a --- .../build/lib/vfs/FileSystemUtils.java | 49 +++++++++++-------- .../build/lib/vfs/FileSystemUtilsTest.java | 22 ++++----- 2 files changed, 40 insertions(+), 31 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java index 33c80637874c7a..93da2c0689c043 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java @@ -843,21 +843,6 @@ public static String readContent(Path inputFile, Charset charset) throws IOExcep return asByteSource(inputFile).asCharSource(charset).read(); } - /** - * Reads at most {@code limit} bytes from {@code inputFile} and returns it as a byte array. - * - * @throws IOException if there was an error. - */ - public static byte[] readContentWithLimit(Path inputFile, int limit) throws IOException { - Preconditions.checkArgument(limit >= 0, "limit needs to be >=0, but it is %s", limit); - ByteSource byteSource = asByteSource(inputFile); - byte[] buffer = new byte[limit]; - try (InputStream inputStream = byteSource.openBufferedStream()) { - int read = ByteStreams.read(inputStream, buffer, 0, limit); - return read == limit ? buffer : Arrays.copyOf(buffer, read); - } - } - /** * The type of {@link IOException} thrown by {@link #readWithKnownFileSize} when fewer bytes than * expected are read. @@ -876,23 +861,47 @@ private ShortReadIOException(Path path, int fileSize, int numBytesRead) { } } + /** + * The type of {@link IOException} thrown by {@link #readWithKnownFileSize} when more bytes than + * expected could be read. + */ + public static class LongReadIOException extends IOException { + public final Path path; + public final int fileSize; + + private LongReadIOException(Path path, int fileSize) { + super("File '" + path + "' is unexpectedly longer than " + fileSize + " bytes)"); + this.path = path; + this.fileSize = fileSize; + } + } + /** * Reads the given file {@code path}, assumed to have size {@code fileSize}, and does a check on * the number of bytes read. * *

Use this method when you already know the size of the file. The check is intended to catch - * issues where filesystems incorrectly truncate files. + * issues where the filesystem incorrectly returns truncated file contents, or where an external + * modification has concurrently truncated or appended to the file. * * @throws IOException if there was an error, or if fewer than {@code fileSize} bytes were read. */ public static byte[] readWithKnownFileSize(Path path, long fileSize) throws IOException { + Preconditions.checkArgument(fileSize >= 0, "fileSize needs to be >=0, but it is %s", fileSize); if (fileSize > Integer.MAX_VALUE) { throw new IOException("Cannot read file with size larger than 2GB"); } - int fileSizeInt = (int) fileSize; - byte[] bytes = readContentWithLimit(path, fileSizeInt); - if (fileSizeInt > bytes.length) { - throw new ShortReadIOException(path, fileSizeInt, bytes.length); + int size = (int) fileSize; + byte[] bytes = new byte[size]; + try (InputStream in = asByteSource(path).openBufferedStream()) { + int read = ByteStreams.read(in, bytes, 0, size); + if (read != size) { + throw new ShortReadIOException(path, size, read); + } + int eof = in.read(); + if (eof != -1) { + throw new LongReadIOException(path, size); + } } return bytes; } diff --git a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java index 7cb1ea2edf66a8..79ee70cce087f0 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java @@ -33,7 +33,6 @@ import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.FileNotFoundException; import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Collection; import org.junit.Before; @@ -407,18 +406,19 @@ public void testMoveFileFixPermissions() throws Exception { } @Test - public void testReadContentWithLimit() throws IOException { + public void testReadWithKnownFileSize() throws IOException { createTestDirectoryTree(); String str = "this is a test of readContentWithLimit method"; - FileSystemUtils.writeContent(file1, StandardCharsets.ISO_8859_1, str); - assertThat(readStringFromFile(file1, 0)).isEmpty(); - assertThat(str.substring(0, 10)).isEqualTo(readStringFromFile(file1, 10)); - assertThat(str).isEqualTo(readStringFromFile(file1, 1000000)); - } - - private String readStringFromFile(Path file, int limit) throws IOException { - byte[] bytes = FileSystemUtils.readContentWithLimit(file, limit); - return new String(bytes, StandardCharsets.ISO_8859_1); + FileSystemUtils.writeContent(file1, ISO_8859_1, str); + + assertThat(FileSystemUtils.readWithKnownFileSize(file1, str.length())) + .isEqualTo(str.getBytes(ISO_8859_1)); + assertThrows( + FileSystemUtils.LongReadIOException.class, + () -> FileSystemUtils.readWithKnownFileSize(file1, str.length() - 1)); + assertThrows( + FileSystemUtils.ShortReadIOException.class, + () -> FileSystemUtils.readWithKnownFileSize(file1, str.length() + 1)); } @Test