You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by bs...@apache.org on 2019/11/14 22:41:18 UTC
[geode] 01/01: GEODE-7450 SSL peerAppDataBuffer expansion needs work
This is an automated email from the ASF dual-hosted git repository.
bschuchardt pushed a commit to branch feature/GEODE-7450
in repository https://gitbox.apache.org/repos/asf/geode.git
commit 1f4331c7aed76c7476cdcff66d4e29028ff2b943
Author: Bruce Schuchardt <bs...@pivotal.io>
AuthorDate: Thu Nov 14 14:37:18 2019 -0800
GEODE-7450 SSL peerAppDataBuffer expansion needs work
The expansion logic for the decrypted buffer in NioSslEngine was
expanding the buffer to twice its old capacity and wouldn't go
beyond that. Some ciphers will compress data a lot more than that, so
we need to continually increase available space in the decryption buffer
if the SslEngine reports a BUFFER_OVERFLOW status. The changes in
this commit do that by always doubling the available space in the
decryption buffer in response to a BUFFER_OVERFLOW.
Along with that I found it unnecessary to take preemptive action to
increase the size of the buffer and deleted that code.
---
.../apache/geode/internal/net/NioSslEngine.java | 20 +++++-----------
.../geode/internal/net/NioSslEngineTest.java | 28 ++++++++++++++++------
.../cache/tier/sockets/DurableClientTestCase.java | 2 --
3 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java b/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java
index e914847..7756dcf 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java
@@ -267,14 +267,17 @@ public class NioSslEngine implements NioFilter {
// transmit large payloads and we may have read a partial chunk
// during the previous unwrap
- // it's better to be pro-active about avoiding buffer overflows
- expandPeerAppData(wrappedBuffer);
peerAppData.limit(peerAppData.capacity());
while (wrappedBuffer.hasRemaining()) {
SSLEngineResult unwrapResult = engine.unwrap(wrappedBuffer, peerAppData);
switch (unwrapResult.getStatus()) {
case BUFFER_OVERFLOW:
- expandPeerAppData(wrappedBuffer);
+ // buffer overflow expand and try again - double the available decryption space
+ int newCapacity =
+ (peerAppData.capacity() - peerAppData.position()) * 2 + peerAppData.position();
+ peerAppData =
+ bufferPool.expandWriteBufferIfNeeded(TRACKED_RECEIVER, peerAppData, newCapacity);
+ peerAppData.limit(peerAppData.capacity());
break;
case BUFFER_UNDERFLOW:
// partial data - need to read more. When this happens the SSLEngine will not have
@@ -291,14 +294,6 @@ public class NioSslEngine implements NioFilter {
return peerAppData;
}
- void expandPeerAppData(ByteBuffer wrappedBuffer) {
- if (peerAppData.capacity() - peerAppData.position() < 2 * wrappedBuffer.remaining()) {
- peerAppData =
- bufferPool.expandWriteBufferIfNeeded(TRACKED_RECEIVER, peerAppData,
- expandedCapacity(wrappedBuffer, peerAppData));
- }
- }
-
@Override
public ByteBuffer ensureWrappedCapacity(int amount, ByteBuffer wrappedBuffer,
BufferType bufferType) {
@@ -321,9 +316,6 @@ public class NioSslEngine implements NioFilter {
peerAppData.compact();
peerAppData.flip();
}
- } else {
- peerAppData =
- bufferPool.expandReadBufferIfNeeded(TRACKED_RECEIVER, peerAppData, bytes);
}
while (peerAppData.remaining() < bytes) {
diff --git a/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java b/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java
index e50b878..46551b7 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java
@@ -254,7 +254,10 @@ public class NioSslEngineTest {
@Test
public void unwrapWithBufferOverflow() throws Exception {
// make the application data too big to fit into the engine's encryption buffer
- ByteBuffer wrappedData = ByteBuffer.allocate(nioSslEngine.peerAppData.capacity() + 100);
+ int originalPeerAppDataCapacity = nioSslEngine.peerAppData.capacity();
+ int originalPeerAppDataPosition = originalPeerAppDataCapacity / 2;
+ nioSslEngine.peerAppData.position(originalPeerAppDataPosition);
+ ByteBuffer wrappedData = ByteBuffer.allocate(originalPeerAppDataCapacity + 100);
byte[] netBytes = new byte[wrappedData.capacity()];
Arrays.fill(netBytes, (byte) 0x1F);
wrappedData.put(netBytes);
@@ -265,16 +268,22 @@ public class NioSslEngineTest {
spyNioSslEngine.engine = testEngine;
testEngine.addReturnResult(
- new SSLEngineResult(BUFFER_OVERFLOW, NEED_UNWRAP, netBytes.length, netBytes.length),
+ new SSLEngineResult(BUFFER_OVERFLOW, NEED_UNWRAP, 0, 0), // results in 30,000 byte buffer
+ new SSLEngineResult(BUFFER_OVERFLOW, NEED_UNWRAP, 0, 0), // 50,000 bytes
+ new SSLEngineResult(BUFFER_OVERFLOW, NEED_UNWRAP, 0, 0), // 90,000 bytes
new SSLEngineResult(OK, FINISHED, netBytes.length, netBytes.length));
+ int expectedCapacity = 2 * originalPeerAppDataCapacity - originalPeerAppDataPosition;
+ expectedCapacity =
+ 2 * (expectedCapacity - originalPeerAppDataPosition) + originalPeerAppDataPosition;
+ expectedCapacity =
+ 2 * (expectedCapacity - originalPeerAppDataPosition) + originalPeerAppDataPosition;
ByteBuffer unwrappedBuffer = spyNioSslEngine.unwrap(wrappedData);
unwrappedBuffer.flip();
-
- verify(spyNioSslEngine, times(2)).expandPeerAppData(any(ByteBuffer.class));
- assertThat(unwrappedBuffer).isEqualTo(ByteBuffer.wrap(netBytes));
+ assertThat(unwrappedBuffer.capacity()).isEqualTo(expectedCapacity);
}
+
@Test
public void unwrapWithBufferUnderflow() throws Exception {
ByteBuffer wrappedData = ByteBuffer.allocate(nioSslEngine.peerAppData.capacity());
@@ -434,7 +443,8 @@ public class NioSslEngineTest {
SocketChannel mockChannel = mock(SocketChannel.class);
// force buffer expansion by making a small decoded buffer appear near to being full
- ByteBuffer unwrappedBuffer = ByteBuffer.allocate(100);
+ int initialUnwrappedBufferSize = 100;
+ ByteBuffer unwrappedBuffer = ByteBuffer.allocate(initialUnwrappedBufferSize);
unwrappedBuffer.position(7).limit(preexistingBytes + 7); // 7 bytes of message header - ignored
nioSslEngine.peerAppData = unwrappedBuffer;
@@ -450,9 +460,9 @@ public class NioSslEngineTest {
TestSSLEngine testSSLEngine = new TestSSLEngine();
testSSLEngine.addReturnResult(
+ new SSLEngineResult(BUFFER_OVERFLOW, NEED_UNWRAP, 0, 0), // expand 100 bytes to 93*2+7
new SSLEngineResult(OK, NEED_UNWRAP, 0, 0), // 10 + 60 bytes = 70
new SSLEngineResult(OK, NEED_UNWRAP, 0, 0), // 70 + 60 bytes = 130
- new SSLEngineResult(BUFFER_OVERFLOW, NEED_UNWRAP, 0, 0), // need 190 bytes capacity
new SSLEngineResult(OK, NEED_UNWRAP, 0, 0)); // 130 + 60 bytes = 190
nioSslEngine.engine = testSSLEngine;
@@ -460,6 +470,10 @@ public class NioSslEngineTest {
verify(mockChannel, times(3)).read(isA(ByteBuffer.class));
assertThat(data.position()).isEqualTo(0);
assertThat(data.limit()).isEqualTo(individualRead * 3 + preexistingBytes);
+ // The initial available space in the unwrapped buffer should have doubled
+ int initialFreeSpace = initialUnwrappedBufferSize - preexistingBytes;
+ assertThat(nioSslEngine.peerAppData.capacity())
+ .isEqualTo(2 * initialFreeSpace + preexistingBytes);
}
diff --git a/geode-cq/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientTestCase.java b/geode-cq/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientTestCase.java
index d5cbf1b..353c39f 100755
--- a/geode-cq/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientTestCase.java
+++ b/geode-cq/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientTestCase.java
@@ -107,8 +107,6 @@ public class DurableClientTestCase extends DurableClientTestBase {
// Verify that it is durable and its properties are correct
assertThat(proxy.isDurable()).isTrue();
- System.out.println("BRUCE: durableClientId is " + durableClientId);
- System.out.println("BRUCE: proxy durable id is " + proxy.getDurableId());
assertThat(durableClientId).isNotEqualTo(proxy.getDurableId());
/*