Ignore priority frames for non existing streams and so prevent a NPE (#10943)

Motivation:

https://github.com/netty/netty/pull/10765 added support for push promise and priority frames when using the Http2FrameCodec. Unfortunally it didnt correctly guard against the possibility to receive a priority frame for an non-existing stream, which resulted in a NPE

Modifications:

- Ignore priority frame for non existing stream
- Correctly implement equals / hashcode for DefaultHttp2PriorityFrame
- Add unit tests

Result:

Fixes https://github.com/netty/netty/issues/10941
This commit is contained in:
Norman Maurer 2021-01-18 14:11:07 +01:00 committed by GitHub
parent 59b3831bbc
commit 8cae1ed959
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 64 additions and 10 deletions

View File

@ -21,12 +21,11 @@ import io.netty.util.internal.UnstableApi;
* Default implementation of {@linkplain Http2PriorityFrame} * Default implementation of {@linkplain Http2PriorityFrame}
*/ */
@UnstableApi @UnstableApi
public final class DefaultHttp2PriorityFrame implements Http2PriorityFrame { public final class DefaultHttp2PriorityFrame extends AbstractHttp2StreamFrame implements Http2PriorityFrame {
private final int streamDependency; private final int streamDependency;
private final short weight; private final short weight;
private final boolean exclusive; private final boolean exclusive;
private Http2FrameStream http2FrameStream;
public DefaultHttp2PriorityFrame(int streamDependency, short weight, boolean exclusive) { public DefaultHttp2PriorityFrame(int streamDependency, short weight, boolean exclusive) {
this.streamDependency = streamDependency; this.streamDependency = streamDependency;
@ -50,25 +49,40 @@ public final class DefaultHttp2PriorityFrame implements Http2PriorityFrame {
} }
@Override @Override
public Http2PriorityFrame stream(Http2FrameStream stream) { public DefaultHttp2PriorityFrame stream(Http2FrameStream stream) {
http2FrameStream = stream; super.stream(stream);
return this; return this;
} }
@Override
public Http2FrameStream stream() {
return http2FrameStream;
}
@Override @Override
public String name() { public String name() {
return "PRIORITY_FRAME"; return "PRIORITY_FRAME";
} }
@Override
public boolean equals(Object o) {
if (!(o instanceof DefaultHttp2PriorityFrame)) {
return false;
}
DefaultHttp2PriorityFrame other = (DefaultHttp2PriorityFrame) o;
boolean same = super.equals(other);
return same && streamDependency == other.streamDependency
&& weight == other.weight && exclusive == other.exclusive;
}
@Override
public int hashCode() {
int hash = super.hashCode();
hash = hash * 31 + streamDependency;
hash = hash * 31 + weight;
hash = hash * 31 + (exclusive ? 1 : 0);
return hash;
}
@Override @Override
public String toString() { public String toString() {
return "DefaultHttp2PriorityFrame(" + return "DefaultHttp2PriorityFrame(" +
"stream=" + http2FrameStream + "stream=" + stream() +
", streamDependency=" + streamDependency + ", streamDependency=" + streamDependency +
", weight=" + weight + ", weight=" + weight +
", exclusive=" + exclusive + ", exclusive=" + exclusive +

View File

@ -657,6 +657,12 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
@Override @Override
public void onPriorityRead(ChannelHandlerContext ctx, int streamId, int streamDependency, public void onPriorityRead(ChannelHandlerContext ctx, int streamId, int streamDependency,
short weight, boolean exclusive) { short weight, boolean exclusive) {
Http2Stream stream = connection().stream(streamId);
if (stream == null) {
// The stream was not opened yet, let's just ignore this for now.
return;
}
onHttp2Frame(ctx, new DefaultHttp2PriorityFrame(streamDependency, weight, exclusive) onHttp2Frame(ctx, new DefaultHttp2PriorityFrame(streamDependency, weight, exclusive)
.stream(requireStream(streamId))); .stream(requireStream(streamId)));
} }

View File

@ -896,6 +896,40 @@ public class Http2FrameCodecTest {
channel.pipeline().fireUserEventTriggered(upgradeEvent); channel.pipeline().fireUserEventTriggered(upgradeEvent);
} }
@Test
public void priorityForNonExistingStream() {
writeHeaderAndAssert(1);
frameInboundWriter.writeInboundPriority(3, 1, (short) 31, true);
}
@Test
public void priorityForExistingStream() {
writeHeaderAndAssert(1);
writeHeaderAndAssert(3);
frameInboundWriter.writeInboundPriority(3, 1, (short) 31, true);
assertInboundStreamFrame(3, new DefaultHttp2PriorityFrame(1, (short) 31, true));
}
private void writeHeaderAndAssert(int streamId) {
frameInboundWriter.writeInboundHeaders(streamId, request, 31, false);
Http2Stream stream = frameCodec.connection().stream(streamId);
assertNotNull(stream);
assertEquals(State.OPEN, stream.state());
assertInboundStreamFrame(streamId, new DefaultHttp2HeadersFrame(request, false, 31));
}
private void assertInboundStreamFrame(int expectedId, Http2StreamFrame streamFrame) {
Http2StreamFrame inboundFrame = inboundHandler.readInbound();
Http2FrameStream stream2 = inboundFrame.stream();
assertNotNull(stream2);
assertEquals(expectedId, stream2.id());
assertEquals(inboundFrame, streamFrame.stream(stream2));
}
private ChannelHandlerContext eqFrameCodecCtx() { private ChannelHandlerContext eqFrameCodecCtx() {
return eq(frameCodec.ctx); return eq(frameCodec.ctx);
} }