DIvan2000/ReplayModCinematic
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix race conditions in movement packet filtering (fixes #760)

Browse Source 
We cannot filter this packet purely on the netty thread because our filter
depends on the current camera position (if it's too far, we do want to teleport
it closer) and accessing the camera is only safe from the main thread.

Previously the second part had not been considered which could lead to race
conditions where the camera may not yet exist on the netty thread at time of
handling. These appear to have gotten the wholly inappropriate "just put a null
check around it" treatment, but that means that the filter sometimes doesn't
work (hence the bug report).

This commit changes handling such that all access to `allowMovement` is always
done on the main thread and then moves the filtering there as well. And
therefore filtering should now work properly.
This commit is contained in:
Jonas Herzig
2022-07-31 11:16:37 +02:00
parent af3f6ddbe2
commit 7550e3dcb7
1 changed files with 30 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

45
src/main/java/com/replaymod/replay/FullReplaySender.java
View File

@@ -267,6 +267,8 @@ public class FullReplaySender extends ChannelDuplexHandler implements ReplaySend
/**
* Whether to allow (process) the next player movement packet.
*
* Must only be accessed from the main thread.
*/
protected boolean allowMovement;
@@ -639,7 +641,7 @@ public class FullReplaySender extends ChannelDuplexHandler implements ReplaySend
if(p instanceof GameJoinS2CPacket) {
GameJoinS2CPacket packet = (GameJoinS2CPacket) p;
int entId = packet.getEntityId();
allowMovement = true;
schedulePacketHandler(() -> allowMovement = true);
actualID = entId;
entId = -1789435; // Camera entity id should be negative which is an invalid id and can't be used by servers
//#if MC>=11400
@@ -773,7 +775,7 @@ public class FullReplaySender extends ChannelDuplexHandler implements ReplaySend
//#endif
//#endif
allowMovement = true;
schedulePacketHandler(() -> allowMovement = true);
}
if(p instanceof PlayerPositionLookS2CPacket) {
@@ -789,8 +791,6 @@ public class FullReplaySender extends ChannelDuplexHandler implements ReplaySend
if(replayHandler.shouldSuppressCameraMovements()) return null;
CameraEntity cent = replayHandler.getCameraEntity();
//#if MC>=10800
//#if MC>=11400
for (PlayerPositionLookS2CPacket.Flag relative : ppl.getFlags()) {
@@ -812,29 +812,28 @@ public class FullReplaySender extends ChannelDuplexHandler implements ReplaySend
}
//#endif
if(cent != null) {
if(!allowMovement && !((Math.abs(cent.getX() - ppl.getX()) > TP_DISTANCE_LIMIT) ||
(Math.abs(cent.getZ() - ppl.getZ()) > TP_DISTANCE_LIMIT))) {
return null;
} else {
allowMovement = false;
}
}
new Runnable() {
schedulePacketHandler(new Runnable() {
@Override
@SuppressWarnings("unchecked")
public void run() {
// FIXME: world shouldn't ever be null at this point, now that we use the packet queue
// probably fine to remove on the next non-patch version (don't want to break stuff now)
if (mc.world == null || !mc.isOnThread()) {
ReplayMod.instance.runLater(this);
return;
}
CameraEntity cent = replayHandler.getCameraEntity();
if (!allowMovement && !((Math.abs(cent.getX() - ppl.getX()) > TP_DISTANCE_LIMIT) ||
(Math.abs(cent.getZ() - ppl.getZ()) > TP_DISTANCE_LIMIT))) {
return;
} else {
allowMovement = false;
}
cent.setCameraPosition(ppl.getX(), ppl.getY(), ppl.getZ());
cent.setCameraRotation(ppl.getYaw(), ppl.getPitch(), cent.roll);
}
}.run();
});
return null;
}
@@ -1273,6 +1272,22 @@ public class FullReplaySender extends ChannelDuplexHandler implements ReplaySend
ReplayMod.instance.runTasks();
}
/**
* Runs the given runnable on the main thread as if it was a packet handler.
* Note that the packet handler queue has different behavior than the standard ReplayMod queue.
*/
private void schedulePacketHandler(Runnable runnable) {
if (mc.isOnThread()) {
runnable.run();
} else {
//#if MC>=11400
mc.execute(runnable);
//#else
//$$ mc.addScheduledTask(runnable);
//#endif
}
}
protected void processPacketSync(Packet p) {
//#if MC>=10904
if (p instanceof UnloadChunkS2CPacket) {