[geode] branch feature/GEODE-2113e updated: found a bug in buffer expansion for SSL

[bs...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

bschuchardt pushed a commit to branch feature/GEODE-2113e
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/feature/GEODE-2113e by this push:
     new 0581d1e  found a bug in buffer expansion for SSL
0581d1e is described below

commit 0581d1e568b3cf79ebb0f1e1747712a59983d110
Author: Bruce Schuchardt <bs...@pivotal.io>
AuthorDate: Mon Jan 28 16:07:23 2019 -0800

    found a bug in buffer expansion for SSL
    
    added unit tests to cover the buffer expansion methods
---
 .../org/apache/geode/internal/net/Buffers.java     |  1 -
 .../org/apache/geode/internal/tcp/MsgReader.java   |  2 +-
 .../org/apache/geode/internal/net/BuffersTest.java | 30 ++++++++++++++++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/geode-core/src/main/java/org/apache/geode/internal/net/Buffers.java b/geode-core/src/main/java/org/apache/geode/internal/net/Buffers.java
index d1dc7b9..9db6e54 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/net/Buffers.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/net/Buffers.java
@@ -129,7 +129,6 @@ public class Buffers {
     }
     ByteBuffer newBuffer = acquireBuffer(type, desiredCapacity, stats);
     newBuffer.clear();
-    existing.flip();
     newBuffer.put(existing);
     newBuffer.flip();
     releaseBuffer(type, existing, stats);
diff --git a/geode-core/src/main/java/org/apache/geode/internal/tcp/MsgReader.java b/geode-core/src/main/java/org/apache/geode/internal/tcp/MsgReader.java
index 85c8d93..1512526 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/tcp/MsgReader.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/tcp/MsgReader.java
@@ -128,7 +128,7 @@ public class MsgReader {
   private void dumpState(String whereFrom, Throwable e, ByteBuffer inputBuffer, int position,
       int limit) {
     logger.info("BRUCE: {}, PID {} Connection to {}, {} SSL", whereFrom, OSProcess.getId(),
-        conn.getRemoteAddress(), conn.getConduit().useSSL());
+        conn.getRemoteAddress(), conn.getConduit().useSSL() ? "with" : "without");
     logger.info("BRUCE: {}, Message length {}; type {}; id {}",
         whereFrom, header.messageLength, header.messageType, header.messageId);
     logger.info(
diff --git a/geode-core/src/test/java/org/apache/geode/internal/net/BuffersTest.java b/geode-core/src/test/java/org/apache/geode/internal/net/BuffersTest.java
index 65f8581..3e736a0 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/net/BuffersTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/net/BuffersTest.java
@@ -15,6 +15,7 @@
 
 package org.apache.geode.internal.net;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.mockito.Mockito.mock;
 
@@ -72,4 +73,33 @@ public class BuffersTest {
       assertEquals(expected, actual);
     }
   }
+
+
+  // the fixed numbers in this test came from a distributed unit test failure
+  @Test
+  public void bufferPositionAndLimitForReadAreCorrectAfterExpansion() throws Exception {
+    ByteBuffer buffer = ByteBuffer.allocate(33842);
+    buffer.position(7);
+    buffer.limit(16384);
+    ByteBuffer newBuffer = Buffers.expandReadBufferIfNeeded(Buffers.BufferType.UNTRACKED, buffer,
+        40899, mock(DMStats.class));
+    assertThat(newBuffer.capacity()).isGreaterThanOrEqualTo(40899);
+    // buffer should be ready to read the same amount of data
+    assertThat(newBuffer.position()).isEqualTo(0);
+    assertThat(newBuffer.limit()).isEqualTo(16384 - 7);
+  }
+
+
+  @Test
+  public void bufferPositionAndLimitForWriteAreCorrectAfterExpansion() throws Exception {
+    ByteBuffer buffer = ByteBuffer.allocate(33842);
+    buffer.position(16384);
+    buffer.limit(buffer.capacity());
+    ByteBuffer newBuffer = Buffers.expandWriteBufferIfNeeded(Buffers.BufferType.UNTRACKED, buffer,
+        40899, mock(DMStats.class));
+    assertThat(newBuffer.capacity()).isGreaterThanOrEqualTo(40899);
+    // buffer should have the same amount of data as the old one
+    assertThat(newBuffer.position()).isEqualTo(16384);
+    assertThat(newBuffer.limit()).isEqualTo(newBuffer.capacity());
+  }
 }