Skip to content

Support underflow handling without thread sleep #701

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 2 commits into from
Jul 29, 2021
Merged

Support underflow handling without thread sleep #701

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba69698
support underflow handling without thread sleep
Jul 29, 2021
217a5e8
refactoring - make isUnderflowHandlingEnabled private property
Jul 29, 2021
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
24 changes: 20 additions & 4 deletions src/main/java/com/rabbitmq/client/impl/nio/SocketChannelFrameHandlerState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import java.nio.ByteBuffer;
import java.nio.channels.SelectionKey;
import java.nio.channels.SocketChannel;
import java.util.concurrent.atomic.AtomicBoolean;


/**
*
Expand Down Expand Up @@ -70,6 +72,8 @@ public class SocketChannelFrameHandlerState {

final FrameBuilder frameBuilder;

private final AtomicBoolean isUnderflowHandlingEnabled = new AtomicBoolean(false);

public SocketChannelFrameHandlerState(SocketChannel channel, NioLoopContext nioLoopsState, NioParams nioParams, SSLEngine sslEngine) {
this.channel = channel;
this.readSelectorState = nioLoopsState.readSelectorState;
Expand Down Expand Up @@ -105,7 +109,7 @@ public SocketChannelFrameHandlerState(SocketChannel channel, NioLoopContext nioL
this.outputStream = new DataOutputStream(
new SslEngineByteBufferOutputStream(sslEngine, plainOut, cipherOut, channel)
);
this.frameBuilder = new SslEngineFrameBuilder(sslEngine, plainIn, cipherIn, channel);
this.frameBuilder = new SslEngineFrameBuilder(sslEngine, plainIn, cipherIn, channel, isUnderflowHandlingEnabled);
}

}
Expand Down Expand Up @@ -176,11 +180,14 @@ void endWriteSequence() {

void prepareForReadSequence() throws IOException {
if(ssl) {
cipherIn.clear();
plainIn.clear();
if (!isUnderflowHandlingEnabled.get()) {
cipherIn.clear();
cipherIn.flip();
}

cipherIn.flip();
plainIn.clear();
plainIn.flip();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on the need of these operations?

Copy link
Author

Choose a reason for hiding this comment

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

cipherIn should aggregate chunks of bytes on each read. So we should not clear it before each chunk reading.

Copy link
Author

@YuryAndr YuryAndr Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also in my implementation continueReading returns false when there is no data for next chunk. So there is no infinite loop around while(continueReading()) while data is not present in buffer. NIO thread can check selector state before trying to get bytes.


} else {
NioHelper.read(channel, plainIn);
plainIn.flip();
Expand All @@ -189,6 +196,15 @@ void prepareForReadSequence() throws IOException {

boolean continueReading() throws IOException {
if(ssl) {
if (isUnderflowHandlingEnabled.get()) {
int bytesRead = NioHelper.read(channel, cipherIn);
if (bytesRead == 0) {
return false;
} else {
cipherIn.flip();
return true;
}
}
if (!plainIn.hasRemaining() && !cipherIn.hasRemaining()) {
// need to try to read something
cipherIn.clear();
Expand Down
27 changes: 17 additions & 10 deletions src/main/java/com/rabbitmq/client/impl/nio/SslEngineFrameBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.io.IOException;
import java.nio.ByteBuffer;
import java.nio.channels.ReadableByteChannel;
import java.util.concurrent.atomic.AtomicBoolean;


/**
* Sub-class of {@link FrameBuilder} that unwraps crypted data from the network.
Expand All @@ -32,20 +34,25 @@ public class SslEngineFrameBuilder extends FrameBuilder {

private final ByteBuffer cipherBuffer;

public SslEngineFrameBuilder(SSLEngine sslEngine, ByteBuffer plainIn, ByteBuffer cipherIn, ReadableByteChannel channel) {
private final AtomicBoolean isUnderflowHandlingEnabled;

public SslEngineFrameBuilder(SSLEngine sslEngine, ByteBuffer plainIn, ByteBuffer cipherIn, ReadableByteChannel channel, final AtomicBoolean isUnderflowHandlingEnabled) {
super(channel, plainIn);
this.sslEngine = sslEngine;
this.cipherBuffer = cipherIn;
this.isUnderflowHandlingEnabled = isUnderflowHandlingEnabled;
}

@Override
protected boolean somethingToRead() throws IOException {
if (applicationBuffer.hasRemaining()) {
if (applicationBuffer.hasRemaining() && !isUnderflowHandlingEnabled.get()) {
return true;
} else {
applicationBuffer.clear();

while (true) {
boolean underflowHandling = false;

try {
SSLEngineResult result = sslEngine.unwrap(cipherBuffer, applicationBuffer);
switch (result.getStatus()) {
case OK:
Expand All @@ -59,18 +66,18 @@ protected boolean somethingToRead() throws IOException {
throw new SSLException("buffer overflow in read");
case BUFFER_UNDERFLOW:
cipherBuffer.compact();
int read = NioHelper.read(channel, cipherBuffer);
if (read == 0) {
return false;
}
cipherBuffer.flip();
break;
underflowHandling = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, removing some reading logic and putting it in the class that was already doing, nice.

return false;
case CLOSED:
throw new SSLException("closed in read");
default:
throw new IllegalStateException("Invalid SSL status: " + result.getStatus());
}
}
} finally {
isUnderflowHandlingEnabled.set(underflowHandling);
}

return false;
}
}

Expand Down