andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Move validation of arguments out of synchronized block

Browse Source 
Motivation:

There is no need already use synchronized when validate the args of the methods.

Modifications:

First validate arguments and then use synchronized

Result:

Less code executed in synchronized block.
This commit is contained in:
Norman Maurer 2016-06-15 15:00:58 +02:00
parent 3719b5f919
commit 836b2d22c1
1 changed files with 209 additions and 204 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

117
handler/src/main/java/io/netty/handler/ssl/OpenSslEngine.java
View File

@ -524,14 +524,8 @@ public final class OpenSslEngine extends SSLEngine {
} }
@Override @Override
public synchronized SSLEngineResult wrap( public SSLEngineResult wrap(
final ByteBuffer[] srcs, final int offset, final int length, final ByteBuffer dst) throws SSLException { final ByteBuffer[] srcs, final int offset, final int length, final ByteBuffer dst) throws SSLException {
// Check to make sure the engine has not been closed
if (isDestroyed()) {
return CLOSED_NOT_HANDSHAKING;
}
// Throw required runtime exceptions // Throw required runtime exceptions
if (srcs == null) { if (srcs == null) {
throw new IllegalArgumentException("srcs is null"); throw new IllegalArgumentException("srcs is null");
@ -550,6 +544,12 @@ public final class OpenSslEngine extends SSLEngine {
throw new ReadOnlyBufferException(); throw new ReadOnlyBufferException();
} }
synchronized (this) {
// Check to make sure the engine has not been closed
if (isDestroyed()) {
return CLOSED_NOT_HANDSHAKING;
}
HandshakeStatus status = NOT_HANDSHAKING; HandshakeStatus status = NOT_HANDSHAKING;
// Prepare OpenSSL to work in server mode and receive handshake // Prepare OpenSSL to work in server mode and receive handshake
if (handshakeState != HandshakeState.FINISHED) { if (handshakeState != HandshakeState.FINISHED) {
@ -572,7 +572,7 @@ public final class OpenSslEngine extends SSLEngine {
int bytesProduced = 0; int bytesProduced = 0;
int bytesConsumed = 0; int bytesConsumed = 0;
int endOffset = offset + length; int endOffset = offset + length;
for (int i = offset; i < endOffset; ++ i) { for (int i = offset; i < endOffset; ++i) {
final ByteBuffer src = srcs[i]; final ByteBuffer src = srcs[i];
if (src == null) { if (src == null) {
throw new IllegalArgumentException("srcs[" + i + "] is null"); throw new IllegalArgumentException("srcs[" + i + "] is null");
@ -602,23 +602,25 @@ public final class OpenSslEngine extends SSLEngine {
pendingNetResult = readPendingBytesFromBIO(dst, bytesConsumed, bytesProduced, status); pendingNetResult = readPendingBytesFromBIO(dst, bytesConsumed, bytesProduced, status);
return pendingNetResult != null ? pendingNetResult : CLOSED_NOT_HANDSHAKING; return pendingNetResult != null ? pendingNetResult : CLOSED_NOT_HANDSHAKING;
case SSL.SSL_ERROR_WANT_READ: case SSL.SSL_ERROR_WANT_READ:
// If there is no pending data to read from BIO we should go back to event loop and try to read // If there is no pending data to read from BIO we should go back to event loop and try
// more data [1]. It is also possible that event loop will detect the socket has been closed. // to read more data [1]. It is also possible that event loop will detect the socket
// [1] https://www.openssl.org/docs/manmaster/ssl/SSL_write.html // has been closed. [1] https://www.openssl.org/docs/manmaster/ssl/SSL_write.html
pendingNetResult = readPendingBytesFromBIO(dst, bytesConsumed, bytesProduced, status); pendingNetResult = readPendingBytesFromBIO(dst, bytesConsumed, bytesProduced, status);
return pendingNetResult != null ? pendingNetResult : return pendingNetResult != null ? pendingNetResult :
new SSLEngineResult(getEngineStatus(), NEED_UNWRAP, bytesConsumed, bytesProduced); new SSLEngineResult(getEngineStatus(),
NEED_UNWRAP, bytesConsumed, bytesProduced);
case SSL.SSL_ERROR_WANT_WRITE: case SSL.SSL_ERROR_WANT_WRITE:
// SSL_ERROR_WANT_WRITE typically means that the underlying transport is not writable and we // SSL_ERROR_WANT_WRITE typically means that the underlying transport is not writable
// should set the "want write" flag on the selector and try again when the underlying transport // and we should set the "want write" flag on the selector and try again when the
// is writable [1]. However we are not directly writing to the underlying transport and instead // underlying transport is writable [1]. However we are not directly writing to the
// writing to a BIO buffer. The OpenSsl documentation says we should do the following [1]: // underlying transport and instead writing to a BIO buffer. The OpenSsl documentation
// says we should do the following [1]:
// //
// "When using a buffering BIO, like a BIO pair, data must be written into or retrieved out of // "When using a buffering BIO, like a BIO pair, data must be written into or retrieved
// the BIO before being able to continue." // out of the BIO before being able to continue."
// //
// So we attempt to drain the BIO buffer below, but if there is no data this condition is // So we attempt to drain the BIO buffer below, but if there is no data this condition
// undefined and we assume their is a fatal error with the openssl engine and close. // is undefined and we assume their is a fatal error with the openssl engine and close.
// [1] https://www.openssl.org/docs/manmaster/ssl/SSL_write.html // [1] https://www.openssl.org/docs/manmaster/ssl/SSL_write.html
pendingNetResult = readPendingBytesFromBIO(dst, bytesConsumed, bytesProduced, status); pendingNetResult = readPendingBytesFromBIO(dst, bytesConsumed, bytesProduced, status);
return pendingNetResult != null ? pendingNetResult : NEED_WRAP_CLOSED; return pendingNetResult != null ? pendingNetResult : NEED_WRAP_CLOSED;
@ -629,8 +631,8 @@ public final class OpenSslEngine extends SSLEngine {
} }
} }
} }
// We need to check if pendingWrittenBytesInBIO was checked yet, as we may not checked if the srcs was empty, // We need to check if pendingWrittenBytesInBIO was checked yet, as we may not checked if the srcs was
// or only contained empty buffers. // empty, or only contained empty buffers.
if (bytesConsumed == 0) { if (bytesConsumed == 0) {
SSLEngineResult pendingNetResult = readPendingBytesFromBIO(dst, 0, bytesProduced, status); SSLEngineResult pendingNetResult = readPendingBytesFromBIO(dst, 0, bytesProduced, status);
if (pendingNetResult != null) { if (pendingNetResult != null) {
@ -640,6 +642,7 @@ public final class OpenSslEngine extends SSLEngine {
return newResult(bytesConsumed, bytesProduced, status); return newResult(bytesConsumed, bytesProduced, status);
} }
}
/** /**
* Log the error, shutdown the engine and throw an exception. * Log the error, shutdown the engine and throw an exception.
@ -662,15 +665,10 @@ public final class OpenSslEngine extends SSLEngine {
return new SSLHandshakeException(err); return new SSLHandshakeException(err);
} }
public synchronized SSLEngineResult unwrap( public SSLEngineResult unwrap(
final ByteBuffer[] srcs, int srcsOffset, final int srcsLength, final ByteBuffer[] srcs, int srcsOffset, final int srcsLength,
final ByteBuffer[] dsts, final int dstsOffset, final int dstsLength) throws SSLException { final ByteBuffer[] dsts, final int dstsOffset, final int dstsLength) throws SSLException {
// Check to make sure the engine has not been closed
if (isDestroyed()) {
return CLOSED_NOT_HANDSHAKING;
}
// Throw required runtime exceptions // Throw required runtime exceptions
if (srcs == null) { if (srcs == null) {
throw new NullPointerException("srcs"); throw new NullPointerException("srcs");
@ -702,6 +700,31 @@ public final class OpenSslEngine extends SSLEngine {
capacity += dst.remaining(); capacity += dst.remaining();
} }
final int srcsEndOffset = srcsOffset + srcsLength;
long len = 0;
for (int i = srcsOffset; i < srcsEndOffset; i++) {
ByteBuffer src = srcs[i];
if (src == null) {
throw new IllegalArgumentException("srcs[" + i + "] is null");
}
len += src.remaining();
}
synchronized (this) {
// Check to make sure the engine has not been closed
if (isDestroyed()) {
return CLOSED_NOT_HANDSHAKING;
}
// protect against protocol overflow attack vector
if (len > MAX_ENCRYPTED_PACKET_LENGTH) {
isInboundDone = true;
isOutboundDone = true;
engineClosed = true;
shutdown();
throw ENCRYPTED_PACKET_OVERSIZED;
}
HandshakeStatus status = NOT_HANDSHAKING; HandshakeStatus status = NOT_HANDSHAKING;
// Prepare OpenSSL to work in server mode and receive handshake // Prepare OpenSSL to work in server mode and receive handshake
if (handshakeState != HandshakeState.FINISHED) { if (handshakeState != HandshakeState.FINISHED) {
@ -719,25 +742,6 @@ public final class OpenSslEngine extends SSLEngine {
} }
} }
final int srcsEndOffset = srcsOffset + srcsLength;
long len = 0;
for (int i = srcsOffset; i < srcsEndOffset; i++) {
ByteBuffer src = srcs[i];
if (src == null) {
throw new IllegalArgumentException("srcs[" + i + "] is null");
}
len += src.remaining();
}
// protect against protocol overflow attack vector
if (len > MAX_ENCRYPTED_PACKET_LENGTH) {
isInboundDone = true;
isOutboundDone = true;
engineClosed = true;
shutdown();
throw ENCRYPTED_PACKET_OVERSIZED;
}
// Write encrypted data to network BIO // Write encrypted data to network BIO
int bytesConsumed = 0; int bytesConsumed = 0;
if (srcsOffset < srcsEndOffset) { if (srcsOffset < srcsEndOffset) {
@ -747,7 +751,7 @@ public final class OpenSslEngine extends SSLEngine {
if (remaining == 0) { if (remaining == 0) {
// We must skip empty buffers as BIO_write will return 0 if asked to write something // We must skip empty buffers as BIO_write will return 0 if asked to write something
// with length 0. // with length 0.
srcsOffset ++; srcsOffset++;
continue; continue;
} }
int written = writeEncryptedData(src); int written = writeEncryptedData(src);
@ -755,7 +759,7 @@ public final class OpenSslEngine extends SSLEngine {
bytesConsumed += written; bytesConsumed += written;
if (written == remaining) { if (written == remaining) {
srcsOffset ++; srcsOffset++;
} else { } else {
// We were not able to write everything into the BIO so break the write loop as otherwise // We were not able to write everything into the BIO so break the write loop as otherwise
// we will produce an error on the next write attempt, which will trigger a SSL.clearError() // we will produce an error on the next write attempt, which will trigger a SSL.clearError()
@ -765,8 +769,8 @@ public final class OpenSslEngine extends SSLEngine {
} else { } else {
// BIO_write returned a negative or zero number, this means we could not complete the write // BIO_write returned a negative or zero number, this means we could not complete the write
// operation and should retry later. // operation and should retry later.
// We ignore BIO_* errors here as we use in memory BIO anyway and will do another SSL_* call later // We ignore BIO_* errors here as we use in memory BIO anyway and will do another SSL_* call
// on in which we will produce an exception in case of an error // later on in which we will produce an exception in case of an error
SSL.clearError(); SSL.clearError();
break; break;
} }
@ -782,21 +786,21 @@ public final class OpenSslEngine extends SSLEngine {
while (idx < endOffset) { while (idx < endOffset) {
ByteBuffer dst = dsts[idx]; ByteBuffer dst = dsts[idx];
if (!dst.hasRemaining()) { if (!dst.hasRemaining()) {
idx ++; idx++;
continue; continue;
} }
int bytesRead = readPlaintextData(dst); int bytesRead = readPlaintextData(dst);
// TODO: We may want to consider if we move this check and only do it in a less often called place at // TODO: We may want to consider if we move this check and only do it in a less often called place
// the price of not being 100% accurate, like for example when calling SSL.getError(...). // at the price of not being 100% accurate, like for example when calling SSL.getError(...).
rejectRemoteInitiatedRenegation(); rejectRemoteInitiatedRenegation();
if (bytesRead > 0) { if (bytesRead > 0) {
bytesProduced += bytesRead; bytesProduced += bytesRead;
if (!dst.hasRemaining()) { if (!dst.hasRemaining()) {
idx ++; idx++;
} else { } else {
// We read everything return now. // We read everything return now.
return newResult(bytesConsumed, bytesProduced, status); return newResult(bytesConsumed, bytesProduced, status);
@ -833,7 +837,7 @@ public final class OpenSslEngine extends SSLEngine {
if (pendingAppData() > 0) { if (pendingAppData() > 0) {
// We filled all buffers but there is still some data pending in the BIO buffer, return BUFFER_OVERFLOW. // We filled all buffers but there is still some data pending in the BIO buffer, return BUFFER_OVERFLOW.
return new SSLEngineResult( return new SSLEngineResult(
BUFFER_OVERFLOW, mayFinishHandshake(status != FINISHED ? getHandshakeStatus(): status), BUFFER_OVERFLOW, mayFinishHandshake(status != FINISHED ? getHandshakeStatus() : status),
bytesConsumed, bytesProduced); bytesConsumed, bytesProduced);
} }
@ -844,6 +848,7 @@ public final class OpenSslEngine extends SSLEngine {
return newResult(bytesConsumed, bytesProduced, status); return newResult(bytesConsumed, bytesProduced, status);
} }
}
private SSLEngineResult sslReadErrorResult(int err, int bytesConsumed, int bytesProduced) throws SSLException { private SSLEngineResult sslReadErrorResult(int err, int bytesConsumed, int bytesProduced) throws SSLException {
String errStr = SSL.getErrorString(err); String errStr = SSL.getErrorString(err);