Skip to content
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

Jump to bottom

[UNDERTOW-2403] Fix race condition in read from buffer at ServletInpu… #1602

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[UNDERTOW-2403] Fix race condition in read from buffer at ServletInpu… #1602

Show file tree
Hide file tree
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
87 changes: 55 additions & 32 deletions servlet/src/main/java/io/undertow/servlet/spec/ServletInputStreamImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,7 @@ public int read(final byte[] b, final int off, final int len) throws IOException
int copied = Math.min(buffer.remaining(), len);
buffer.get(b, off, copied);
if (!buffer.hasRemaining()) {
pooled.close();
pooled = null;
closePoolIfNotNull();
if (listener != null) {
readIntoBufferNonBlocking();
}
Expand All @@ -196,50 +195,83 @@ public int read(final byte[] b, final int off, final int len) throws IOException

private void readIntoBuffer() throws IOException {
if (pooled == null && !anyAreSet(state, FLAG_FINISHED)) {
pooled = bufferPool.allocate();

int res = Channels.readBlocking(channel, pooled.getBuffer());
pooled.getBuffer().flip();
final ByteBuffer byteBuffer;
try {
PooledByteBuffer pooled = this.pooled = bufferPool.allocate();
byteBuffer = pooled.getBuffer();
} catch (NullPointerException e) {
// check for NPE + FLAG_FINISHED, it indicates a race condition where the buffer was closed and set to null by
// another thread
// instead of paying the price of synchronization, we just ignore the NPE and return, mimicking the code path
// we would follow in case the check in the if statement above returned false
// this is unlikely to happen, it will happen only during timeouts and server shutdowns
if (anyAreSet(state, FLAG_FINISHED)) {
return;
}
throw e;
}
int res = Channels.readBlocking(channel, byteBuffer);
byteBuffer.flip();
if (res == -1) {
setFlags(FLAG_FINISHED);
pooled.close();
pooled = null;
closePoolIfNotNull();
}
}
}

private void readIntoBufferNonBlocking() throws IOException {
if (pooled == null && !anyAreSet(state, FLAG_FINISHED)) {
pooled = bufferPool.allocate();
final ByteBuffer byteBuffer;
try {
PooledByteBuffer pooled = this.pooled = bufferPool.allocate();
byteBuffer = pooled.getBuffer();
} catch (NullPointerException e) {
// check for NPE + FLAG_FINISHED, it indicates a race condition where the buffer was closed and set to null by
// another thread
// instead of paying the price of synchronization, we just ignore the NPE and return, mimicking the code path
// we would follow in case the check in the if statement above returned false
// this is unlikely to happen, it will happen only during timeouts and server shutdowns
if (anyAreSet(state, FLAG_FINISHED)) {
return;
}
throw e;
}
if (listener == null) {
int res = channel.read(pooled.getBuffer());
if (res == 0) {
pooled.close();
pooled = null;
int res = channel.read(byteBuffer);
if (res == 0 && pooled != null) {
closePoolIfNotNull();
return;
}
pooled.getBuffer().flip();
if (res == -1) {
setFlags(FLAG_FINISHED);
pooled.close();
pooled = null;
closePoolIfNotNull();
}
} else {
int res = channel.read(pooled.getBuffer());
int res = channel.read(byteBuffer);
pooled.getBuffer().flip();
if (res == -1) {
setFlags(FLAG_FINISHED);
pooled.close();
pooled = null;
closePoolIfNotNull();
} else if (res == 0) {
clearFlags(FLAG_READY);
pooled.close();
pooled = null;
closePoolIfNotNull();
}
}
}
}

private void closePoolIfNotNull() {
try {
if (pooled != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ONly thing Id consider is to have local var? unless thats calculated choice to sometimes pay price of exception, rather than constant cost of local var and assignment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fl4via ^^

Copy link
Member Author

@fl4via fl4via Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We will pay the price of the NPE anyway, but, yes, I think adding the local var is a good idea because it will make the exception less likely.

if (pooled != null) {
   final PooledByteBuffer pooled = this.pooled; // we can still read a null value here, hence, NPE when closing
   this.pooled = null;
   pooled.close();  // marginal risk of NPE here
}

Copy link
Member Author

Choose a reason for hiding this comment

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

As a secod thought, will it make it less likely, if the time spent seting it to a variable might be the same time it takes to invoke close directly? Or is it not? I'll try to see if I can find some information on this. But the code I suggested above might have the same statistical risk of getting the NPE as the one we have without the local variable.

pooled.close();
pooled = null;
}
} catch (NullPointerException npe) {
// ignore it, this can happen if reading while another thread shutdown this input stream, caused by a timeout or a jdk shutdown
}
}

@Override
public int available() throws IOException {
if (anyAreSet(state, FLAG_CLOSED)) {
Expand All @@ -264,17 +296,11 @@ public void close() throws IOException {
try {
while (allAreClear(state, FLAG_FINISHED)) {
readIntoBuffer();
if (pooled != null) {
pooled.close();
pooled = null;
}
closePoolIfNotNull(); // race condition can happen if read bytes reads -1
}
} finally {
setFlags(FLAG_FINISHED);
if (pooled != null) {
pooled.close();
pooled = null;
}
closePoolIfNotNull();
channel.shutdownReads();
}
}
Expand Down Expand Up @@ -327,10 +353,7 @@ public void run() {
}
});
} finally {
if (pooled != null) {
pooled.close();
pooled = null;
}
closePoolIfNotNull();
IoUtils.safeClose(channel);
}
}
Expand Down
9 changes: 9 additions & 0 deletions spotbugs-exclude.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,15 @@
<Class name="io.undertow.client.http.HttpClientConnection$ClientReadListener"/>
<Method name="handleEvent"/>
</Match>
<Match>
<Bug pattern="DCN_NULLPOINTER_EXCEPTION"/>
<Class name="io.undertow.servlet.spec.ServletInputStreamImpl"/>
<Or>
<Method name="readIntoBuffer"/>
<Method name="readIntoBufferNonBlocking"/>
<Method name="closePoolIfNotNull"/>
</Or>
</Match>
<!-- ignore benchmarks -->
<Match>
<Package name="io.undertow.benchmarks"/>
Expand Down
Loading