Skip to content

Add locking around read/write in NativeSsl. #490

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

Merged
merged 1 commit into from
May 16, 2018
Merged

Add locking around read/write in NativeSsl. #490

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 66 additions & 20 deletions common/src/main/java/org/conscrypt/NativeSsl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
import java.security.cert.X509Certificate;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import javax.crypto.SecretKey;
import javax.net.ssl.SSLException;
import javax.net.ssl.SSLHandshakeException;
Expand All @@ -57,6 +59,7 @@ final class NativeSsl {
private final AliasChooser aliasChooser;
private final PSKCallbacks pskCallbacks;
private X509Certificate[] localCertificates;
private final ReadWriteLock lock = new ReentrantReadWriteLock();
private volatile long ssl;

private NativeSsl(long ssl, SSLParametersImpl parameters,
Expand Down Expand Up @@ -381,32 +384,54 @@ void initialize(String hostname, OpenSSLKey channelIdPrivateKey) throws IOExcept
// TODO(nathanmittler): Remove once after we switch to the engine socket.
void doHandshake(FileDescriptor fd, int timeoutMillis)
throws CertificateException, IOException {
if (isClosed() || fd == null || !fd.valid()) {
throw new SocketException("Socket is closed");
lock.readLock().lock();
try {
if (isClosed() || fd == null || !fd.valid()) {
throw new SocketException("Socket is closed");
}
Copy link
Contributor

@narayank narayank May 15, 2018

Choose a reason for hiding this comment

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

This if () block is repeated in several places. Why not extract it out into a separate function ? (maybe with an "@GuardedBy" ?

Copy link
Contributor Author

@flooey flooey May 16, 2018

Choose a reason for hiding this comment

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

That seems like a good idea for a future change, yeah.

NativeCrypto.SSL_do_handshake(ssl, this, fd, handshakeCallbacks, timeoutMillis);
} finally {
lock.readLock().unlock();
}
NativeCrypto.SSL_do_handshake(ssl, this, fd, handshakeCallbacks, timeoutMillis);
}

int doHandshake() throws IOException {
return NativeCrypto.ENGINE_SSL_do_handshake(ssl, this, handshakeCallbacks);
lock.readLock().lock();
try {
return NativeCrypto.ENGINE_SSL_do_handshake(ssl, this, handshakeCallbacks);
Copy link
Contributor

@narayank narayank May 15, 2018

Choose a reason for hiding this comment

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

Why do some methods have the isClosed check whereas others do not ? (other examples, readDirectByteBuffer, writeDirectByteBuffer and forceRead)

Copy link
Contributor Author

@flooey flooey May 16, 2018

Choose a reason for hiding this comment

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

The pattern is that the methods used by SSLSocket have the check and the ones used by SSLEngine don't, but they should probably all check for being closed. (Note that it's a case of which exception is thrown rather than correctness, as the native code checks for ssl == 0 and throws NullPointerException in that case. It'd be better to throw SSLException if it's closed.) Another change I'd like to push to a future change.

} finally {
lock.readLock().unlock();
}
}

// TODO(nathanmittler): Remove once after we switch to the engine socket.
int read(FileDescriptor fd, byte[] buf, int offset, int len, int timeoutMillis)
throws IOException {
if (isClosed() || fd == null || !fd.valid()) {
throw new SocketException("Socket is closed");
lock.readLock().lock();
try {
if (isClosed() || fd == null || !fd.valid()) {
throw new SocketException("Socket is closed");
}
return NativeCrypto
.SSL_read(ssl, this, fd, handshakeCallbacks, buf, offset, len, timeoutMillis);
} finally {
lock.readLock().unlock();
}
return NativeCrypto.SSL_read(ssl, this, fd, handshakeCallbacks, buf, offset, len, timeoutMillis);
}

// TODO(nathanmittler): Remove once after we switch to the engine socket.
void write(FileDescriptor fd, byte[] buf, int offset, int len, int timeoutMillis)
throws IOException {
if (isClosed() || fd == null || !fd.valid()) {
throw new SocketException("Socket is closed");
lock.readLock().lock();
try {
if (isClosed() || fd == null || !fd.valid()) {
throw new SocketException("Socket is closed");
}
NativeCrypto
.SSL_write(ssl, this, fd, handshakeCallbacks, buf, offset, len, timeoutMillis);
} finally {
lock.readLock().unlock();
}
NativeCrypto.SSL_write(ssl, this, fd, handshakeCallbacks, buf, offset, len, timeoutMillis);
}

@SuppressWarnings("deprecation") // PSKKeyManager is deprecated, but in our own package
Expand Down Expand Up @@ -508,17 +533,32 @@ boolean wasShutdownSent() {

int readDirectByteBuffer(long destAddress, int destLength)
throws IOException, CertificateException {
return NativeCrypto.ENGINE_SSL_read_direct(
ssl, this, destAddress, destLength, handshakeCallbacks);
lock.readLock().lock();
try {
return NativeCrypto.ENGINE_SSL_read_direct(
ssl, this, destAddress, destLength, handshakeCallbacks);
} finally {
lock.readLock().unlock();
}
}

int writeDirectByteBuffer(long sourceAddress, int sourceLength) throws IOException {
return NativeCrypto.ENGINE_SSL_write_direct(
ssl, this, sourceAddress, sourceLength, handshakeCallbacks);
lock.readLock().lock();
try {
return NativeCrypto.ENGINE_SSL_write_direct(
ssl, this, sourceAddress, sourceLength, handshakeCallbacks);
} finally {
lock.readLock().unlock();
}
}

void forceRead() throws IOException {
NativeCrypto.ENGINE_SSL_force_read(ssl, this, handshakeCallbacks);
lock.readLock().lock();
try {
NativeCrypto.ENGINE_SSL_force_read(ssl, this, handshakeCallbacks);
} finally {
lock.readLock().unlock();
}
}

int getPendingReadableBytes() {
Expand All @@ -530,8 +570,16 @@ int getMaxSealOverhead() {
}

void close() {
NativeCrypto.SSL_free(ssl, this);
ssl = 0L;
lock.writeLock().lock();
try {
if (!isClosed()) {
long toFree = ssl;
ssl = 0L;
NativeCrypto.SSL_free(toFree, this);
}
} finally {
lock.writeLock().unlock();
}
}

boolean isClosed() {
Expand All @@ -553,9 +601,7 @@ private boolean isClient() {
@Override
protected final void finalize() throws Throwable {
try {
if (!isClosed()) {
close();
}
close();
} finally {
super.finalize();
}
Expand Down