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.

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

[UNDERTOW-2444] Fix RST scenario violation in H2 #1662

Merged
merged 1 commit into from
Oct 4, 2024
Merged
Changes from all commits
Commits
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
39 changes: 36 additions & 3 deletions core/src/main/java/io/undertow/protocols/http2/Http2Channel.java
Original file line number Diff line number Diff line change
Expand Up @@ -1225,6 +1225,7 @@ private StreamHolder handleRstStream(int streamId, boolean receivedRst) {
resetStreamTracker.store(streamId, holder);
if(streamId % 2 == (isClient() ? 1 : 0)) {
sendConcurrentStreamsAtomicUpdater.getAndDecrement(this);
holder.sent = true;
} else {
receiveConcurrentStreamsAtomicUpdater.getAndDecrement(this);
}
Expand All @@ -1239,13 +1240,14 @@ private StreamHolder handleRstStream(int streamId, boolean receivedRst) {
//Server side originated, no input from client other than RST
//this can happen on page refresh when push happens, but client
//still has valid cache entry
//NOTE: this is specific case when its set.
holder.resetByPeer = receivedRst;
} else {
handleRstWindow();
}
}
} else if(receivedRst){
final StreamHolder resetStream = resetStreamTracker.find(streamId);
final StreamHolder resetStream = resetStreamTracker.find(streamId, true);
if(resetStream != null && resetStream.resetByPeer) {
//This means other side reset stream at some point.
//depending on peer or network latency our frames might be late and
Expand Down Expand Up @@ -1378,6 +1380,10 @@ private static final class StreamHolder {
* This flag is set only in case of short lived server push that was reset by remote end.
*/
boolean resetByPeer = false;
/**
* flag indicate whether our side originated. This is done for caching purposes, handlng differs.
*/
boolean sent = false;
Http2StreamSourceChannel sourceChannel;
Http2StreamSinkChannel sinkChannel;

Expand All @@ -1388,6 +1394,13 @@ private static final class StreamHolder {
StreamHolder(Http2StreamSinkChannel sinkChannel) {
this.sinkChannel = sinkChannel;
}

@Override
public String toString() {
return "StreamHolder [sourceClosed=" + sourceClosed + ", sinkClosed=" + sinkClosed + ", resetByPeer=" + resetByPeer
+ ", sent=" + sent + ", sourceChannel=" + sourceChannel + ", sinkChannel=" + sinkChannel + "]";
}

}

// cache that keeps track of streams until they can be evicted @see Http2Channel#RST_STREAM_EVICATION_TIME
Expand All @@ -1403,12 +1416,27 @@ private void store(int streamId, StreamHolder streamHolder) {
streamHolders.put(streamId, streamHolder);
entries.add(new StreamCacheEntry(streamId));
}
private StreamHolder find(int streamId) {

/**
* Method will return only sent
* @param streamId
* @return
*/
private StreamHolder find(final int streamId) {
return find(streamId, false);
}

private StreamHolder find(final int streamId, final boolean all) {
for (Iterator<StreamCacheEntry> iterator = entries.iterator(); iterator.hasNext();) {
StreamCacheEntry entry = iterator.next();
if (entry.shouldEvict()) {
iterator.remove();
StreamHolder holder = streamHolders.remove(entry.streamId);
if(!holder.sent || holder.resetByPeer) {
//if its not our end of chain, its just cached, so we only cache for purpose of
// handling eager RST
continue;
}
AbstractHttp2StreamSourceChannel receiver = holder.sourceChannel;
if(receiver != null) {
IoUtils.safeClose(receiver);
Expand All @@ -1422,7 +1450,12 @@ private StreamHolder find(int streamId) {
}
} else break;
}
return streamHolders.get(streamId);
final StreamHolder holder = streamHolders.get(streamId);
if(holder != null && (!all && !holder.sent)) {
return null;
} else {
return holder;
}
}

private Map<Integer, StreamHolder> getStreamHolders() {
Expand Down
Loading