commit b35c3d072ef8a1b18a96e669a289f5fa28d78c8b
author George Gensure <ggensure@uber.com> Tue Jan 14 02:57:52 2020 -0800
committer Philipp Wollermann <philwo@google.com> Thu Feb 13 11:06:08 2020 +0100
tree 7f614c810ad5f6cdfd7e0b33f723adb39fe8eb56
parent 0f4c498a270f05b3896d57055b6489e824821eda

Prevent NPE on backwards seek on Chunker

Ensure that a reset() is not immediately followed by a data member
access with a call to maybeInitialize(). Practically, this can occur if
an offset has progressed locally, but the remote indicates a non-0
committed size, and would always have thrown an NPE on a requisite seek,
proven with the accompanied test case.

Closes #10566.

PiperOrigin-RevId: 289615198

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

diff --git a/src/main/java/com/google/devtools/build/lib/remote/Chunker.java b/src/main/java/com/google/devtools/build/lib/remote/Chunker.java
index 8904e59..7ce80ee 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/Chunker.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/Chunker.java
@@ -139,18 +139,14 @@
   /**
    * Seek to an offset, if necessary resetting or initializing
    *
-   * <p>Closes any open resources (file handles, ...).
+   * <p>May close open resources in order to seek to an earlier offset.
    */
   public void seek(long toOffset) throws IOException {
-    maybeInitialize();
     if (toOffset < offset) {
       reset();
-      if (toOffset != 0) {
-        data.skip(toOffset);
-      }
-    } else if (offset != toOffset) {
-      data.skip(toOffset - offset);
     }
+    maybeInitialize();
+    ByteStreams.skipFully(data, toOffset - offset);
     offset = toOffset;
   }
 

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

diff --git a/src/test/java/com/google/devtools/build/lib/remote/ChunkerTest.java b/src/test/java/com/google/devtools/build/lib/remote/ChunkerTest.java
index 7fd0375..bb07311 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/ChunkerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/ChunkerTest.java
@@ -159,6 +159,20 @@
     assertThat(next.getData()).hasSize(8);
   }
 
+  @Test
+  public void seekBackwards() throws IOException {
+    byte[] data = new byte[10];
+    Chunker chunker = Chunker.builder().setInput(data).setChunkSize(10).build();
+
+    chunker.seek(4);
+    chunker.seek(2);
+
+    Chunk next = chunker.next();
+    assertThat(next).isNotNull();
+    assertThat(next.getOffset()).isEqualTo(2);
+    assertThat(next.getData()).hasSize(8);
+  }
+
   private void assertNextEquals(Chunker chunker, byte... data) throws IOException {
     assertThat(chunker.hasNext()).isTrue();
     ByteString next = chunker.next().getData();