From ba9221ffae5f4f1ab1d3ed4695d16611c405bcc2 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Sun, 1 Mar 2026 18:19:42 +0000 Subject: [PATCH 1/3] SSLEngine.unwrap precondition tests. This is completely analagous to #1412 but for unwrap(). Does not include failing tests for #1372. --- .../javax/net/ssl/SSLEngineTest.java | 88 ++++++++++++++++++- .../main/java/org/conscrypt/TestUtils.java | 8 ++ 2 files changed, 95 insertions(+), 1 deletion(-) 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 d436a29de..2ce8c2821 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 @@ -1059,7 +1059,93 @@ private void assertWrapSucceeds(ByteBuffer[] buffers, int offset, int length) th } @Test - public void bufferArrayOffsets() throws Exception { + public void unwrapPreconditions() throws Exception { + int bufferSize = 128; + int arrayLength = 5; + ByteBuffer buffer = ByteBuffer.allocate(bufferSize); + ByteBuffer readOnlyBuffer = buffer.asReadOnlyBuffer(); + ByteBuffer[] buffers = BufferType.HEAP.newBufferArray(arrayLength, bufferSize); + ByteBuffer[] buffersWithNullEntry = Arrays.copyOf(buffers, buffers.length); + int nullBufferIndex = 2; + buffersWithNullEntry[nullBufferIndex] = null; + ByteBuffer[] buffersWithReadOnlyEntry = Arrays.copyOf(buffers, buffers.length); + int readOnlyBufferIndex = 2; + buffersWithReadOnlyEntry[readOnlyBufferIndex] = + buffersWithReadOnlyEntry[readOnlyBufferIndex].asReadOnlyBuffer(); + + // Client/server mode not set => IllegalStateException + assertThrows( + IllegalStateException.class, () -> newUnconnectedEngine().unwrap(buffer, buffer)); + assertThrows( + IllegalStateException.class, () -> newUnconnectedEngine().unwrap(buffer, buffers)); + assertThrows(IllegalStateException.class, + () -> newUnconnectedEngine().unwrap(buffer, buffers, 0, 1)); + + // Read-only destination => ReadOnlyBufferException + assertThrows(ReadOnlyBufferException.class, + () -> newConnectedEngine().unwrap(buffer, readOnlyBuffer)); + assertThrows(ReadOnlyBufferException.class, + () -> newConnectedEngine().unwrap(buffer, buffersWithReadOnlyEntry)); + assertThrows(ReadOnlyBufferException.class, + () + -> newConnectedEngine().unwrap( + buffer, buffersWithReadOnlyEntry, 0, arrayLength)); + + // Null destination => IllegalArgumentException + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(buffer, (ByteBuffer) null)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(buffer, (ByteBuffer[]) null)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(buffer, null, 0, 1)); + + // Null source => IllegalArgumentException + assertThrows( + IllegalArgumentException.class, () -> newConnectedEngine().unwrap(null, buffer)); + assertThrows( + IllegalArgumentException.class, () -> newConnectedEngine().unwrap(null, buffers)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(null, buffers, 0, 1)); + + // Null entries in buffer array => IllegalArgumentException + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(buffer, buffersWithNullEntry)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(buffer, buffersWithNullEntry, 0, arrayLength)); + + // Bad offset or length => IndexOutOfBoundsException + assertThrows(IndexOutOfBoundsException.class, + () -> newConnectedEngine().unwrap(buffer, buffers, 0, arrayLength + 1)); + assertThrows(IndexOutOfBoundsException.class, + () -> newConnectedEngine().unwrap(buffer, buffers, arrayLength, 1)); + wrapThenUnwrap(bufferSize, buffers, 0, arrayLength); + // Zero length array is allowed + wrapThenUnwrap(bufferSize, buffers, 0, 0); + wrapThenUnwrap(bufferSize, buffers, arrayLength, 0); + } + + private void wrapThenUnwrap(int bufferSize, ByteBuffer[] dest, int offset, int length) + throws Exception { + ByteBuffer src = ByteBuffer.allocate(bufferSize); + ByteBuffer tlsBuffer = ByteBuffer.allocate(bufferSize + 128); + tlsBuffer.clear(); + + try (TestSSLEnginePair pair = TestSSLEnginePair.create()) { + SSLEngineResult result = pair.client.wrap(src, tlsBuffer); + assertEquals(Status.OK, result.getStatus()); + + tlsBuffer.flip(); + for (int i = offset; i < offset + length; i++) { + dest[i].clear(); + } + // Unwrap result ignored because unwrap may not succeed (e.g. overflowing zero length + // buffer array), but it should not throw any exception due to preconditions + pair.server.unwrap(tlsBuffer, dest, offset, length); + } + } + + @Test + public void bufferArrayOffsets() throws Exception{ TestSSLEnginePair pair = TestSSLEnginePair.create(); ByteBuffer tlsBuffer = ByteBuffer.allocate(600); int bufferSize = 100; diff --git a/testing/src/main/java/org/conscrypt/TestUtils.java b/testing/src/main/java/org/conscrypt/TestUtils.java index aceb6a764..a02efd099 100644 --- a/testing/src/main/java/org/conscrypt/TestUtils.java +++ b/testing/src/main/java/org/conscrypt/TestUtils.java @@ -134,6 +134,14 @@ public ByteBuffer newRandomBuffer(int size) { buffer.flip(); return buffer; } + + public ByteBuffer[] newBufferArray(int arrayLength, int bufferSize) { + ByteBuffer[] result = new ByteBuffer[arrayLength]; + for (int i = 0; i < arrayLength; i++) { + result[i] = newBuffer(bufferSize); + } + return result; + } } private TestUtils() {} From c47f0f5ab46fc1fb12f566fdfc110791dcbfb582 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Sat, 28 Feb 2026 14:39:45 +0000 Subject: [PATCH 2/3] Tests for SSLEngine unwrap preconditions. Mirrors the tests for wrap preconditions in #1420. Does not contain any tests or fixes for bug #1372. They will follow in a smaller, focussed PR. --- .../javax/net/ssl/SSLEngineTest.java | 112 +++++++++++++++++- .../main/java/org/conscrypt/TestUtils.java | 16 +-- 2 files changed, 119 insertions(+), 9 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 2ce8c2821..4d9acd253 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 @@ -1058,6 +1058,116 @@ private void assertWrapSucceeds(ByteBuffer[] buffers, int offset, int length) th } } + /* + * The preconditions for the unwrap() methods of SSLEngine have a relatively + * complex mapping from precondition to the exception thrown on failure. This method + * checks that all the failure cases throw the correct exception and that + * corner cases which should not throw are also handled correctly. + */ + @Test + public void unwrapPreconditions() throws Exception { + int bufferSize = 128; + int arrayLength = 5; + ByteBuffer srcBuffer = BufferType.HEAP.newRandomBuffer(bufferSize); + ByteBuffer dstBuffer = ByteBuffer.allocate(bufferSize); + ByteBuffer readOnlyDestBuffer = dstBuffer.asReadOnlyBuffer(); + + ByteBuffer[] dsts = BufferType.HEAP.newBufferArray(arrayLength, bufferSize); + ByteBuffer[] dstsWithNullEntry = Arrays.copyOf(dsts, dsts.length); + int nullBufferIndex = 2; + dstsWithNullEntry[nullBufferIndex] = null; + ByteBuffer[] dstsWithReadOnlyEntry = Arrays.copyOf(dsts, dsts.length); + int readOnlyBufferIndex = 2; + dstsWithReadOnlyEntry[readOnlyBufferIndex] = + dstsWithReadOnlyEntry[readOnlyBufferIndex].asReadOnlyBuffer(); + + // Failure cases + // Client/server mode not set => IllegalStateException + assertThrows(IllegalStateException.class, + () -> newUnconnectedEngine().unwrap(srcBuffer, dstBuffer)); + assertThrows(IllegalStateException.class, + () -> newUnconnectedEngine().unwrap(srcBuffer, dsts)); + assertThrows(IllegalStateException.class, + () -> newUnconnectedEngine().unwrap(srcBuffer, dsts, 0, 1)); + + // Read-only destination => ReadOnlyBufferException + assertThrows(ReadOnlyBufferException.class, + () -> newConnectedEngine().unwrap(srcBuffer, readOnlyDestBuffer)); + assertThrows(ReadOnlyBufferException.class, + () -> newConnectedEngine().unwrap(srcBuffer, dstsWithReadOnlyEntry)); + assertThrows(ReadOnlyBufferException.class, + () -> newConnectedEngine().unwrap( + srcBuffer, dstsWithReadOnlyEntry, 0, arrayLength)); + + // Null destination => IllegalArgumentException + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(srcBuffer, (ByteBuffer) null)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(srcBuffer, (ByteBuffer[]) null)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(srcBuffer, null, 0, 1)); + + // Null source => IllegalArgumentException + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(null, dstBuffer)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(null, dsts)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(null, dsts, 0, 1)); + + // Null entries in destination buffer array => IllegalArgumentException + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap(srcBuffer, dstsWithNullEntry)); + assertThrows(IllegalArgumentException.class, + () -> newConnectedEngine().unwrap( + srcBuffer, dstsWithNullEntry, 0, arrayLength)); + + // Bad offset or length => IndexOutOfBoundsException + assertThrows(IndexOutOfBoundsException.class, + () -> newConnectedEngine().unwrap(srcBuffer, dsts, 0, arrayLength + 1)); + assertThrows(IndexOutOfBoundsException.class, + () -> newConnectedEngine().unwrap(srcBuffer, dsts, arrayLength, 1)); + assertThrows(IndexOutOfBoundsException.class, + () -> newConnectedEngine().unwrap(srcBuffer, dsts, arrayLength - 1, 2)); + + // Corner cases which should not throw + // Zero length arrays of output buffers should never throw + assertUnwrapDoesNotThrow(dsts, 0, 0); + assertUnwrapDoesNotThrow(dsts, arrayLength, 0); + + // TODO(prb): Add tests for null/read-only entries outside the selected offset and length + // after https://github.com/google/conscrypt/issues/1372 is fixed. + } + + // Asserts that an unwrap call with the given arguments does not throw a precondition + // exception. Note that the unwrap call itself may not produce plaintext if the selected + // destination buffers don't have enough capacity, but this is purely a precondition test + // and actual unwrap functionality is tested elsewhere. + private void assertUnwrapDoesNotThrow(ByteBuffer[] dsts, int offset, int length) throws Exception { + try (TestSSLEnginePair pair = TestSSLEnginePair.create()) { + assertConnected(pair); + + // Wrap some plaintext to get TLS ciphertext to unwrap. + ByteBuffer plaintext = BufferType.HEAP.newRandomBuffer(128); + ByteBuffer ciphertext = + ByteBuffer.allocate(pair.client.getSession().getPacketBufferSize()); + SSLEngineResult wrapResult = pair.client.wrap(plaintext, ciphertext); + assertEquals(Status.OK, wrapResult.getStatus()); + ciphertext.flip(); + + // Reset the selected destination buffers to their initial (empty) state. + for (int i = offset; i < offset + length; i++) { + dsts[i].clear(); + } + + // Unwrap: may return BUFFER_OVERFLOW if the selected buffers don't have enough capacity but that's + // fine for this *precondition* test. + SSLEngineResult result = pair.server.unwrap(ciphertext, dsts, offset, length); + assertTrue(result.getStatus() == Status.OK + || result.getStatus() == Status.BUFFER_OVERFLOW); + } + } + @Test public void unwrapPreconditions() throws Exception { int bufferSize = 128; @@ -1145,7 +1255,7 @@ private void wrapThenUnwrap(int bufferSize, ByteBuffer[] dest, int offset, int l } @Test - public void bufferArrayOffsets() throws Exception{ + public void bufferArrayOffsets() throws Exception { TestSSLEnginePair pair = TestSSLEnginePair.create(); ByteBuffer tlsBuffer = ByteBuffer.allocate(600); int bufferSize = 100; diff --git a/testing/src/main/java/org/conscrypt/TestUtils.java b/testing/src/main/java/org/conscrypt/TestUtils.java index a02efd099..f3b1f2268 100644 --- a/testing/src/main/java/org/conscrypt/TestUtils.java +++ b/testing/src/main/java/org/conscrypt/TestUtils.java @@ -109,6 +109,14 @@ ByteBuffer newBuffer(int size) { private static final Random random = new Random(System.currentTimeMillis()); abstract ByteBuffer newBuffer(int size); + public ByteBuffer[] newBufferArray(int arrayLength, int bufferSize) { + ByteBuffer[] result = new ByteBuffer[arrayLength]; + for (int i = 0; i < arrayLength; i++) { + result[i] = newBuffer(bufferSize); + } + return result; + } + public ByteBuffer[] newRandomBuffers(int... sizes) { int numBuffers = sizes.length; ByteBuffer[] result = new ByteBuffer[numBuffers]; @@ -134,14 +142,6 @@ public ByteBuffer newRandomBuffer(int size) { buffer.flip(); return buffer; } - - public ByteBuffer[] newBufferArray(int arrayLength, int bufferSize) { - ByteBuffer[] result = new ByteBuffer[arrayLength]; - for (int i = 0; i < arrayLength; i++) { - result[i] = newBuffer(bufferSize); - } - return result; - } } private TestUtils() {} From 00c39d0e60d666f09a5a7559ac3f219a951a2034 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Sun, 1 Mar 2026 18:25:09 +0000 Subject: [PATCH 3/3] Fix merge error. --- .../javax/net/ssl/SSLEngineTest.java | 86 ------------------- 1 file changed, 86 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 4d9acd253..b8db21437 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 @@ -1168,92 +1168,6 @@ private void assertUnwrapDoesNotThrow(ByteBuffer[] dsts, int offset, int length) } } - @Test - public void unwrapPreconditions() throws Exception { - int bufferSize = 128; - int arrayLength = 5; - ByteBuffer buffer = ByteBuffer.allocate(bufferSize); - ByteBuffer readOnlyBuffer = buffer.asReadOnlyBuffer(); - ByteBuffer[] buffers = BufferType.HEAP.newBufferArray(arrayLength, bufferSize); - ByteBuffer[] buffersWithNullEntry = Arrays.copyOf(buffers, buffers.length); - int nullBufferIndex = 2; - buffersWithNullEntry[nullBufferIndex] = null; - ByteBuffer[] buffersWithReadOnlyEntry = Arrays.copyOf(buffers, buffers.length); - int readOnlyBufferIndex = 2; - buffersWithReadOnlyEntry[readOnlyBufferIndex] = - buffersWithReadOnlyEntry[readOnlyBufferIndex].asReadOnlyBuffer(); - - // Client/server mode not set => IllegalStateException - assertThrows( - IllegalStateException.class, () -> newUnconnectedEngine().unwrap(buffer, buffer)); - assertThrows( - IllegalStateException.class, () -> newUnconnectedEngine().unwrap(buffer, buffers)); - assertThrows(IllegalStateException.class, - () -> newUnconnectedEngine().unwrap(buffer, buffers, 0, 1)); - - // Read-only destination => ReadOnlyBufferException - assertThrows(ReadOnlyBufferException.class, - () -> newConnectedEngine().unwrap(buffer, readOnlyBuffer)); - assertThrows(ReadOnlyBufferException.class, - () -> newConnectedEngine().unwrap(buffer, buffersWithReadOnlyEntry)); - assertThrows(ReadOnlyBufferException.class, - () - -> newConnectedEngine().unwrap( - buffer, buffersWithReadOnlyEntry, 0, arrayLength)); - - // Null destination => IllegalArgumentException - assertThrows(IllegalArgumentException.class, - () -> newConnectedEngine().unwrap(buffer, (ByteBuffer) null)); - assertThrows(IllegalArgumentException.class, - () -> newConnectedEngine().unwrap(buffer, (ByteBuffer[]) null)); - assertThrows(IllegalArgumentException.class, - () -> newConnectedEngine().unwrap(buffer, null, 0, 1)); - - // Null source => IllegalArgumentException - assertThrows( - IllegalArgumentException.class, () -> newConnectedEngine().unwrap(null, buffer)); - assertThrows( - IllegalArgumentException.class, () -> newConnectedEngine().unwrap(null, buffers)); - assertThrows(IllegalArgumentException.class, - () -> newConnectedEngine().unwrap(null, buffers, 0, 1)); - - // Null entries in buffer array => IllegalArgumentException - assertThrows(IllegalArgumentException.class, - () -> newConnectedEngine().unwrap(buffer, buffersWithNullEntry)); - assertThrows(IllegalArgumentException.class, - () -> newConnectedEngine().unwrap(buffer, buffersWithNullEntry, 0, arrayLength)); - - // Bad offset or length => IndexOutOfBoundsException - assertThrows(IndexOutOfBoundsException.class, - () -> newConnectedEngine().unwrap(buffer, buffers, 0, arrayLength + 1)); - assertThrows(IndexOutOfBoundsException.class, - () -> newConnectedEngine().unwrap(buffer, buffers, arrayLength, 1)); - wrapThenUnwrap(bufferSize, buffers, 0, arrayLength); - // Zero length array is allowed - wrapThenUnwrap(bufferSize, buffers, 0, 0); - wrapThenUnwrap(bufferSize, buffers, arrayLength, 0); - } - - private void wrapThenUnwrap(int bufferSize, ByteBuffer[] dest, int offset, int length) - throws Exception { - ByteBuffer src = ByteBuffer.allocate(bufferSize); - ByteBuffer tlsBuffer = ByteBuffer.allocate(bufferSize + 128); - tlsBuffer.clear(); - - try (TestSSLEnginePair pair = TestSSLEnginePair.create()) { - SSLEngineResult result = pair.client.wrap(src, tlsBuffer); - assertEquals(Status.OK, result.getStatus()); - - tlsBuffer.flip(); - for (int i = offset; i < offset + length; i++) { - dest[i].clear(); - } - // Unwrap result ignored because unwrap may not succeed (e.g. overflowing zero length - // buffer array), but it should not throw any exception due to preconditions - pair.server.unwrap(tlsBuffer, dest, offset, length); - } - } - @Test public void bufferArrayOffsets() throws Exception { TestSSLEnginePair pair = TestSSLEnginePair.create();