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

HttpConnectionHandler should send a GO_AWAY frame with PROTOCOL_ERROR… #12985

Open
wants to merge 2 commits into
base: 4.1
Choose a base branch
from
Open

HttpConnectionHandler should send a GO_AWAY frame with PROTOCOL_ERROR… #12985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd00916
HttpConnectionHandler should send a GO_AWAY frame with PROTOCOL_ERROR…
vietj Nov 10, 2022
6189504
Update codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Co…
normanmaurer Nov 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,11 @@ protected void onStreamError(ChannelHandlerContext ctx, boolean outbound,

if (stream == null) {
if (!outbound || connection().local().mayHaveCreatedStream(streamId)) {
resetUnknownStream(ctx, streamId, http2Ex.error().code(), ctx.newPromise());
// See https://datatracker.ietf.org/doc/html/rfc7540#section-5.1.1:
//
// An endpoint that receives an unexpected stream identifier MUST respond with a connection error
// of type PROTOCOL_ERROR.
onError(ctx, outbound, connectionError(PROTOCOL_ERROR, "Unknown stream"));
normanmaurer marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
resetStream(ctx, stream, http2Ex.error().code(), ctx.newPromise());
Expand Down
33 changes: 17 additions & 16 deletions codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ public void teardown() throws Exception {
}

@Test
public void inflightFrameAfterStreamResetShouldNotMakeConnectionUnusable() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Uhh.... this is a really important case. It seems like this change is overzealous and confusing what the spec is saying.

From RFC 7540§5.1.1 (Note that the language is the same in RFC 9113§5.1.1):

The identifier of a newly established stream MUST be numerically
greater than all streams that the initiating endpoint has opened or
reserved. This governs streams that are opened using a HEADERS frame
and streams that are reserved using PUSH_PROMISE. An endpoint that
receives an unexpected stream identifier MUST respond with a
connection error (Section 5.4.1) of type PROTOCOL_ERROR.

That is talking about newly-established streams. The error checking above is not limiting itself to HEADERS and PUSH_PROMISES.

Also, Netty forgets about streams after it sends RST_STREAM, so there is a problem of accidentally considering trailers as creating a "new stream" even though they were racing.

So I'm arguing that this change is ignoring the RFC, specifically RFC 7540§5.4.2 (same language in RFC 9113):

A RST_STREAM is the last frame that an endpoint can send on a stream.
The peer that sends the RST_STREAM frame MUST be prepared to receive
any frames that were sent or enqueued for sending by the remote peer.
These frames can be ignored, except where they modify connection
state (such as the state maintained for header compression
(Section 4.3) or flow control).

Copy link
Member

@ejona86 ejona86 Nov 15, 2022 •
edited

Choose a reason for hiding this comment

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

Basically, what I'm saying is that with the current "memory" of Netty, it is difficult to do both things at once, and we are currently handling it the better way. Handling the RST_STREAM race is something that happens in practice with conforming implementations. Handling the "creating new stream with lower ID" only happens with a broken remote implementation. To be fully compliant would increase memory usage, and that is an option, but is complicated and helps almost nobody.

We could get partially compliant by just checking this case for PUSH_PROMISE. HEADERS are hard because of HTTP 1xx responses and trailers. I don't see a way to reliably detect if HEADERS are for a now-closed stream or are creating a new stream, without remembering detailed closed stream states. But in any case, any checks along those lines should be in DefaultHttp2ConnectionHandler, not in onStreamError(), because onStreamError() clearly needs to handle cases like racy DATA frames received after a RST_STREAM.

Copy link
Member

Choose a reason for hiding this comment

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

@vietj please check the comments of @ejona86

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have a look today @normanmaurer

Copy link
Contributor Author

@vietj vietj Nov 18, 2022 •
edited

Choose a reason for hiding this comment

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

if I understand correctly @ejona86 you are saying that when a stream id is received and not actually known by the state machine (because it does not retain such information and because the client is allowed to increase the stream id sequence as its will), we don't know whether that stream actually ever existed or it existed but is now closed

Copy link
Member

Choose a reason for hiding this comment

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

Any update /cc @ejona86

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @ejona86 :-)

Copy link
Member

Choose a reason for hiding this comment

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

you are saying that when a stream id is received and not actually known by the state machine (because it does not retain such information and because the client is allowed to increase the stream id sequence as its will), we don't know whether that stream actually ever existed or it existed but is now closed

Correct. And the code assumes it is the more likely case today, that the stream did exist.

public void unknownStreamShouldGoAway() throws Exception {
final CountDownLatch latch = new CountDownLatch(1);
doAnswer(new Answer<Void>() {
@Override
Expand All @@ -170,8 +170,8 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
latch.countDown();
return null;
}
}).when(clientListener).onHeadersRead(any(ChannelHandlerContext.class), eq(5), any(Http2Headers.class),
anyInt(), anyShort(), anyBoolean(), anyInt(), anyBoolean());
}).when(serverListener).onGoAwayRead(any(ChannelHandlerContext.class), any(Integer.class), any(Long.class),
any(ByteBuf.class));

bootstrapEnv(1, 1, 2, 1);

Expand All @@ -188,14 +188,6 @@ public void run() throws Http2Exception {
}
});

runInChannel(clientChannel, new Http2Runnable() {
@Override
public void run() throws Http2Exception {
http2Client.encoder().writeHeaders(ctx(), 5, headers, 0, weight, false, 0, false, newPromise());
http2Client.flush(ctx());
}
});

assertTrue(latch.await(DEFAULT_AWAIT_TIMEOUT_SECONDS, SECONDS));
}

Expand Down Expand Up @@ -487,6 +479,16 @@ public void run() throws Http2Exception {
public void headersUsingHigherValuedStreamIdPreventsUsingLowerStreamId() throws Exception {
bootstrapEnv(1, 1, 2, 0);

final CountDownLatch clientGoAwayLatch = new CountDownLatch(1);

doAnswer(new Answer<Void>() {
@Override
public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
clientGoAwayLatch.countDown();
return null;
}
}).when(clientListener).onGoAwayRead(any(ChannelHandlerContext.class), anyInt(), anyLong(), any(ByteBuf.class));

final Http2Headers headers = dummyHeaders();
runInChannel(clientChannel, new Http2Runnable() {
@Override
Expand All @@ -507,13 +509,12 @@ public void run() throws Http2Exception {
verify(serverListener, never()).onHeadersRead(any(ChannelHandlerContext.class), eq(3), any(Http2Headers.class),
anyInt(), anyShort(), anyBoolean(), anyInt(), anyBoolean());

// Client should receive a RST_STREAM for stream 3, but there is not Http2Stream object so the listener is never
// notified.
// Client should receive a GO_AWAY frame
assertTrue(clientGoAwayLatch.await(DEFAULT_AWAIT_TIMEOUT_SECONDS, SECONDS));

verify(serverListener, never()).onGoAwayRead(any(ChannelHandlerContext.class), anyInt(), anyLong(),
any(ByteBuf.class));
any(ByteBuf.class));
verify(serverListener, never()).onRstStreamRead(any(ChannelHandlerContext.class), anyInt(), anyLong());
verify(clientListener, never()).onGoAwayRead(any(ChannelHandlerContext.class), anyInt(), anyLong(),
any(ByteBuf.class));
verify(clientListener, never()).onRstStreamRead(any(ChannelHandlerContext.class), anyInt(), anyLong());
}

Expand Down