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

Fix NPE in InboundHttp2ToHttpAdapter #7215

Closed
wants to merge 6 commits into
base: 4.1
from

Conversation

Projects
None yet
2 participants
@durigon
Contributor

durigon commented Sep 15, 2017

Motivation:

  • When a HTTP/2 PUSH_PROMISE comes in, we call onPushPromiseRead in InboundHttp2ToHttpAdapter. This makes us create a FullHttpMessage, which eventually calls "HttpConversionUtil.parseStatus", which occurs always NPE because status is null.

Modifications:

  • Fix setting status code to 200 to prevent NPE in "InboundHttp2ToHttpAdapter::onPushPromiseRead".

Result:

According to the rfc7540 specification,

https://tools.ietf.org/html/rfc7540#section-8.2.1
Server push is semantically equivalent to a server responding to a
request; however, in this case, that request is also sent by the
server, as a PUSH_PROMISE frame.
https://tools.ietf.org/html/rfc7540#section-8.2.2
After sending the PUSH_PROMISE frame, the server can begin delivering
the pushed response as a response (Section 8.1.2.4) on a server-
initiated stream that uses the promised stream identifier.

Motiviation:
At the moment an NPE is thrown if someone tries to use the InboundHttp2ToHttpAdapter.

Modifications:
- Ensure the status was null in "InboundHttp2ToHttpAdapter::onPushPromiseRead" before calling "HttpConversionUtil.parseStatus" methods.
- Fix setting status to OK in "InboundHttp2ToHttpAdapter::onPushPromiseRead".

Result:
Fixes [#7214].

@durigon durigon changed the title from Motiviation: to Fix NPE in InboundHttp2ToHttpAdapter Sep 15, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 15, 2017

Member

Can you please add a test case?

Member

normanmaurer commented Sep 15, 2017

Can you please add a test case?

@durigon

This comment has been minimized.

Show comment
Hide comment
@durigon

durigon Sep 15, 2017

Contributor

@normanmaurer I've tested the fix code locally, but it takes a while because I'm not used to writing test cases.
Are there any test examples to refer to?

Contributor

durigon commented Sep 15, 2017

@normanmaurer I've tested the fix code locally, but it takes a while because I'm not used to writing test cases.
Are there any test examples to refer to?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 15, 2017

Member

@durigon hmm I would have need to take a deeper look but I would check some of the http2 tests to get inspiration. At worse I can also do a followup for a test if its too much work for you. Can you also please share the full stacktrace of the NPE in this issue which makes it easier to understand why this fix is needed ?

Also it seems like either something is missing in this PR or the commit message is not correct as you say:

Ensure the status was null in "InboundHttp2ToHttpAdapter::onPushPromiseRead" before calling "HttpConversionUtil.parseStatus" methods.

There seems to be no change related to this. Can you clarify ?

Member

normanmaurer commented Sep 15, 2017

@durigon hmm I would have need to take a deeper look but I would check some of the http2 tests to get inspiration. At worse I can also do a followup for a test if its too much work for you. Can you also please share the full stacktrace of the NPE in this issue which makes it easier to understand why this fix is needed ?

Also it seems like either something is missing in this PR or the commit message is not correct as you say:

Ensure the status was null in "InboundHttp2ToHttpAdapter::onPushPromiseRead" before calling "HttpConversionUtil.parseStatus" methods.

There seems to be no change related to this. Can you clarify ?

durigon added some commits Sep 15, 2017

Fix NPE in InboundHttp2ToHttpAdapter
Motivation:
- At the moment an NPE is thrown if someone tries to use the InboundHttp2ToHttpAdapter.
Modifications:
- Ensure the status was null in "InboundHttp2ToHttpAdapter::onPushPromiseRead" before calling "HttpConversionUtil.parseStatus" methods.
- Fix setting status to OK in "InboundHttp2ToHttpAdapter::onPushPromiseRead".
Result:
- Fixes [#7214].
@durigon

This comment has been minimized.

Show comment
Hide comment
@durigon

durigon Sep 15, 2017

Contributor

@normanmaurer Commit messages were written incorrectly, so I removed it from the commit message.

Contributor

durigon commented Sep 15, 2017

@normanmaurer Commit messages were written incorrectly, so I removed it from the commit message.

@durigon

This comment has been minimized.

Show comment
Hide comment
@durigon

durigon Sep 15, 2017

Contributor

@normanmaurer I have edited the test case(serverRequestPushPromise) in InboundHttp2ToHttpAdapterTest.java.

The full stacktrace of the NPE in this issue as following :

io.netty.handler.codec.http2.Http2Exception: Unrecognized HTTP status code 'null' encountered in translation to HTTP/1.x
at io.netty.handler.codec.http2.Http2Exception.connectionError(Http2Exception.java:99)
at io.netty.handler.codec.http2.HttpConversionUtil.parseStatus(HttpConversionUtil.java:182)
at io.netty.handler.codec.http2.HttpConversionUtil.toHttpResponse(HttpConversionUtil.java:204)
at io.netty.handler.codec.http2.InboundHttp2ToHttpAdapter.newMessage(InboundHttp2ToHttpAdapter.java:157)
at io.netty.handler.codec.http2.InboundHttp2ToHttpAdapter.processHeadersBegin(InboundHttp2ToHttpAdapter.java:190)
at io.netty.handler.codec.http2.InboundHttp2ToHttpAdapter.onPushPromiseRead(InboundHttp2ToHttpAdapter.java:299)
at io.netty.handler.codec.http2.Http2FrameListenerDecorator.onPushPromiseRead(Http2FrameListenerDecorator.java:85)
at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder$FrameReadListener.onPushPromiseRead(DefaultHttp2ConnectionDecoder.java:478)
at io.netty.handler.codec.http2.Http2InboundFrameLogger$1.onPushPromiseRead(Http2InboundFrameLogger.java:112)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader$3.processFragment(DefaultHttp2FrameReader.java:565)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readPushPromiseFrame(DefaultHttp2FrameReader.java:573)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader.processPayloadState(DefaultHttp2FrameReader.java:266)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readFrame(DefaultHttp2FrameReader.java:160)
at io.netty.handler.codec.http2.Http2InboundFrameLogger.readFrame(Http2InboundFrameLogger.java:41)
at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.decodeFrame(DefaultHttp2ConnectionDecoder.java:116)
at io.netty.handler.codec.http2.Http2ConnectionHandler$FrameDecoder.decode(Http2ConnectionHandler.java:353)
at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:413)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:489)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:428)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:265)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1273)
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1084)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:489)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:428)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:265)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1334)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:926)
at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:134)
at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:644)
at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:579)
at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:496)
at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:458)
at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:858)
at io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run(DefaultThreadFactory.java:138)
at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.NullPointerException
at io.netty.handler.codec.http.HttpResponseStatus.parseLine(HttpResponseStatus.java:458)
at io.netty.handler.codec.http2.HttpConversionUtil.parseStatus(HttpConversionUtil.java:175)
... 41 more

