Skip to content

SSLEngine.unwrap precondition tests. #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
prbprbprb wants to merge 2 commits into
base: master
Choose a base branch
from
+94 −0
Open

SSLEngine.unwrap precondition tests. #1414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c4e3225
SSLEngine.unwrap precondition tests.
prbprbprb Oct 31, 2025
f415407
Merge branch 'master' into unwrap_preconditions
prbprbprb Nov 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions common/src/test/java/org/conscrypt/javax/net/ssl/SSLEngineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,6 +1033,92 @@ private void assertWrapSucceeds(ByteBuffer[] buffers, int offset, int length) th
}
}

@Test
public void unwrapPreconditions() throws Exception {
Copy link
Collaborator

@juergw juergw Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test first sets up some test data, and then calls unwrap several times. This only works as expected if the test data is not changed, which is not guaranteed for unwrap. It should be fine here, because the precondition check should fail at the start before any changes to the input happen. But still, we should make sure that the reader is aware of this. So I think this should either create fresh input data for each call, or have a comment on why it is fine to reuse the input data.

Copy link
Contributor Author

@prbprbprb prbprbprb Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I'll rework it.

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);
Copy link
Collaborator

@juergw juergw Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to move wrapThenUnwrap into a new test.

Copy link
Contributor Author

@prbprbprb prbprbprb Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

Just for clarity, you're basically suggesting to split into a precondition failures test and and precondition "should pass" test? I can do that and add suitable comments.

Copy link
Collaborator

@juergw juergw Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, exactly. it is also related to the other comment: for the prcondition pass test, can we re-use the input data? or should we create new input data for each test case, to make sure that the previous test doesn't change the test data?

Copy link
Contributor Author

@prbprbprb prbprbprb Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pass cases as they currently are, will often return an error due to data re-use, under/overflow etc but not throw. There are tests elsewhere to check data flows correctly. These tests are just to ensure that the preconditions checks performed by warp/unwrap behave as expected.

That said, it's probably good practice to ensure that pass tests work end to end, so I'll split the wrap ones out too.

Copy link
Contributor Author

@prbprbprb prbprbprb Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like #1420 ?

// 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();
Expand Down
8 changes: 8 additions & 0 deletions testing/src/main/java/org/conscrypt/TestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
Expand Down