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

Reference counting bug in GO_AWAY frame processing #7892

Closed
rschmitt opened this issue Apr 27, 2018 · 2 comments
Closed

Reference counting bug in GO_AWAY frame processing #7892

rschmitt opened this issue Apr 27, 2018 · 2 comments
Assignees
Labels
Milestone

Comments

@rschmitt
Copy link

Expected behavior

The server should be able to process a GO_AWAY frame from the client without throwing an IllegalReferenceCountException.

Actual behavior

I get an IllegalReferenceCountException, as described below.

Steps to reproduce

Here's what happens when the client (which in my case is nghttp) sends a GOAWAY frame, along with the refCnt of the ByteBuf instance at each step:

  1. The ByteBuf comes in here containing a seventeen byte GO_AWAY frame; it is assigned to the cumulator field, which is initially null (as we are not currently in the middle of processing a frame). refCnt: 1.
  2. The FrameListener creates an intermediate DefaultHttp2GoAwayFrame object and retains it. refCnt: 2.
  3. The Http2MultiplexCodec calls onHttp2GoAwayFrame, which broadcasts the frame to all its child streams, followed by fireChannelRead to propagate the GO_AWAY through the rest of the parent channel.
    1. The onHttp2GoAwayFrame method calls release on the supplied frame. refCnt: 1.
    2. The fireChannelRead call will typically end up in the TailContext, which will release the object again in the onUnhandledInboundMessage method. refCnt: 0.
  4. Finally, the ByteToMessageDecoder tries to call release on the cumulator field here, whose refCnt is already zero.

Minimal yet complete reproducer code (or URL to code)

I can attempt to write a repro case if desired. I hope that the above play-by-play will make the bug clear. My guess is that the onHttp2GoAwayFrame method should not be releasing the frame; @normanmaurer mentioned in another context that "all on* methods of this class will 'NOT' take ownership of the buffer."

Netty version

4.1.24

JVM version (e.g. java -version)

1.8.0_161

OS version (e.g. uname -a)

Darwin f40f243a5fab.ant.amazon.com 16.7.0 Darwin Kernel Version 16.7.0: Tue Jan 30 11:27:06 PST 2018; root:xnu-3789.73.11~1/RELEASE_X86_64 x86_64

@normanmaurer
Copy link
Member

@rschmitt you are right... I will come up with a PR shortly.

@normanmaurer normanmaurer self-assigned this Apr 27, 2018
@normanmaurer normanmaurer added this to the 4.1.25.Final milestone Apr 27, 2018
normanmaurer added a commit that referenced this issue Apr 27, 2018
… a DefaultHttp2GoAwayFrame with a non empty ByteBuffer is received.

Motivation:

We incorrectly called frame.release() in onHttp2GoAwayFrame which could lead to IllegalReferenceCountExceptions.

Modifications:

- Not call frame.release
- Add a unit test

Result:

Fxies #7892.
@normanmaurer
Copy link
Member

@rschmitt PTAL #7894

normanmaurer added a commit that referenced this issue Apr 28, 2018
… a DefaultHttp2GoAwayFrame with a non empty ByteBuffer is received.

Motivation:

We incorrectly called frame.release() in onHttp2GoAwayFrame which could lead to IllegalReferenceCountExceptions.  The call of release() is inappropriate because the fireChannelRead() in onHttp2Frame() will handle it.

Modifications:

- Not call frame.release()
- Add a unit test

Result:

Fxies #7892.
normanmaurer added a commit that referenced this issue Apr 28, 2018
… a DefaultHttp2GoAwayFrame with a non empty ByteBuffer is received. (#7894)

Motivation:

We incorrectly called frame.release() in onHttp2GoAwayFrame which could lead to IllegalReferenceCountExceptions.  The call of release() is inappropriate because the fireChannelRead() in onHttp2Frame() will handle it.

Modifications:

- Not call frame.release()
- Add a unit test

Result:

Fxies #7892.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants