Prevent TailExceptionsHandler from racing a graceful closeWith() close#616
Open
purpurcof wants to merge 1 commit into
Open
Prevent TailExceptionsHandler from racing a graceful closeWith() close#616purpurcof wants to merge 1 commit into
purpurcof wants to merge 1 commit into
Conversation
Owner
|
That actually makes a lot of sense. I'll take a look at it once I'm home. |
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
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.
How do I reproduce this?
Hard to reproduce reliably against a live server/client - it depends on the kick packet's write not completing synchronously, which is more likely under load (many concurrent connections, e.g. via SoulFire bot floods, per #476). Instead, I isolated the exact mechanism with a standalone repro on plain sockets (no Netty/Sonar dependencies needed): RaceTest.java (attached). It simulates an async write-then-close racing against an immediate, independent second close() call while the write is still in flight - the same pattern as VerificationHandler#fail() calling disconnect() and then throwing.
Every run: the write fails with
SocketException: Socket closed, and the client receives only a truncated fragment of the payload instead of the full message - reproducing the "client gets a bare reset instead of the kick message" symptom from #476.I also tested and ruled out the alternative theory that this is caused by a TCP RST from unread data in the receive buffer at close() time (RstTest.java, attached) - that does not reproduce here even with 2MB of unread data, with a SO_LINGER(0) sanity control confirming the harness can detect RST when it actually occurs.
Root cause
VerificationHandler#fail()callsuser.disconnect(...), which queues an asyncchannel.writeAndFlush(msg).addListener(ChannelFutureListener.CLOSE)inProtocolUtil#closeWith(), and then immediately throwsQuietDecoderException.INSTANCEto unwind. That exception propagates throughSonarPacketDecoder#channelRead()and reachesTailExceptionsHandler#exceptionCaught(), which callsctx.close()unconditionally - a second, independent close racing the one already queued. If the kick packet write hasn't finished flushing yet, this second close aborts it, so the client only gets a truncated packet instead of the full kick message.#595 attempted to fix this by catching
QuietDecoderExceptionin the decoder, butQuietDecoderException.INSTANCEis also thrown from ~15 other places (MinecraftVarInt21FrameDecoder,LoginStartPacket,SystemChatPacket,PluginMessagePacket, etc.) that never calldisconnect()first and rely entirely on reachingTailExceptionsHandlerto close the connection at all. Catching it in the decoder would silently leave those connections open indefinitely - a resource leak / DoS surface for an antibot plugin.Fix
Tag the channel with a
CLOSINGattribute right whencloseWith()queues its write, and haveTailExceptionsHandlerskip its ownclose()only when that attribute is set - every otherQuietDecoderExceptionthrow site keeps closing the connection exactly as before.Verification
Reproduced the race directly with RaceTest.java (attached, 5/5 runs). Have not been able to verify against a live server + SoulFire per the repro steps in #476 - would appreciate a maintainer or the original reporter confirming against a real setup.
References