From d1570d357454ce9ae3c2bf90ccf1c6d28c9defb7 Mon Sep 17 00:00:00 2001 From: simoleo89 Date: Tue, 9 Jun 2026 11:02:09 +0000 Subject: [PATCH] fix: economy-integrity, currency thread-safety, and resource-leak hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the full-codebase audit. Economy/security (Batch A): - CatalogBuyItemEvent: clamp client `count` to 1..100 — the club-offer branch accumulated cost in plain ints, so a huge count overflowed to a negative total, bypassed the affordability checks and CREDITED the buyer (free currency/subscription exploit). - HousekeepingGiveCredits/GiveCurrency: bound `amount` to +/-1e9 to stop overflow/negative-balance grants via the privileged path. - RoomTrade: synchronize accept/confirm/offer/remove and add a `completed` re-entry guard so two simultaneous confirms can't run tradeItems() twice (item/credit duplication). - HabboInfo: serialize credits + currencies read-modify-write and the saveCurrencies snapshot on a dedicated lock (never held across DB I/O) — fixes lost updates and Trove rehash-during-iteration corruption between the credit-roller thread and purchase/trade handlers. - AchievementManager/HabboStats: atomic incrementProgress() so concurrent progress sources don't lose updates. Resource/stability (Batch B): - GameMessageRateLimit: release the wrapped ByteBuf on every drop path (ClientMessage isn't ReferenceCounted, so the decoder's auto-release is a no-op) — fixes a refcount leak on pre-auth/rate-limited packets. - AuthRateLimiter: opportunistically evict window-expired STATE/PROBE_STATE entries — previously grew unbounded, one entry per unique client IP. - ForumThread/ForumThreadComment: close getGeneratedKeys() ResultSets via try-with-resources, and create the first comment after the thread's connection is released (was holding two pooled connections at once). - DatabasePool: add socketTimeout/connectTimeout/tcpKeepAlive so a stalled MariaDB can't pin a pooled connection (and its thread) indefinitely. Concurrency visibility (Batch C, partial): - Room: mark allowBotsWalk/allowPets/allowPetsEat volatile (read every cycle, written from settings handlers on another thread). --- .../com/eu/habbo/database/DatabasePool.java | 8 ++++ .../achievements/AchievementManager.java | 4 +- .../habbohotel/guilds/forums/ForumThread.java | 26 ++++++----- .../guilds/forums/ForumThreadComment.java | 11 ++--- .../com/eu/habbo/habbohotel/rooms/Room.java | 8 ++-- .../eu/habbo/habbohotel/rooms/RoomTrade.java | 19 +++++--- .../eu/habbo/habbohotel/users/HabboInfo.java | 44 ++++++++++++++----- .../eu/habbo/habbohotel/users/HabboStats.java | 10 +++++ .../incoming/catalog/CatalogBuyItemEvent.java | 9 ++++ .../HousekeepingGiveCreditsEvent.java | 3 +- .../HousekeepingGiveCurrencyEvent.java | 3 +- .../gameserver/auth/AuthRateLimiter.java | 28 ++++++++++++ .../decoders/GameMessageRateLimit.java | 7 +++ 13 files changed, 142 insertions(+), 38 deletions(-) diff --git a/Emulator/src/main/java/com/eu/habbo/database/DatabasePool.java b/Emulator/src/main/java/com/eu/habbo/database/DatabasePool.java index acd50764..813f8f15 100644 --- a/Emulator/src/main/java/com/eu/habbo/database/DatabasePool.java +++ b/Emulator/src/main/java/com/eu/habbo/database/DatabasePool.java @@ -79,6 +79,14 @@ class DatabasePool { databaseConfiguration.addDataSourceProperty("useLocalSessionState", "true"); databaseConfiguration.addDataSourceProperty("cacheResultSetMetadata", "true"); databaseConfiguration.addDataSourceProperty("elideSetAutoCommits", "true"); + + // Fail fast instead of pinning a pooled connection (and its worker + // thread) indefinitely on a stalled/slow MariaDB. HikariCP's + // connectionTimeout only bounds the pool *borrow*; these bound the + // actual socket/connect round-trip. Overridable via db.params. + databaseConfiguration.addDataSourceProperty("socketTimeout", "30000"); + databaseConfiguration.addDataSourceProperty("connectTimeout", "10000"); + databaseConfiguration.addDataSourceProperty("tcpKeepAlive", "true"); databaseConfiguration.addDataSourceProperty("maintainTimeStats", "false"); databaseConfiguration.setPoolName("HabboHikariPool"); diff --git a/Emulator/src/main/java/com/eu/habbo/habbohotel/achievements/AchievementManager.java b/Emulator/src/main/java/com/eu/habbo/habbohotel/achievements/AchievementManager.java index 7e5ecdb8..fcc5ba99 100644 --- a/Emulator/src/main/java/com/eu/habbo/habbohotel/achievements/AchievementManager.java +++ b/Emulator/src/main/java/com/eu/habbo/habbohotel/achievements/AchievementManager.java @@ -100,9 +100,9 @@ public class AchievementManager { if (oldLevel != null && (oldLevel.level == achievement.levels.size() && currentProgress >= oldLevel.progress)) //Maximum achievement gotten. return; - habbo.getHabboStats().setProgress(achievement, currentProgress + amount); + int newProgress = habbo.getHabboStats().incrementProgress(achievement, amount); - AchievementLevel newLevel = achievement.getLevelForProgress(currentProgress + amount); + AchievementLevel newLevel = achievement.getLevelForProgress(newProgress); if (AchievementManager.TALENTTRACK_ENABLED) { for (TalentTrackType type : TalentTrackType.values()) { diff --git a/Emulator/src/main/java/com/eu/habbo/habbohotel/guilds/forums/ForumThread.java b/Emulator/src/main/java/com/eu/habbo/habbohotel/guilds/forums/ForumThread.java index 0bc0c265..54517e4e 100644 --- a/Emulator/src/main/java/com/eu/habbo/habbohotel/guilds/forums/ForumThread.java +++ b/Emulator/src/main/java/com/eu/habbo/habbohotel/guilds/forums/ForumThread.java @@ -101,21 +101,27 @@ public class ForumThread implements Runnable, ISerialize { if (statement.executeUpdate() < 1) return null; - ResultSet set = statement.getGeneratedKeys(); - if (set.next()) { - int threadId = set.getInt(1); - createdThread = new ForumThread(threadId, guild.getId(), opener.getHabboInfo().getId(), subject, 0, timestamp, timestamp, ForumThreadState.OPEN, false, false, 0, null); - cacheThread(createdThread); - - ForumThreadComment comment = ForumThreadComment.create(createdThread, opener, message); - createdThread.addComment(comment); - - Emulator.getPluginManager().fireEvent(new GuildForumThreadCreated(createdThread)); + try (ResultSet set = statement.getGeneratedKeys()) { + if (set.next()) { + int threadId = set.getInt(1); + createdThread = new ForumThread(threadId, guild.getId(), opener.getHabboInfo().getId(), subject, 0, timestamp, timestamp, ForumThreadState.OPEN, false, false, 0, null); + cacheThread(createdThread); + } } } catch (SQLException e) { LOGGER.error("Caught SQL exception", e); } + // ForumThreadComment.create() opens its OWN connection; do it after the + // thread's connection has been released to avoid holding two pooled + // connections simultaneously per forum-thread creation. + if (createdThread != null) { + ForumThreadComment comment = ForumThreadComment.create(createdThread, opener, message); + createdThread.addComment(comment); + + Emulator.getPluginManager().fireEvent(new GuildForumThreadCreated(createdThread)); + } + return createdThread; } diff --git a/Emulator/src/main/java/com/eu/habbo/habbohotel/guilds/forums/ForumThreadComment.java b/Emulator/src/main/java/com/eu/habbo/habbohotel/guilds/forums/ForumThreadComment.java index c404ddb5..99b6cf61 100644 --- a/Emulator/src/main/java/com/eu/habbo/habbohotel/guilds/forums/ForumThreadComment.java +++ b/Emulator/src/main/java/com/eu/habbo/habbohotel/guilds/forums/ForumThreadComment.java @@ -98,12 +98,13 @@ public class ForumThreadComment implements Runnable, ISerialize { if (statement.executeUpdate() < 1) return null; - ResultSet set = statement.getGeneratedKeys(); - if (set.next()) { - int commentId = set.getInt(1); - createdComment = new ForumThreadComment(commentId, thread.getThreadId(), poster.getHabboInfo().getId(), message, timestamp, ForumThreadState.OPEN, 0); + try (ResultSet set = statement.getGeneratedKeys()) { + if (set.next()) { + int commentId = set.getInt(1); + createdComment = new ForumThreadComment(commentId, thread.getThreadId(), poster.getHabboInfo().getId(), message, timestamp, ForumThreadState.OPEN, 0); - Emulator.getPluginManager().fireEvent(new GuildForumThreadCommentCreated(createdComment)); + Emulator.getPluginManager().fireEvent(new GuildForumThreadCommentCreated(createdComment)); + } } } catch (SQLException e) { LOGGER.error("Caught SQL exception", e); diff --git a/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/Room.java b/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/Room.java index fa31b151..b8b25ceb 100644 --- a/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/Room.java +++ b/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/Room.java @@ -158,10 +158,12 @@ public class Room implements Comparable, ISerialize, Runnable { private String tags; private boolean publicRoom; private boolean staffPromotedRoom; - private boolean allowPets; - private boolean allowPetsEat; + // Read every room cycle (processBots/processPets) but written from settings/ + // admin packet handlers on another thread — volatile for cross-thread visibility. + private volatile boolean allowPets; + private volatile boolean allowPetsEat; private boolean allowWalkthrough; - private boolean allowBotsWalk; + private volatile boolean allowBotsWalk; private boolean allowEffects; private boolean hideWall; private int chatMode; diff --git a/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTrade.java b/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTrade.java index b28eb85a..38d2b66f 100644 --- a/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTrade.java +++ b/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTrade.java @@ -26,6 +26,7 @@ public class RoomTrade { private final List users; private final Room room; + private boolean completed = false; public RoomTrade(Habbo userOne, Habbo userTwo, Room room) { this.users = new ArrayList<>(); @@ -54,7 +55,7 @@ public class RoomTrade { this.sendMessageToUsers(new TradeStartComposer(this)); } - public void offerItem(Habbo habbo, HabboItem item) { + public synchronized void offerItem(Habbo habbo, HabboItem item) { RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); if (user.getItems().contains(item)) @@ -67,7 +68,7 @@ public class RoomTrade { this.updateWindow(); } - public void offerMultipleItems(Habbo habbo, THashSet items) { + public synchronized void offerMultipleItems(Habbo habbo, THashSet items) { RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); for (HabboItem item : items) { @@ -81,7 +82,7 @@ public class RoomTrade { this.updateWindow(); } - public void removeItem(Habbo habbo, HabboItem item) { + public synchronized void removeItem(Habbo habbo, HabboItem item) { RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); if (!user.getItems().contains(item)) @@ -94,7 +95,7 @@ public class RoomTrade { this.updateWindow(); } - public void accept(Habbo habbo, boolean value) { + public synchronized void accept(Habbo habbo, boolean value) { RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); user.setAccepted(value); @@ -110,7 +111,13 @@ public class RoomTrade { } } - public void confirm(Habbo habbo) { + public synchronized void confirm(Habbo habbo) { + // Re-entry guard: both participants confirm on their own EventLoop + // threads. Without this (and the method-level lock) two concurrent + // confirms could each observe "all confirmed" and run tradeItems() + // twice → item/credit duplication. + if (this.completed) return; + RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); user.confirm(); @@ -122,6 +129,8 @@ public class RoomTrade { accepted = false; } if (accepted) { + this.completed = true; + if (this.tradeItems()) { this.closeWindow(); this.sendMessageToUsers(new TradeCompleteComposer()); diff --git a/Emulator/src/main/java/com/eu/habbo/habbohotel/users/HabboInfo.java b/Emulator/src/main/java/com/eu/habbo/habbohotel/users/HabboInfo.java index 373642be..aa441a05 100644 --- a/Emulator/src/main/java/com/eu/habbo/habbohotel/users/HabboInfo.java +++ b/Emulator/src/main/java/com/eu/habbo/habbohotel/users/HabboInfo.java @@ -55,6 +55,11 @@ public class HabboInfo implements Runnable { private RideablePet riding; private Class currentGame; private TIntIntHashMap currencies; + // Serializes credits + currencies read-modify-write and the saveCurrencies + // snapshot so the credit-roller thread and purchase/trade handler threads + // can't lose updates or rehash the Trove map mid-iteration. Never held + // across run()'s DB I/O. + private final Object currencyLock = new Object(); private GamePlayer gamePlayer; private int photoRoomId; private int photoTimestamp; @@ -123,11 +128,16 @@ public class HabboInfo implements Runnable { } private void saveCurrencies() { - List entries = new ArrayList<>(this.currencies.size()); - this.currencies.forEachEntry((type, amount) -> { - entries.add(new int[]{type, amount}); - return true; - }); + // Snapshot under the lock so a concurrent adjustOrPutValue/put can't + // rehash the Trove map while we iterate; do the DB batch off-lock. + List entries; + synchronized (this.currencyLock) { + entries = new ArrayList<>(this.currencies.size()); + this.currencies.forEachEntry((type, amount) -> { + entries.add(new int[]{type, amount}); + return true; + }); + } try { SqlQueries.batchUpdate( @@ -238,7 +248,9 @@ public class HabboInfo implements Runnable { } public int getCurrencyAmount(int type) { - return this.currencies.get(type); + synchronized (this.currencyLock) { + return this.currencies.get(type); + } } public TIntIntHashMap getCurrencies() { @@ -246,12 +258,16 @@ public class HabboInfo implements Runnable { } public void addCurrencyAmount(int type, int amount) { - this.currencies.adjustOrPutValue(type, amount, amount); + synchronized (this.currencyLock) { + this.currencies.adjustOrPutValue(type, amount, amount); + } this.run(); } public void setCurrencyAmount(int type, int amount) { - this.currencies.put(type, amount); + synchronized (this.currencyLock) { + this.currencies.put(type, amount); + } this.run(); } @@ -384,16 +400,22 @@ public class HabboInfo implements Runnable { } public int getCredits() { - return this.credits; + synchronized (this.currencyLock) { + return this.credits; + } } public void setCredits(int credits) { - this.credits = credits; + synchronized (this.currencyLock) { + this.credits = credits; + } this.run(); } public void addCredits(int credits) { - this.credits += credits; + synchronized (this.currencyLock) { + this.credits += credits; + } this.run(); } diff --git a/Emulator/src/main/java/com/eu/habbo/habbohotel/users/HabboStats.java b/Emulator/src/main/java/com/eu/habbo/habbohotel/users/HabboStats.java index b45c372f..cbd84b82 100644 --- a/Emulator/src/main/java/com/eu/habbo/habbohotel/users/HabboStats.java +++ b/Emulator/src/main/java/com/eu/habbo/habbohotel/users/HabboStats.java @@ -460,6 +460,16 @@ public class HabboStats implements Runnable { } } + /** Atomic read-add-write so concurrent progress sources don't lose updates. Returns the new total. */ + public int incrementProgress(Achievement achievement, int amount) { + synchronized (this.achievementProgress) { + Integer current = this.achievementProgress.get(achievement); + int next = (current != null ? current : 0) + amount; + this.achievementProgress.put(achievement, next); + return next; + } + } + public int getRentedTimeEnd() { return this.rentedTimeEnd; } diff --git a/Emulator/src/main/java/com/eu/habbo/messages/incoming/catalog/CatalogBuyItemEvent.java b/Emulator/src/main/java/com/eu/habbo/messages/incoming/catalog/CatalogBuyItemEvent.java index a80067b7..fdf052bd 100644 --- a/Emulator/src/main/java/com/eu/habbo/messages/incoming/catalog/CatalogBuyItemEvent.java +++ b/Emulator/src/main/java/com/eu/habbo/messages/incoming/catalog/CatalogBuyItemEvent.java @@ -53,6 +53,15 @@ public class CatalogBuyItemEvent extends MessageHandler { String extraData = this.packet.readString(); int count = this.packet.readInt(); + // Clamp the client-supplied quantity. Without this the club-offer + // branch accumulates cost in plain ints and a huge count overflows + // to a negative total, bypassing the affordability checks and + // CREDITING the buyer (free currency/subscription exploit). + if (count < 1 || count > 100) { + this.client.sendResponse(new AlertPurchaseFailedComposer(AlertPurchaseFailedComposer.SERVER_ERROR).compose()); + return; + } + try { if (this.client.getHabbo().getInventory().getItemsComponent().itemCount() > HabboInventory.MAXIMUM_ITEMS) { this.client.sendResponse(new AlertPurchaseFailedComposer(AlertPurchaseFailedComposer.SERVER_ERROR).compose()); diff --git a/Emulator/src/main/java/com/eu/habbo/messages/incoming/housekeeping/HousekeepingGiveCreditsEvent.java b/Emulator/src/main/java/com/eu/habbo/messages/incoming/housekeeping/HousekeepingGiveCreditsEvent.java index e0d16e2d..50a04b24 100644 --- a/Emulator/src/main/java/com/eu/habbo/messages/incoming/housekeeping/HousekeepingGiveCreditsEvent.java +++ b/Emulator/src/main/java/com/eu/habbo/messages/incoming/housekeeping/HousekeepingGiveCreditsEvent.java @@ -12,6 +12,7 @@ import java.sql.SQLException; public class HousekeepingGiveCreditsEvent extends MessageHandler { private static final String ACTION_KEY = "user.give_credits"; + private static final int MAX_GRANT = 1_000_000_000; @Override public int getRatelimit() { @@ -27,7 +28,7 @@ public class HousekeepingGiveCreditsEvent extends MessageHandler { int userId = this.packet.readInt(); int amount = this.packet.readInt(); - if (userId <= 0 || amount == 0) { + if (userId <= 0 || amount == 0 || amount < -MAX_GRANT || amount > MAX_GRANT) { this.client.sendResponse(new HousekeepingActionResultComposer(ACTION_KEY, false, 0, "housekeeping.error.invalid_input")); return; } diff --git a/Emulator/src/main/java/com/eu/habbo/messages/incoming/housekeeping/HousekeepingGiveCurrencyEvent.java b/Emulator/src/main/java/com/eu/habbo/messages/incoming/housekeeping/HousekeepingGiveCurrencyEvent.java index ba347b93..afe2df66 100644 --- a/Emulator/src/main/java/com/eu/habbo/messages/incoming/housekeeping/HousekeepingGiveCurrencyEvent.java +++ b/Emulator/src/main/java/com/eu/habbo/messages/incoming/housekeeping/HousekeepingGiveCurrencyEvent.java @@ -18,6 +18,7 @@ import java.sql.SQLException; */ public class HousekeepingGiveCurrencyEvent extends MessageHandler { private static final int CURRENCY_DUCKETS = 0; + private static final int MAX_GRANT = 1_000_000_000; @Override public int getRatelimit() { @@ -36,7 +37,7 @@ public class HousekeepingGiveCurrencyEvent extends MessageHandler { String actionKey = "user.give_currency_" + currencyType; - if (userId <= 0 || amount == 0) { + if (userId <= 0 || amount == 0 || amount < -MAX_GRANT || amount > MAX_GRANT) { this.client.sendResponse(new HousekeepingActionResultComposer(actionKey, false, 0, "housekeeping.error.invalid_input")); return; } diff --git a/Emulator/src/main/java/com/eu/habbo/networking/gameserver/auth/AuthRateLimiter.java b/Emulator/src/main/java/com/eu/habbo/networking/gameserver/auth/AuthRateLimiter.java index 67cabf17..1d0186e3 100644 --- a/Emulator/src/main/java/com/eu/habbo/networking/gameserver/auth/AuthRateLimiter.java +++ b/Emulator/src/main/java/com/eu/habbo/networking/gameserver/auth/AuthRateLimiter.java @@ -11,8 +11,34 @@ public final class AuthRateLimiter { private static final Map> STATE = new ConcurrentHashMap<>(); private static final Map> PROBE_STATE = new ConcurrentHashMap<>(); + // Both maps are keyed by client IP and reachable by unauthenticated traffic. + // recordSuccess removes STATE on login, but failed-only and probe-only IPs + // never get removed otherwise — unbounded growth over the JVM lifetime. + // Opportunistically evict window-expired entries once the maps get large. + private static final int SWEEP_THRESHOLD = 10_000; + private static final long SWEEP_MIN_INTERVAL_MS = 60_000L; + private static volatile long lastSweepMillis = 0L; + private AuthRateLimiter() {} + private static void maybeSweep(long now) { + if (STATE.size() < SWEEP_THRESHOLD && PROBE_STATE.size() < SWEEP_THRESHOLD) return; + if (now - lastSweepMillis < SWEEP_MIN_INTERVAL_MS) return; + lastSweepMillis = now; + + long stateWindowMs = configInt("login.ratelimit.window_sec", 60) * 1000L; + STATE.entrySet().removeIf(e -> { + State s = e.getValue().get(); + return s == null || (s.lockedUntilMillis <= now && (now - s.windowStartMillis) > stateWindowMs); + }); + + long probeWindowMs = configInt("login.probe.window_sec", 60) * 1000L; + PROBE_STATE.entrySet().removeIf(e -> { + ProbeState p = e.getValue().get(); + return p == null || (now - p.windowStartMillis) > probeWindowMs; + }); + } + public static boolean isLocked(String ip) { if (!isEnabled() || ip == null || ip.isEmpty()) return false; @@ -38,6 +64,7 @@ public final class AuthRateLimiter { if (!isEnabled() || ip == null || ip.isEmpty()) return; long now = System.currentTimeMillis(); + maybeSweep(now); long windowMs = configInt("login.ratelimit.window_sec", 60) * 1000L; int maxAttempts = configInt("login.ratelimit.max_attempts", 5); long lockoutMs = configInt("login.ratelimit.lockout_sec", 120) * 1000L; @@ -64,6 +91,7 @@ public final class AuthRateLimiter { if (isLocked(ip)) return false; long now = System.currentTimeMillis(); + maybeSweep(now); long windowMs = configInt("login.probe.window_sec", 60) * 1000L; int maxAttempts = configInt("login.probe.max_attempts", 20); diff --git a/Emulator/src/main/java/com/eu/habbo/networking/gameserver/decoders/GameMessageRateLimit.java b/Emulator/src/main/java/com/eu/habbo/networking/gameserver/decoders/GameMessageRateLimit.java index 1b83a3ee..85967273 100644 --- a/Emulator/src/main/java/com/eu/habbo/networking/gameserver/decoders/GameMessageRateLimit.java +++ b/Emulator/src/main/java/com/eu/habbo/networking/gameserver/decoders/GameMessageRateLimit.java @@ -23,7 +23,12 @@ public class GameMessageRateLimit extends MessageToMessageDecoder protected void decode(ChannelHandlerContext ctx, ClientMessage message, List out) throws Exception { GameClient client = ctx.channel().attr(GameServerAttributes.CLIENT).get(); + // ClientMessage is not ReferenceCounted, so MessageToMessageDecoder's + // auto-release is a no-op for it; on every drop path we must release the + // wrapped ByteBuf ourselves or it leaks (it is only released downstream + // in ChannelReadHandler on the success path). if (client == null) { + message.release(); return; } @@ -42,6 +47,7 @@ public class GameMessageRateLimit extends MessageToMessageDecoder } if (count > MAX_COUNTER) { + message.release(); return; } @@ -53,6 +59,7 @@ public class GameMessageRateLimit extends MessageToMessageDecoder LOGGER.warn("Global packet rate limit exceeded for {} ({} packets/sec) — dropping excess packets", username, globalCount); } + message.release(); return; }