Contributor

durigon commented Sep 15, 2017

@normanmaurer I have edited the test case(serverRequestPushPromise) in InboundHttp2ToHttpAdapterTest.java.

The full stacktrace of the NPE in this issue as following :

io.netty.handler.codec.http2.Http2Exception: Unrecognized HTTP status code 'null' encountered in translation to HTTP/1.x
at io.netty.handler.codec.http2.Http2Exception.connectionError(Http2Exception.java:99)
at io.netty.handler.codec.http2.HttpConversionUtil.parseStatus(HttpConversionUtil.java:182)
at io.netty.handler.codec.http2.HttpConversionUtil.toHttpResponse(HttpConversionUtil.java:204)
at io.netty.handler.codec.http2.InboundHttp2ToHttpAdapter.newMessage(InboundHttp2ToHttpAdapter.java:157)
at io.netty.handler.codec.http2.InboundHttp2ToHttpAdapter.processHeadersBegin(InboundHttp2ToHttpAdapter.java:190)
at io.netty.handler.codec.http2.InboundHttp2ToHttpAdapter.onPushPromiseRead(InboundHttp2ToHttpAdapter.java:299)
at io.netty.handler.codec.http2.Http2FrameListenerDecorator.onPushPromiseRead(Http2FrameListenerDecorator.java:85)
at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder$FrameReadListener.onPushPromiseRead(DefaultHttp2ConnectionDecoder.java:478)
at io.netty.handler.codec.http2.Http2InboundFrameLogger$1.onPushPromiseRead(Http2InboundFrameLogger.java:112)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader$3.processFragment(DefaultHttp2FrameReader.java:565)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readPushPromiseFrame(DefaultHttp2FrameReader.java:573)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader.processPayloadState(DefaultHttp2FrameReader.java:266)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readFrame(DefaultHttp2FrameReader.java:160)
at io.netty.handler.codec.http2.Http2InboundFrameLogger.readFrame(Http2InboundFrameLogger.java:41)
at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.decodeFrame(DefaultHttp2ConnectionDecoder.java:116)
at io.netty.handler.codec.http2.Http2ConnectionHandler$FrameDecoder.decode(Http2ConnectionHandler.java:353)
at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:413)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:489)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:428)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:265)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1273)
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1084)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:489)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:428)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:265)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1334)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:926)
at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:134)
at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:644)
at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:579)
at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:496)
at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:458)
at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:858)
at io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run(DefaultThreadFactory.java:138)
at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.NullPointerException
at io.netty.handler.codec.http.HttpResponseStatus.parseLine(HttpResponseStatus.java:458)
at io.netty.handler.codec.http2.HttpConversionUtil.parseStatus(HttpConversionUtil.java:175)
... 41 more

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 16, 2017

Member

@durigon can you please sign our ICLA and let me know once done:

https://netty.io/s/icla

Member

normanmaurer commented Sep 16, 2017

@durigon can you please sign our ICLA and let me know once done:

https://netty.io/s/icla

@durigon

This comment has been minimized.

Show comment
Hide comment
@durigon

durigon Sep 17, 2017

Contributor

@normanmaurer I have signed the ICLA.

Contributor

durigon commented Sep 17, 2017

@normanmaurer I have signed the ICLA.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 17, 2017

Member

Squashed and cherry-picked into 4.1 (282aa35).

@durigon thanks!

Member

normanmaurer commented Sep 17, 2017

Squashed and cherry-picked into 4.1 (282aa35).

@durigon thanks!

@normanmaurer normanmaurer added the defect label Sep 17, 2017

@normanmaurer normanmaurer self-assigned this Sep 17, 2017

@normanmaurer normanmaurer added this to the 4.1.16.Final milestone Sep 17, 2017

@durigon

This comment has been minimized.

Show comment
Hide comment
@durigon

durigon Sep 18, 2017

Contributor

@normanmaurer Thanks!

Contributor

durigon commented Sep 18, 2017

@normanmaurer Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment