ByteToMessageDecoder#handlerRemoved may release cumulation buffer pre…#6728
Closed
Scottmitch wants to merge 1 commit into
Closed
ByteToMessageDecoder#handlerRemoved may release cumulation buffer pre…#6728Scottmitch wants to merge 1 commit into
Scottmitch wants to merge 1 commit into
Conversation
Scottmitch
force-pushed
the
byte_msg_decode
branch
from
f297958 to
6f306d5
Compare
Member
There was a problem hiding this comment.
why not just not merge both into one int ? No need to have two boolean here.
Member
Author
There was a problem hiding this comment.
2 booleans was a bit simpler but I will just use a byte if we want to be more definitive about memory usage.
Member
There was a problem hiding this comment.
was not about memory but more about "logic" as it can only be in one state of both. But this is just a nit. So feel free to ignore
Member
Author
There was a problem hiding this comment.
done ... I changed to a byte anyways :)
Member
There was a problem hiding this comment.
can you also have this test in ReplayingDecoderTest ?
…maturely Motivation: ByteToMessageDecoder#handlerRemoved will immediately release the cumulation buffer, but it is possible that a child class may still be using this buffer, and therefore use a dereferenced buffer. Modifications: - ByteToMessageDecoder#handlerRemoved and ByteToMessageDecoder#decode should coordinate to avoid the case where a child class is using the cumulation buffer but ByteToMessageDecoder releases that buffer. Result: Child classes of ByteToMessageDecoder are less likely to reference a released buffer.
Scottmitch
force-pushed
the
byte_msg_decode
branch
from
6f306d5 to
4981dcb
Compare
normanmaurer
approved these changes
May 10, 2017
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…maturely
Motivation:
ByteToMessageDecoder#handlerRemoved will immediately release the cumulation buffer, but it is possible that a child class may still be using this buffer, and therefore use a dereferenced buffer.
Modifications:
Result:
Child classes of ByteToMessageDecoder are less likely to reference a released buffer.