From c8b11e3aabb3cbdea3977128d0522a9d602021fe Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Wed, 29 Oct 2025 09:08:32 +0000 Subject: [PATCH 1/6] Improve SSLEngine.wrap precondition tests. --- .../javax/net/ssl/SSLEngineTest.java | 129 ++++-------------- 1 file changed, 30 insertions(+), 99 deletions(-) diff --git a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java index c49648955..0297292e8 100644 --- a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java +++ b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java @@ -22,6 +22,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -933,115 +934,45 @@ public void wrapPreconditions() throws Exception { ByteBuffer[] badBuffers = new ByteBuffer[] { buffer, buffer, null, buffer }; // Client/server mode not set => IllegalStateException - try { - newUnconnectedEngine().wrap(buffer, buffer); - fail(); - } catch (IllegalStateException e) { - // Expected - } - - try { - newUnconnectedEngine().wrap(buffers, buffer); - fail(); - } catch (IllegalStateException e) { - // Expected - } - - try { - newUnconnectedEngine().wrap(buffers, 0, 1, buffer); - fail(); - } catch (IllegalStateException e) { - // Expected - } + assertThrows( + IllegalStateException.class, () -> newUnconnectedEngine().wrap(buffer, buffer)); + assertThrows( + IllegalStateException.class, () -> newUnconnectedEngine().wrap(buffers, buffer)); + assertThrows(IllegalStateException.class, + () -> newUnconnectedEngine().wrap(buffers, 0, 1, buffer)); // Read-only destination => ReadOnlyBufferException - try { - newConnectedEngine().wrap(buffer, buffer.asReadOnlyBuffer()); - fail(); - } catch (ReadOnlyBufferException e) { - // Expected - } - - try { - newConnectedEngine().wrap(buffers, buffer.asReadOnlyBuffer()); - fail(); - } catch (ReadOnlyBufferException e) { - // Expected - } - - try { - newConnectedEngine().wrap(buffers, 0, 1, buffer.asReadOnlyBuffer()); - fail(); - } catch (ReadOnlyBufferException e) { - // Expected - } + assertThrows(ReadOnlyBufferException.class, + () -> newConnectedEngine().wrap(buffer, buffer.asReadOnlyBuffer())); + assertThrows(ReadOnlyBufferException.class, + () -> newConnectedEngine().wrap(buffers, buffer.asReadOnlyBuffer())); + assertThrows(ReadOnlyBufferException.class, + () -> newConnectedEngine().wrap(buffers, 0, 1, buffer.asReadOnlyBuffer())); // Null destination => IllegalArgumentException - try { - newConnectedEngine().wrap(buffer, null); - fail(); - } catch (IllegalArgumentException e) { - // Expected - } - - try { - newConnectedEngine().wrap(buffers, null); - fail(); - } catch (IllegalArgumentException e) { - // Expected - } - - try { - newConnectedEngine().wrap(buffers, 0, 1, null); - fail(); - } catch (IllegalArgumentException e) { - // Expected - } + assertThrows(IllegalArgumentException.class, () -> newConnectedEngine().wrap(buffer, null)); + assertThrows( + IllegalArgumentException.class, () -> newConnectedEngine().wrap(buffers, null)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().wrap(buffers, 0, 1, null)); // Null source => IllegalArgumentException - try { - newConnectedEngine().wrap((ByteBuffer) null, buffer); - fail(); - } catch (IllegalArgumentException e) { - // Expected - } - - try { - newConnectedEngine().wrap((ByteBuffer[]) null, buffer); - fail(); - } catch (IllegalArgumentException e) { - // Expected - } - - try { - newConnectedEngine().wrap(null, 0, 1, buffer); - fail(); - } catch (IllegalArgumentException e) { - // Expected - } + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().wrap((ByteBuffer) null, buffer)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().wrap((ByteBuffer[]) null, buffer)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().wrap(null, 0, 1, buffer)); // Null entries in buffer array => IllegalArgumentException - try { - newConnectedEngine().wrap(badBuffers, buffer); - fail(); - } catch (IllegalArgumentException e) { - // Expected - } - - try { - newConnectedEngine().wrap(badBuffers, 0, badBuffers.length, buffer); - fail(); - } catch (IllegalArgumentException e) { - // Expected - } + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().wrap(badBuffers, buffer)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().wrap(badBuffers, 0, badBuffers.length, buffer)); // Bad offset or length => IndexOutOfBoundsException - try { - newConnectedEngine().wrap(buffers, 0, 7, buffer); - fail(); - } catch (IndexOutOfBoundsException e) { - // Expected - } + assertThrows(IndexOutOfBoundsException.class, + () -> newConnectedEngine().wrap(buffers, 0, 7, buffer)); } @Test From ba8fa2d848f8b44399c79e2bb6fe5c81c196b1bd Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Wed, 29 Oct 2025 09:15:43 +0000 Subject: [PATCH 2/6] Add missing tests --- .../test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java index 0297292e8..7b277dd49 100644 --- a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java +++ b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java @@ -969,10 +969,15 @@ public void wrapPreconditions() throws Exception { () -> newConnectedEngine().wrap(badBuffers, buffer)); assertThrows(IllegalArgumentException.class, () -> newConnectedEngine().wrap(badBuffers, 0, badBuffers.length, buffer)); + // But not if they are outside the selected offset and length + newConnectedEngine().wrap(badBuffers, 0, 2, buffer); + newConnectedEngine().wrap(badBuffers, 3, 1, buffer); // Bad offset or length => IndexOutOfBoundsException assertThrows(IndexOutOfBoundsException.class, () -> newConnectedEngine().wrap(buffers, 0, 7, buffer)); + assertThrows(IndexOutOfBoundsException.class, + () -> newConnectedEngine().wrap(buffers, 3, 1, buffer)); } @Test From 6ba7aaf54bad6ccd5233fda2c96c0cf47a562b0e Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Wed, 29 Oct 2025 09:38:39 +0000 Subject: [PATCH 3/6] Parameterise tests better. --- .../javax/net/ssl/SSLEngineTest.java | 29 +++++++++++-------- .../main/java/org/conscrypt/TestUtils.java | 8 +++++ 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java index 7b277dd49..8a2f67136 100644 --- a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java +++ b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java @@ -929,9 +929,14 @@ public void test_SSLEngine_setSSLParameters() throws Exception { @Test public void wrapPreconditions() throws Exception { - ByteBuffer buffer = ByteBuffer.allocate(10); - ByteBuffer[] buffers = new ByteBuffer[] { buffer, buffer, buffer }; - ByteBuffer[] badBuffers = new ByteBuffer[] { buffer, buffer, null, buffer }; + int bufferSize = 128; + int arrayLength = 5; + ByteBuffer buffer = ByteBuffer.allocate(bufferSize); + ByteBuffer readOnlyBuffer = buffer.asReadOnlyBuffer(); + ByteBuffer[] buffers = BufferType.HEAP.newRandomBufferArray(arrayLength, bufferSize); + ByteBuffer[] badBuffers = Arrays.copyOf(buffers, buffers.length); + int nullBufferIndex = 2; + badBuffers[nullBufferIndex] = null; // Client/server mode not set => IllegalStateException assertThrows( @@ -943,18 +948,18 @@ public void wrapPreconditions() throws Exception { // Read-only destination => ReadOnlyBufferException assertThrows(ReadOnlyBufferException.class, - () -> newConnectedEngine().wrap(buffer, buffer.asReadOnlyBuffer())); + () -> newConnectedEngine().wrap(buffer, readOnlyBuffer)); assertThrows(ReadOnlyBufferException.class, - () -> newConnectedEngine().wrap(buffers, buffer.asReadOnlyBuffer())); + () -> newConnectedEngine().wrap(buffers, readOnlyBuffer)); assertThrows(ReadOnlyBufferException.class, - () -> newConnectedEngine().wrap(buffers, 0, 1, buffer.asReadOnlyBuffer())); + () -> newConnectedEngine().wrap(buffers, 0, arrayLength, readOnlyBuffer)); // Null destination => IllegalArgumentException assertThrows(IllegalArgumentException.class, () -> newConnectedEngine().wrap(buffer, null)); assertThrows( IllegalArgumentException.class, () -> newConnectedEngine().wrap(buffers, null)); assertThrows(IllegalArgumentException.class, - () -> newConnectedEngine().wrap(buffers, 0, 1, null)); + () -> newConnectedEngine().wrap(buffers, 0, arrayLength, null)); // Null source => IllegalArgumentException assertThrows(IllegalArgumentException.class, @@ -968,16 +973,16 @@ public void wrapPreconditions() throws Exception { assertThrows(IllegalArgumentException.class, () -> newConnectedEngine().wrap(badBuffers, buffer)); assertThrows(IllegalArgumentException.class, - () -> newConnectedEngine().wrap(badBuffers, 0, badBuffers.length, buffer)); + () -> newConnectedEngine().wrap(badBuffers, 0, arrayLength, buffer)); // But not if they are outside the selected offset and length - newConnectedEngine().wrap(badBuffers, 0, 2, buffer); - newConnectedEngine().wrap(badBuffers, 3, 1, buffer); + newConnectedEngine().wrap(badBuffers, 0, nullBufferIndex, buffer); + newConnectedEngine().wrap(badBuffers, nullBufferIndex + 1, 1, buffer); // Bad offset or length => IndexOutOfBoundsException assertThrows(IndexOutOfBoundsException.class, - () -> newConnectedEngine().wrap(buffers, 0, 7, buffer)); + () -> newConnectedEngine().wrap(buffers, 0, arrayLength + 1, buffer)); assertThrows(IndexOutOfBoundsException.class, - () -> newConnectedEngine().wrap(buffers, 3, 1, buffer)); + () -> newConnectedEngine().wrap(buffers, arrayLength, 1, buffer)); } @Test diff --git a/testing/src/main/java/org/conscrypt/TestUtils.java b/testing/src/main/java/org/conscrypt/TestUtils.java index c239c0aa3..f62a23466 100644 --- a/testing/src/main/java/org/conscrypt/TestUtils.java +++ b/testing/src/main/java/org/conscrypt/TestUtils.java @@ -118,6 +118,14 @@ public ByteBuffer[] newRandomBuffers(int... sizes) { return result; } + public ByteBuffer[] newRandomBufferArray(int arrayLength, int bufferSize) { + ByteBuffer[] result = new ByteBuffer[arrayLength]; + for (int i = 0; i < arrayLength; i++) { + result[i] = newRandomBuffer(bufferSize); + } + return result; + } + public ByteBuffer newRandomBuffer(int size) { byte[] data = new byte[size]; random.nextBytes(data); From bcc368e46845b071edbcc505ecf735474e1a5022 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Wed, 29 Oct 2025 10:32:45 +0000 Subject: [PATCH 4/6] Rename variable. --- .../org/conscrypt/javax/net/ssl/SSLEngineTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java index 8a2f67136..3f398a5fd 100644 --- a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java +++ b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java @@ -934,9 +934,9 @@ public void wrapPreconditions() throws Exception { ByteBuffer buffer = ByteBuffer.allocate(bufferSize); ByteBuffer readOnlyBuffer = buffer.asReadOnlyBuffer(); ByteBuffer[] buffers = BufferType.HEAP.newRandomBufferArray(arrayLength, bufferSize); - ByteBuffer[] badBuffers = Arrays.copyOf(buffers, buffers.length); + ByteBuffer[] buffersWithNullEntry = Arrays.copyOf(buffers, buffers.length); int nullBufferIndex = 2; - badBuffers[nullBufferIndex] = null; + buffersWithNullEntry[nullBufferIndex] = null; // Client/server mode not set => IllegalStateException assertThrows( @@ -971,12 +971,12 @@ public void wrapPreconditions() throws Exception { // Null entries in buffer array => IllegalArgumentException assertThrows(IllegalArgumentException.class, - () -> newConnectedEngine().wrap(badBuffers, buffer)); + () -> newConnectedEngine().wrap(buffersWithNullEntry, buffer)); assertThrows(IllegalArgumentException.class, - () -> newConnectedEngine().wrap(badBuffers, 0, arrayLength, buffer)); + () -> newConnectedEngine().wrap(buffersWithNullEntry, 0, arrayLength, buffer)); // But not if they are outside the selected offset and length - newConnectedEngine().wrap(badBuffers, 0, nullBufferIndex, buffer); - newConnectedEngine().wrap(badBuffers, nullBufferIndex + 1, 1, buffer); + newConnectedEngine().wrap(buffersWithNullEntry, 0, nullBufferIndex, buffer); + newConnectedEngine().wrap(buffersWithNullEntry, nullBufferIndex + 1, 1, buffer); // Bad offset or length => IndexOutOfBoundsException assertThrows(IndexOutOfBoundsException.class, From ff67dd3e7711af45b1ba5ca9707a0947ac4e7bd2 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Wed, 29 Oct 2025 10:34:45 +0000 Subject: [PATCH 5/6] Clang format. --- .../conscrypt/javax/net/ssl/SSLEngineTest.java | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java index 3f398a5fd..a4c074cd2 100644 --- a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java +++ b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java @@ -26,6 +26,14 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import org.conscrypt.TestUtils; +import org.conscrypt.TestUtils.BufferType; +import org.conscrypt.java.security.StandardNames; +import org.conscrypt.java.security.TestKeyStore; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + import java.io.IOException; import java.net.Socket; import java.nio.ByteBuffer; @@ -36,6 +44,7 @@ import java.util.HashSet; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; + import javax.crypto.SecretKey; import javax.crypto.spec.SecretKeySpec; import javax.net.ssl.KeyManager; @@ -50,13 +59,6 @@ import javax.net.ssl.SSLSession; import javax.net.ssl.X509ExtendedKeyManager; import javax.net.ssl.X509ExtendedTrustManager; -import org.conscrypt.TestUtils; -import org.conscrypt.TestUtils.BufferType; -import org.conscrypt.java.security.StandardNames; -import org.conscrypt.java.security.TestKeyStore; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class SSLEngineTest { From 1c13a724e79275787be55bf90164db1674df1618 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Wed, 29 Oct 2025 10:44:06 +0000 Subject: [PATCH 6/6] Extra tests for array bounds behaviour. --- .../java/org/conscrypt/javax/net/ssl/SSLEngineTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java index a4c074cd2..789133726 100644 --- a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java +++ b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java @@ -985,6 +985,12 @@ public void wrapPreconditions() throws Exception { () -> newConnectedEngine().wrap(buffers, 0, arrayLength + 1, buffer)); assertThrows(IndexOutOfBoundsException.class, () -> newConnectedEngine().wrap(buffers, arrayLength, 1, buffer)); + assertThrows(IndexOutOfBoundsException.class, + () -> newConnectedEngine().wrap(buffers, arrayLength - 1, 2, buffer)); + newConnectedEngine().wrap(buffers, 0, arrayLength, buffer); + // Zero length array is allowed + newConnectedEngine().wrap(buffers, 0, 0, buffer); + newConnectedEngine().wrap(buffers, arrayLength, 0, buffer); } @Test