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

[1.12.2] Race Condition in FMLProxyPacket.processPacket #6295

Closed
mrgrim opened this issue Nov 3, 2019 · 2 comments
Closed

[1.12.2] Race Condition in FMLProxyPacket.processPacket #6295

mrgrim opened this issue Nov 3, 2019 · 2 comments
Labels
Stale This request hasn't had an update from the author in a long time.

Comments

@mrgrim
Copy link

mrgrim commented Nov 3, 2019

Minecraft Version: 1.12.2

Forge Version: 14.23.5.2847

There's a somewhat hard to trigger race condition for SimpleImpl (maybe the event driven system too, but I've not looked too closely at that one) on server side channels where the INetHandler reference provided by the MessageContext object in the onMessage callback is set incorrectly. This happens when multiple connected clients are sending packets to the server over the channel.

Each client connection is assigned to a thread in the netty server IO thread pool. When multiple clients are sending custom payload packets, they can come in over multiple threads. FMLProxyPacket.processPacket stores the INetHandler reference as a netty channel attribute, and this reference is pulled by the SimpleChannelHandlerWrapper.channelRead0 method. Due to multiple threads calling FMLProxyPacket.processPacket, this attribute can be modified before it is read here causing the received packet to be attributed to the wrong INetHandler.

This happens because all client connections end up sharing the same EmbeddedChannel for the "second stage". A naive solution to this issue is to wrap everything inside the top level "if" statement in FMLProxyPacket inside a "synchronized (internalChannel)" block. I have done some basic functional testing, and I cannot reproduce the incorrect INetHandler reference when this is done.

However, I have not performed any comprehensive performance testing. I'm not well equipped to create a synthetic test to simulate many clients communicating with a server using mods that stress custom payload packet use. I'm not even sure where to start, honestly.

I wanted to see what the Forge team thinks, if it's worth addressing in this old version, if you have access to good test setups that can properly measure this. This issue could be the root cause for any number of rare, random, and difficult to reproduce problems, so it maybe it's worth it.

@mrgrim mrgrim added the Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. label Nov 3, 2019
@mrgrim mrgrim changed the title [1.12.2] Race Condition in FMLProxyPacker.processPacket [1.12.2] Race Condition in FMLProxyPacket.processPacket Nov 3, 2019
@stale
Copy link

stale bot commented May 1, 2020

This issue has been automatically marked as stale because it has not had activity in a long time. If this issue is still relevant and should remain open, please reply with a short explanation (e.g. "I have checked the code and this issue is still relevant because ___." or "Here's a screenshot of this issue on the latest version"). Thank you for your contributions!

@stale stale bot added the Stale This request hasn't had an update from the author in a long time. label May 1, 2020
@stale
Copy link

stale bot commented May 15, 2020

This issue has been automatically closed because it has not had activity in a long time. Please feel free to reopen it or create a new issue.

@stale stale bot closed this as completed May 15, 2020
@autoforge autoforge bot removed the Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. label Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale This request hasn't had an update from the author in a long time.
Projects
None yet
Development

No branches or pull requests

1 participant