Fix possible buffer leak when stream can't be mapped #14746
Conversation
Motivation: We should guard against the sitation where we can't map the stream. While this should never happen its better to be safe than sorry. Modifications: - Ensure the required Http2FrameStream can be found and only after that try to construct the specific frame. - Release ByteBuffer when DefaultHttp2DataFrame constructor throws Result: Fix possible buffer leaks
|
Noticed while working on something else |
codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java
Show resolved
Hide resolved
| onHttp2Frame(ctx, new DefaultHttp2GoAwayFrame(lastStreamId, errorCode, debugData).retain()); | ||
| onHttp2Frame(ctx, new DefaultHttp2GoAwayFrame(lastStreamId, errorCode, debugData.retain())); |
There was a problem hiding this comment.
As noted above, I think the pattern of building the frame and then once we're ready to forward it calling .retain() was better as it didn't retain until everything else was done. With this change if the constructor throws (perhaps a constructor assert added in the future) we leak the debugData buffer.
There was a problem hiding this comment.
I disagree because it is kind of a miss-use of the API. We don't want to retain the frame we really want to retain the payload. And we need to do this before calling the constructor as in theory it might call release on it.
There was a problem hiding this comment.
It sounds like when we pass the content into the frame constructor we're delegating ownership of it, otherwise the constructor has no right to release the buffer etc, very similar to the notion of move in C++ and Rust. That makes a lot of sense to me.
Maybe we should make the DefaultHttp2DataFrame constructor (as far as I can tell it's the only one that may throw right now) release the content if it's going to throw. That would remove the odd noise in onDataRead(..).
Motivation: We should guard against the sitation where we can't map the stream. While this should never happen its better to be safe than sorry. Modifications: - Ensure the required Http2FrameStream can be found and only after that try to construct the specific frame. - Release ByteBuffer when DefaultHttp2DataFrame constructor throws Result: Fix possible buffer leaks
Motivation: We should guard against the sitation where we can't map the stream. While this should never happen its better to be safe than sorry. Modifications: - Ensure the required Http2FrameStream can be found and only after that try to construct the specific frame. - Release ByteBuffer when DefaultHttp2DataFrame constructor throws Result: Fix possible buffer leaks
Motivation: We should guard against the sitation where we can't map the stream. While this should never happen its better to be safe than sorry. Modifications: - Ensure the required Http2FrameStream can be found and only after that try to construct the specific frame. - Release ByteBuffer when DefaultHttp2DataFrame constructor throws Result: Fix possible buffer leaks
4 participants
Motivation:
We should guard against the sitation where we can't map the stream. While this should never happen its better to be safe than sorry.
Modifications:
Result:
Fix possible buffer leaks