On Wed, 9 Oct 2024 07:47:38 GMT, Daniel Jeliński <djelin...@openjdk.org> wrote:
>> Artur Barashev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Return null if there is no record we attempted to decode
>
> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line
> 122:
>
>> 120: SSLEngineResult clientResult;
>> 121: // Client-side plain TCP socket.
>> 122: clientSocket = new Socket("localhost", port);
>
> if you use a SocketChannel instead, you'll be able to deal with ByteBuffers
> directly instead of going through byte arrays.
Good idea, done!
> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line
> 134:
>
>> 132:
>> 133: // Send client_hello, read and store all available
>> messages from the server.
>> 134: while (delayed.size() < 6) {
>
> Could you drop the delayed queue, and instead send user_canceled/close_notify
> before you start reading from the socket? It would simplify the code and
> improve the coverage.
Indeed, done!
> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line
> 162:
>
>> 160: // Send user_canceled and close_notify alerts to
>> server. Server should process
>> 161: // 2 unencrypted alerts and send its own
>> close_notify alert back to the client.
>> 162: ByteBuffer serverCloseNotify = clientWriteRead();
>
> Make sure to read until EOF
Done
> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line
> 196:
>
>> 194: }
>> 195:
>> 196: protected ByteBuffer clientWriteRead() throws IOException {
>
> could you split the write and read methods?
Done
> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line
> 204:
>
>> 202: inspectTlsBuffer(cTOs);
>> 203:
>> 204: byte[] outbound = new byte[cTOs.limit() - cTOs.position()];
>
> limit() - position() is equivalent to remaining()
Indeed, but this code is gone now.
> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line
> 215:
>
>> 213:
>> 214: try {
>> 215: len = is.read(inbound);
>
> this method might return a partial TLS record or multiple records. SSLEngine
> expects to receive the entire record in one buffer.
Reading until the EOF now
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794153103
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794153406
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794153565
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794153746
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794154338
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794155144
