google-cloud-nio: CloudStorageReadChannel: fix channel position overflow (#2283)

droazen · michaelbausor · commit 4055f1ffeb78 · 2017-08-24T08:29:52.000-07:00
This fixes a critical bug in CloudStorageReadChannel in which the channel position
(a long) was getting down-cast to int in CloudStorageReadChannel.innerOpen(). This
could cause the channel position to become negative or a wildly incorrect positive
value if the channel position was greater than MAX_INT.

In practice, this bug would only manifest itself if the channel was constructed with
a non-zero initial position, or if the channel was reopened in response to an error.

Included a test case that fails without the fix, and passes with the fix.
diff --git a/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java b/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java
@@ -82,7 +82,7 @@ private CloudStorageReadChannel(Storage gcsStorage, BlobId file, long position,
   private void innerOpen() throws IOException {
     this.channel = gcsStorage.reader(file);
     if (position > 0) {
-      channel.seek((int) position);
+      channel.seek(position);
     }
   }
 
diff --git a/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java b/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java
@@ -37,6 +37,7 @@
 import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
+import org.mockito.ArgumentCaptor;
 
 import javax.net.ssl.SSLHandshakeException;
 import java.io.IOException;
@@ -191,4 +192,25 @@ public void testSetPosition() throws IOException {
     verify(gcsChannel).seek(1);
     verify(gcsChannel, times(5)).isOpen();
   }
+
+  /*
+   * This test case was crafted in response to a bug in CloudStorageReadChannel in which the
+   * channel position (a long) was getting truncated to an int when seeking on the encapsulated
+   * ReadChannel in innerOpen(). This test case fails when the bad long -> int cast is present,
+   * and passes when it's removed.
+   */
+  @Test
+  public void testChannelPositionDoesNotGetTruncatedToInt() throws IOException {
+    // This position value will overflow to a negative value if a long -> int cast is attempted
+    long startPosition = 11918483280L;
+    ArgumentCaptor<Long> captor = ArgumentCaptor.forClass(Long.class);
+
+    // Invoke CloudStorageReadChannel.create() to trigger a call to the private
+    // CloudStorageReadChannel.innerOpen() method, which does a seek on our gcsChannel.
+    CloudStorageReadChannel.create(gcsStorage, file, startPosition, 1);
+
+    // Confirm that our position did not overflow during the seek in CloudStorageReadChannel.innerOpen()
+    verify(gcsChannel).seek(captor.capture());
+    assertThat(captor.getValue()).isEqualTo(startPosition);
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,7 @@ private CloudStorageReadChannel(Storage gcsStorage, BlobId file, long position,`
`82`	`82`	`private void innerOpen() throws IOException {`
`83`	`83`	`this.channel = gcsStorage.reader(file);`
`84`	`84`	`if (position > 0) {`
`85`		`- channel.seek((int) position);`
	`85`	`+ channel.seek(position);`
`86`	`86`	`}`
`87`	`87`	`}`
`88`	`88`