Fix reentrancy bug in ByteToMessageDecoder#15833
Conversation
Motivation: If the ByteToMessageDecoder gets a reentrant channelRead call, we need to avoid closing or otherwise manipulating the cumulation buffer. Modification: Guard reentrant channelRead calls by queueing up messages and letting the top-level call process them all in order. Result: Reentrant calls to ByteToMessageDecoder.channelRead will no longer cause weird IllegalReferenceCountException on the cumulation buffer.
|
This will fix the build errors in #15830 |
|
This needs to be back ported to 4.1 as well. |
|
Opened 4.1 version of this PR in #15834 |
| ByteBuf buf2 = channel.alloc().buffer(); | ||
| buf2.writeLong(42); // Adding 8 bytes. | ||
| channel.writeInbound(buf2); // Reentrant call back into ByteToMessageDecoder | ||
| ctx.read(); // With LocalChannel this would be a reentrant call. For EmbeddedChannel it's harmless. |
There was a problem hiding this comment.
so do you want to do also add a test for LocalChannel ?
There was a problem hiding this comment.
I've been trying to do this. It gets pretty complicated and hard to follow.
|
btw what exactly triggers this? ive tried to design handlers that can produce multiple elements at a time (eg the decompressors) in such a way that nested channelRead calls aren't triggered in the first place, and are instead "held back" upstream |
|
@yawkat the case I ran into was using x25519mlkem key exchange in the SslHandler on the local transport. I think the exchange messages got so big that they needed multiple reads to deliver, so when autoRead is off, the SslHandler will at one point do an unwrap, get buffer underflow, then call ctx.read(), which reenters the ByteToMessageDecoder.channelRead. |
|
Sounds like sslhandler should wait until channelReadComplete to do the read call |
I think while this might also fix it we should better ensure the |
| ctx.fireChannelRead(input); | ||
| } | ||
| } | ||
| } while (inputMessages != null && (input = inputMessages.pollFirst()) != null); |
There was a problem hiding this comment.
I also wonder if we need to do anything special in handlerRemoved now.
There was a problem hiding this comment.
handlerRemoved is fine because it just punts the removal to the decodeRemovalReentryProtection method… but there, I wonder if we should also wait until the inputMessages is null or empty
There was a problem hiding this comment.
Added a test to capture this.
|
I got an ok from Norman after he closed #15844 Merging |
4 participants
Motivation:
If the ByteToMessageDecoder gets a reentrant channelRead call, we need to avoid closing or otherwise manipulating the cumulation buffer.
Modification:
Guard reentrant channelRead calls by queueing up messages and letting the top-level call process them all in order.
Result:
Reentrant calls to ByteToMessageDecoder.channelRead will no longer cause weird IllegalReferenceCountException on the cumulation buffer.