From 9a823d9dab82631ff4600eff38d597807e8d1221 Mon Sep 17 00:00:00 2001
From: George Gensure <ggensure@uber.com>
Date: Tue, 14 Jan 2020 02:57:52 -0800
Subject: [PATCH] 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
---
 .../google/devtools/build/lib/remote/Chunker.java  | 10 +++-------
 .../devtools/build/lib/remote/ChunkerTest.java     | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 7 deletions(-)

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 8904e59e20bd37..7ce80ee24b8294 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 @@ public void reset() throws IOException {
   /**
    * 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;
   }
 
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 7fd0375bf5ea44..bb07311ad3cbba 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 @@ public void seekAfterReset() throws IOException {
     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();