diff --git a/src/main/java/vip/fubuki/playersync/PlayerSync.java b/src/main/java/vip/fubuki/playersync/PlayerSync.java index 5c7f03a..5727974 100644 --- a/src/main/java/vip/fubuki/playersync/PlayerSync.java +++ b/src/main/java/vip/fubuki/playersync/PlayerSync.java @@ -117,16 +117,15 @@ public class PlayerSync { "PRIMARY KEY (`id`)" + ");" ); + // FIX H-8: Use prepared statements for server_id to prevent SQL injection from config long current = System.currentTimeMillis(); - JDBCsetUp.executeUpdate( - "INSERT INTO `" + dbName + "`.`server_info`(id,enable,last_update) " + - "VALUES(" + JdbcConfig.SERVER_ID.get() + ",true," + current + ") " + - "ON DUPLICATE KEY UPDATE id= " + JdbcConfig.SERVER_ID.get() + ",enable = 1," + - "last_update=" + current + ";" + JDBCsetUp.executePreparedUpdate( + "INSERT INTO `" + dbName + "`.`server_info`(id,enable,last_update) VALUES(?,true,?) ON DUPLICATE KEY UPDATE id=VALUES(id),enable=1,last_update=VALUES(last_update)", + JdbcConfig.SERVER_ID.get(), current ); - JDBCsetUp.executeUpdate( - "UPDATE `" + dbName + "`.`server_info` SET last_update=" + System.currentTimeMillis() + - " WHERE id='" + JdbcConfig.SERVER_ID.get() + "'" + JDBCsetUp.executePreparedUpdate( + "UPDATE `" + dbName + "`.`server_info` SET last_update=? WHERE id=?", + System.currentTimeMillis(), JdbcConfig.SERVER_ID.get() ); // Create curios table if the Curios mod is loaded diff --git a/src/main/java/vip/fubuki/playersync/sync/VanillaSync.java b/src/main/java/vip/fubuki/playersync/sync/VanillaSync.java index b07acef..fd08f5b 100644 --- a/src/main/java/vip/fubuki/playersync/sync/VanillaSync.java +++ b/src/main/java/vip/fubuki/playersync/sync/VanillaSync.java @@ -389,16 +389,22 @@ public class VanillaSync { } }); - // Wait for main thread to finish applying (prevents lock release before data is applied) + // FIX H-3: Release lock BEFORE waiting on latch to prevent deadlock. + // If we hold the lock while waiting, onServerShutdown trying to acquire + // the same lock will deadlock (shutdown blocks main thread, preventing + // server.execute() from draining, preventing latch countdown). + lock.unlock(); + if (!applyLatch.await(15, TimeUnit.SECONDS)) { PlayerSync.LOGGER.error("Timeout waiting for main thread sync for player {}", player_uuid); syncNotCompletedPlayer.remove(player_uuid); } + return; // Lock already released, skip finally } catch (Exception e) { PlayerSync.LOGGER.error("Internal Exception detected!", e); syncNotCompletedPlayer.remove(player_uuid); } finally { - lock.unlock(); + if (lock.isHeldByCurrentThread()) lock.unlock(); } } @@ -726,54 +732,54 @@ public class VanillaSync { } /** - * FIX: All save operations (inventory, curios, mod-compat) are now under the per-player lock - * to prevent race conditions with concurrent auto-save tasks on the executor. + * FIX C-2: All save operations run on the MAIN THREAD (onPlayerLogout fires on main thread). + * Entity state (inventory, curios, effects) is read safely on the correct thread. + * DB writes block briefly but this is required for correctness. */ - public static void doPlayerLogout(PlayerEvent.PlayerLoggedOutEvent event) throws SQLException, IOException { - String player_uuid = event.getEntity().getUUID().toString(); - Player player = event.getEntity(); - ReentrantLock lock = getPlayerLock(player_uuid); - lock.lock(); - try { - // Save ALL data under lock: curios, mod-compat, then main inventory, then mark offline - if (ModList.get().isLoaded("curios")) { - ModsSupport modsSupport = new ModsSupport(); - if (player.isDeadOrDying()) { - modsSupport.saveCuriosFromCacheOrApi(player); - } else { - modsSupport.onPlayerLeave(player); - } - } - ModCompatSync.storeAll(player); - store(player, false); - JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid); - } finally { - lock.unlock(); - removePlayerLock(player_uuid); - } - } - @SubscribeEvent - public static void onPlayerLogout(PlayerEvent.PlayerLoggedOutEvent event) throws SQLException { + public static void onPlayerLogout(PlayerEvent.PlayerLoggedOutEvent event) { String player_uuid = event.getEntity().getUUID().toString(); if (deadPlayerWhileLogging.contains(player_uuid)) { PlayerSync.LOGGER.warn("A dead or dying player was kicked, uuid: {}", player_uuid); - JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid); + try { + JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid); + } catch (SQLException e) { + PlayerSync.LOGGER.error("Error marking dead player offline: {}", player_uuid, e); + } deadPlayerWhileLogging.remove(player_uuid); } else if (syncNotCompletedPlayer.contains(player_uuid)) { PlayerSync.LOGGER.warn("Player {} logged out with uncompleted sync. Data won't be saved for safety.", player_uuid); - JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid); + try { + JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid); + } catch (SQLException e) { + PlayerSync.LOGGER.error("Error marking unsynced player offline: {}", player_uuid, e); + } syncNotCompletedPlayer.remove(player_uuid); } else { - // FIX: All saves moved inside doPlayerLogout under the per-player lock - // to prevent race conditions with auto-save - executorService.submit(() -> { - try { - doPlayerLogout(event); - } catch (Exception e) { - PlayerSync.LOGGER.error("Error during player logout save for {}", player_uuid, e); + Player player = event.getEntity(); + ReentrantLock lock = getPlayerLock(player_uuid); + lock.lock(); + try { + // Save curios (main thread - safe to read Curios API) + if (ModList.get().isLoaded("curios")) { + ModsSupport modsSupport = new ModsSupport(); + if (player.isDeadOrDying()) { + modsSupport.saveCuriosFromCacheOrApi(player); + } else { + modsSupport.onPlayerLeave(player); + } } - }); + // Save mod compat data (main thread - safe to read Accessories/CosmeticArmor) + ModCompatSync.storeAll(player); + // Save main inventory + effects + advancements (main thread - safe) + store(player, false); + JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid); + } catch (Exception e) { + PlayerSync.LOGGER.error("Error during player logout save for {}", player_uuid, e); + } finally { + lock.unlock(); + removePlayerLock(player_uuid); + } } } @@ -1002,42 +1008,34 @@ public class VanillaSync { } // Auto-save all online players + // FIX C-1/C-2/C-4: onServerTick runs on the MAIN THREAD. We call store() and mod saves + // directly here to safely read entity state (inventory, curios, effects, etc.). + // The DB writes inside store() block briefly (~1-5ms per player) but this is acceptable + // for a 60-second interval. This eliminates all off-thread entity access duplication exploits. if (autoSaveTickCounter >= AUTO_SAVE_INTERVAL_TICKS) { autoSaveTickCounter = 0; MinecraftServer server = ServerLifecycleHooks.getCurrentServer(); if (server != null) { for (ServerPlayer player : server.getPlayerList().getPlayers()) { - // Skip dead players and players whose sync hasn't completed yet if (player.isDeadOrDying() || syncNotCompletedPlayer.contains(player.getUUID().toString())) { continue; } - executorService.submit(() -> { - try { - store(player, false); - } catch (Exception e) { - PlayerSync.LOGGER.error("Error auto-saving player {}", player.getUUID(), e); + String puuid = player.getUUID().toString(); + ReentrantLock lock = getPlayerLock(puuid); + if (!lock.tryLock()) continue; // Skip if already being saved (logout in progress) + try { + store(player, false); + if (ModList.get().isLoaded("curios") && !player.isDeadOrDying()) { + new ModsSupport().StoreCurios(player, false); } - }); - executorService.submit(() -> { - try { - // Only auto-save curios for alive players to prevent saving empty data - if (!player.isDeadOrDying()) { - new ModsSupport().StoreCurios(player, false); - } - } catch (SQLException e) { - PlayerSync.LOGGER.error("Error auto-saving Curios data for player {}", player.getUUID(), e); + if (!player.isDeadOrDying()) { + ModCompatSync.storeAll(player); } - }); - // Auto-save mod compatibility data (Accessories, CosmeticArmor) - executorService.submit(() -> { - try { - if (!player.isDeadOrDying()) { - ModCompatSync.storeAll(player); - } - } catch (Exception e) { - PlayerSync.LOGGER.error("Error auto-saving mod compat data for player {}", player.getUUID(), e); - } - }); + } catch (Exception e) { + PlayerSync.LOGGER.error("Error auto-saving player {}", player.getUUID(), e); + } finally { + lock.unlock(); + } } } } diff --git a/src/main/java/vip/fubuki/playersync/sync/addons/CuriosCache.java b/src/main/java/vip/fubuki/playersync/sync/addons/CuriosCache.java index db90ea9..58f1efc 100644 --- a/src/main/java/vip/fubuki/playersync/sync/addons/CuriosCache.java +++ b/src/main/java/vip/fubuki/playersync/sync/addons/CuriosCache.java @@ -36,8 +36,11 @@ public class CuriosCache { //Create a method to store temporary curios data when player is dead. //Then check player status in the logged out event,and take a normal sync if player is alive. //If player is dead or dying,the cache will be used to prevent the empty data from the failure of getting handlerOpt. + // FIX H-5: Cache curios on death regardless of keepInventory. Without this, + // players on servers WITHOUT keepInventory who die then disconnect before respawning + // would have their curios data overwritten with empty data (Curios API returns empty for dead players). public static void tryStoreCuriosToCache(net.minecraft.world.entity.player.Player player) { - if (!ModList.get().isLoaded("curios") || !CuriosCache.isKeepInventoryActive(player)) { + if (!ModList.get().isLoaded("curios")) { return; } diff --git a/src/main/java/vip/fubuki/playersync/sync/addons/ModsSupport.java b/src/main/java/vip/fubuki/playersync/sync/addons/ModsSupport.java index 7cfc537..82f3bf1 100644 --- a/src/main/java/vip/fubuki/playersync/sync/addons/ModsSupport.java +++ b/src/main/java/vip/fubuki/playersync/sync/addons/ModsSupport.java @@ -543,8 +543,14 @@ public class ModsSupport { if (modified) { // Write the modified .dat file back and force RS2 to reload fileNbt.put("data", dataNbt); - net.minecraft.nbt.NbtIo.writeCompressed(fileNbt, datFile.toPath()); - PlayerSync.LOGGER.info("Wrote modified RS2 storage data file"); + // FIX C-6: Atomic write - write to temp file then rename. + // Direct write can corrupt the ENTIRE RS2 storage for the server on crash mid-write. + java.nio.file.Path tmpPath = datFile.toPath().resolveSibling(datFile.getName() + ".tmp"); + net.minecraft.nbt.NbtIo.writeCompressed(fileNbt, tmpPath); + java.nio.file.Files.move(tmpPath, datFile.toPath(), + java.nio.file.StandardCopyOption.REPLACE_EXISTING, + java.nio.file.StandardCopyOption.ATOMIC_MOVE); + PlayerSync.LOGGER.info("Wrote modified RS2 storage data file (atomic)"); // Force the StorageRepository to reload from disk // The simplest way is via reflection on the data storage cache @@ -599,6 +605,7 @@ public class ModsSupport { try { net.minecraft.resources.ResourceLocation loc = net.minecraft.core.registries.BuiltInRegistries.ITEM.getKey(stack.getItem()); + if (loc == null) return null; // FIX C-5: null check prevents NPE on unregistered items if (!loc.getNamespace().equals("refinedstorage") && !loc.getNamespace().equals("extradisks")) { return null; } diff --git a/src/main/java/vip/fubuki/playersync/util/LocalJsonUtil.java b/src/main/java/vip/fubuki/playersync/util/LocalJsonUtil.java index b91fb6d..5d9d668 100644 --- a/src/main/java/vip/fubuki/playersync/util/LocalJsonUtil.java +++ b/src/main/java/vip/fubuki/playersync/util/LocalJsonUtil.java @@ -30,7 +30,13 @@ public class LocalJsonUtil { String key = trim.substring(0, equalIndex); String value = trim.substring(equalIndex + 1); - map.put(keyParser.apply(key), value); + // FIX M-1: Catch parse exceptions per-entry to prevent one malformed key + // from emptying the entire map (e.g. cosmetic armor slots all lost) + try { + map.put(keyParser.apply(key), value); + } catch (Exception e) { + // Skip malformed entries instead of crashing the whole parse + } } return map; }