From 7550e3dcb763b75e0dc090efd7119b57e730c437 Mon Sep 17 00:00:00 2001
From: Jonas Herzig <me@johni0702.de>
Date: Sun, 31 Jul 2022 11:16:37 +0200
Subject: [PATCH] Fix race conditions in movement packet filtering (fixes #760)

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.
---
 .../replaymod/replay/FullReplaySender.java    | 45 ++++++++++++-------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/src/main/java/com/replaymod/replay/FullReplaySender.java b/src/main/java/com/replaymod/replay/FullReplaySender.java
index a6e8ae7d..fd41e6e2 100644
--- a/src/main/java/com/replaymod/replay/FullReplaySender.java
+++ b/src/main/java/com/replaymod/replay/FullReplaySender.java
@@ -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) {