Skip to content
Permalink
Browse files
8224829: AsyncSSLSocketClose.java has timing issue
Reviewed-by: mdoerr
Backport-of: a4277e5
  • Loading branch information
RealCLanger committed Oct 6, 2021
1 parent ddc3288 commit ceccbc35b9e1cff019ccaae666e823b9b9dfea3e
Showing 10 changed files with 618 additions and 292 deletions.
@@ -59,7 +59,7 @@ void changeReadCiphers(SSLReadCipher readCipher) {
}

@Override
public synchronized void close() throws IOException {
public void close() throws IOException {
if (!isClosed) {
super.close();
}
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1996, 2018, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1996, 2019, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -58,13 +58,18 @@ final class DTLSOutputRecord extends OutputRecord implements DTLSRecord {
}

@Override
public synchronized void close() throws IOException {
if (!isClosed) {
if (fragmenter != null && fragmenter.hasAlert()) {
isCloseWaiting = true;
} else {
super.close();
public void close() throws IOException {
recordLock.lock();
try {
if (!isClosed) {
if (fragmenter != null && fragmenter.hasAlert()) {
isCloseWaiting = true;
} else {
super.close();
}
}
} finally {
recordLock.unlock();
}
}

@@ -1,5 +1,5 @@
/*
* Copyright (c) 1996, 2018, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1996, 2019, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -31,6 +31,7 @@
import java.io.OutputStream;
import java.nio.BufferUnderflowException;
import java.nio.ByteBuffer;
import java.util.concurrent.locks.ReentrantLock;
import javax.crypto.BadPaddingException;
import sun.security.ssl.SSLCipher.SSLReadCipher;

@@ -43,10 +44,10 @@
abstract class InputRecord implements Record, Closeable {
SSLReadCipher readCipher;
// Needed for KeyUpdate, used after Handshake.Finished
TransportContext tc;
TransportContext tc;

final HandshakeHash handshakeHash;
boolean isClosed;
volatile boolean isClosed;

// The ClientHello version to accept. If set to ProtocolVersion.SSL20Hello
// and the first message we read is a ClientHello in V2 format, we convert
@@ -56,6 +57,8 @@ abstract class InputRecord implements Record, Closeable {
// fragment size
int fragmentSize;

final ReentrantLock recordLock = new ReentrantLock();

InputRecord(HandshakeHash handshakeHash, SSLReadCipher readCipher) {
this.readCipher = readCipher;
this.helloVersion = ProtocolVersion.TLS10;
@@ -92,14 +95,19 @@ void finishHandshake() {
* and flag the record as holding no data.
*/
@Override
public synchronized void close() throws IOException {
if (!isClosed) {
isClosed = true;
readCipher.dispose();
public void close() throws IOException {
recordLock.lock();
try {
if (!isClosed) {
isClosed = true;
readCipher.dispose();
}
} finally {
recordLock.unlock();
}
}

synchronized boolean isClosed() {
boolean isClosed() {
return isClosed;
}

@@ -1,5 +1,5 @@
/*
* Copyright (c) 1996, 2018, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1996, 2019, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -30,6 +30,7 @@
import java.io.IOException;
import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.util.concurrent.locks.ReentrantLock;
import sun.security.ssl.SSLCipher.SSLWriteCipher;

/**
@@ -68,6 +69,8 @@
// closed or not?
volatile boolean isClosed;

final ReentrantLock recordLock = new ReentrantLock();

/*
* Mappings from V3 cipher suite encodings to their pure V2 equivalents.
* This is taken from the SSL V3 specification, Appendix E.
@@ -89,15 +92,25 @@
// Please set packetSize and protocolVersion in the implementation.
}

synchronized void setVersion(ProtocolVersion protocolVersion) {
this.protocolVersion = protocolVersion;
void setVersion(ProtocolVersion protocolVersion) {
recordLock.lock();
try {
this.protocolVersion = protocolVersion;
} finally {
recordLock.unlock();
}
}

/*
* Updates helloVersion of this record.
*/
synchronized void setHelloVersion(ProtocolVersion helloVersion) {
this.helloVersion = helloVersion;
void setHelloVersion(ProtocolVersion helloVersion) {
recordLock.lock();
try {
this.helloVersion = helloVersion;
} finally {
recordLock.unlock();
}
}

/*
@@ -108,9 +121,14 @@ boolean isEmpty() {
return false;
}

synchronized boolean seqNumIsHuge() {
return (writeCipher.authenticator != null) &&
boolean seqNumIsHuge() {
recordLock.lock();
try {
return (writeCipher.authenticator != null) &&
writeCipher.authenticator.seqNumIsHuge();
} finally {
recordLock.unlock();
}
}

// SSLEngine and SSLSocket
@@ -148,68 +166,93 @@ void setDeliverStream(OutputStream outputStream) {
}

// Change write ciphers, may use change_cipher_spec record.
synchronized void changeWriteCiphers(SSLWriteCipher writeCipher,
void changeWriteCiphers(SSLWriteCipher writeCipher,
boolean useChangeCipherSpec) throws IOException {
if (isClosed()) {
if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
SSLLogger.warning("outbound has closed, ignore outbound " +
"change_cipher_spec message");
recordLock.lock();
try {
if (isClosed()) {
if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
SSLLogger.warning("outbound has closed, ignore outbound " +
"change_cipher_spec message");
}
return;
}
return;
}

if (useChangeCipherSpec) {
encodeChangeCipherSpec();
}

/*
* Dispose of any intermediate state in the underlying cipher.
* For PKCS11 ciphers, this will release any attached sessions,
* and thus make finalization faster.
*
* Since MAC's doFinal() is called for every SSL/TLS packet, it's
* not necessary to do the same with MAC's.
*/
writeCipher.dispose();
if (useChangeCipherSpec) {
encodeChangeCipherSpec();
}

this.writeCipher = writeCipher;
this.isFirstAppOutputRecord = true;
/*
* Dispose of any intermediate state in the underlying cipher.
* For PKCS11 ciphers, this will release any attached sessions,
* and thus make finalization faster.
*
* Since MAC's doFinal() is called for every SSL/TLS packet, it's
* not necessary to do the same with MAC's.
*/
writeCipher.dispose();

this.writeCipher = writeCipher;
this.isFirstAppOutputRecord = true;
} finally {
recordLock.unlock();
}
}

// Change write ciphers using key_update handshake message.
synchronized void changeWriteCiphers(SSLWriteCipher writeCipher,
void changeWriteCiphers(SSLWriteCipher writeCipher,
byte keyUpdateRequest) throws IOException {
if (isClosed()) {
if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
SSLLogger.warning("outbound has closed, ignore outbound " +
"key_update handshake message");
recordLock.lock();
try {
if (isClosed()) {
if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
SSLLogger.warning("outbound has closed, ignore outbound " +
"key_update handshake message");
}
return;
}
return;
}

// encode the handshake message, KeyUpdate
byte[] hm = HANDSHAKE_MESSAGE_KEY_UPDATE.clone();
hm[hm.length - 1] = keyUpdateRequest;
encodeHandshake(hm, 0, hm.length);
flush();
// encode the handshake message, KeyUpdate
byte[] hm = HANDSHAKE_MESSAGE_KEY_UPDATE.clone();
hm[hm.length - 1] = keyUpdateRequest;
encodeHandshake(hm, 0, hm.length);
flush();

// Dispose of any intermediate state in the underlying cipher.
writeCipher.dispose();
// Dispose of any intermediate state in the underlying cipher.
writeCipher.dispose();

this.writeCipher = writeCipher;
this.isFirstAppOutputRecord = true;
this.writeCipher = writeCipher;
this.isFirstAppOutputRecord = true;
} finally {
recordLock.unlock();
}
}

synchronized void changePacketSize(int packetSize) {
this.packetSize = packetSize;
void changePacketSize(int packetSize) {
recordLock.lock();
try {
this.packetSize = packetSize;
} finally {
recordLock.unlock();
}
}

synchronized void changeFragmentSize(int fragmentSize) {
this.fragmentSize = fragmentSize;
void changeFragmentSize(int fragmentSize) {
recordLock.lock();
try {
this.fragmentSize = fragmentSize;
} finally {
recordLock.unlock();
}
}

synchronized int getMaxPacketSize() {
return packetSize;
int getMaxPacketSize() {
recordLock.lock();
try {
return packetSize;
} finally {
recordLock.unlock();
}
}

// apply to DTLS SSLEngine
@@ -228,13 +271,18 @@ void launchRetransmission() {
}

@Override
public synchronized void close() throws IOException {
if (isClosed) {
return;
}
public void close() throws IOException {
recordLock.lock();
try {
if (isClosed) {
return;
}

isClosed = true;
writeCipher.dispose();
isClosed = true;
writeCipher.dispose();
} finally {
recordLock.unlock();
}
}

boolean isClosed() {
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1996, 2018, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1996, 2019, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -51,13 +51,18 @@ final class SSLEngineOutputRecord extends OutputRecord implements SSLRecord {
}

@Override
public synchronized void close() throws IOException {
if (!isClosed) {
if (fragmenter != null && fragmenter.hasAlert()) {
isCloseWaiting = true;
} else {
super.close();
public void close() throws IOException {
recordLock.lock();
try {
if (!isClosed) {
if (fragmenter != null && !fragmenter.isEmpty()) {
isCloseWaiting = true;
} else {
super.close();
}
}
} finally {
recordLock.unlock();
}
}

1 comment on commit ceccbc3

@openjdk-notifier
Copy link

@openjdk-notifier openjdk-notifier bot commented on ceccbc3 Oct 6, 2021

Choose a reason for hiding this comment

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

Please sign in to comment.