fix: economy-integrity, currency thread-safety, and resource-leak hardening

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).
This commit is contained in:
simoleo89
2026-06-09 11:02:09 +00:00
parent c98d3a3205
commit d1570d3574
13 changed files with 142 additions and 38 deletions
@@ -79,6 +79,14 @@ class DatabasePool {
databaseConfiguration.addDataSourceProperty("useLocalSessionState", "true"); databaseConfiguration.addDataSourceProperty("useLocalSessionState", "true");
databaseConfiguration.addDataSourceProperty("cacheResultSetMetadata", "true"); databaseConfiguration.addDataSourceProperty("cacheResultSetMetadata", "true");
databaseConfiguration.addDataSourceProperty("elideSetAutoCommits", "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.addDataSourceProperty("maintainTimeStats", "false");
databaseConfiguration.setPoolName("HabboHikariPool"); databaseConfiguration.setPoolName("HabboHikariPool");
@@ -100,9 +100,9 @@ public class AchievementManager {
if (oldLevel != null && (oldLevel.level == achievement.levels.size() && currentProgress >= oldLevel.progress)) //Maximum achievement gotten. if (oldLevel != null && (oldLevel.level == achievement.levels.size() && currentProgress >= oldLevel.progress)) //Maximum achievement gotten.
return; 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) { if (AchievementManager.TALENTTRACK_ENABLED) {
for (TalentTrackType type : TalentTrackType.values()) { for (TalentTrackType type : TalentTrackType.values()) {
@@ -101,20 +101,26 @@ public class ForumThread implements Runnable, ISerialize {
if (statement.executeUpdate() < 1) if (statement.executeUpdate() < 1)
return null; return null;
ResultSet set = statement.getGeneratedKeys(); try (ResultSet set = statement.getGeneratedKeys()) {
if (set.next()) { if (set.next()) {
int threadId = set.getInt(1); int threadId = set.getInt(1);
createdThread = new ForumThread(threadId, guild.getId(), opener.getHabboInfo().getId(), subject, 0, timestamp, timestamp, ForumThreadState.OPEN, false, false, 0, null); createdThread = new ForumThread(threadId, guild.getId(), opener.getHabboInfo().getId(), subject, 0, timestamp, timestamp, ForumThreadState.OPEN, false, false, 0, null);
cacheThread(createdThread); 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); ForumThreadComment comment = ForumThreadComment.create(createdThread, opener, message);
createdThread.addComment(comment); createdThread.addComment(comment);
Emulator.getPluginManager().fireEvent(new GuildForumThreadCreated(createdThread)); Emulator.getPluginManager().fireEvent(new GuildForumThreadCreated(createdThread));
} }
} catch (SQLException e) {
LOGGER.error("Caught SQL exception", e);
}
return createdThread; return createdThread;
} }
@@ -98,13 +98,14 @@ public class ForumThreadComment implements Runnable, ISerialize {
if (statement.executeUpdate() < 1) if (statement.executeUpdate() < 1)
return null; return null;
ResultSet set = statement.getGeneratedKeys(); try (ResultSet set = statement.getGeneratedKeys()) {
if (set.next()) { if (set.next()) {
int commentId = set.getInt(1); int commentId = set.getInt(1);
createdComment = new ForumThreadComment(commentId, thread.getThreadId(), poster.getHabboInfo().getId(), message, timestamp, ForumThreadState.OPEN, 0); 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) { } catch (SQLException e) {
LOGGER.error("Caught SQL exception", e); LOGGER.error("Caught SQL exception", e);
} }
@@ -158,10 +158,12 @@ public class Room implements Comparable<Room>, ISerialize, Runnable {
private String tags; private String tags;
private boolean publicRoom; private boolean publicRoom;
private boolean staffPromotedRoom; private boolean staffPromotedRoom;
private boolean allowPets; // Read every room cycle (processBots/processPets) but written from settings/
private boolean allowPetsEat; // admin packet handlers on another thread — volatile for cross-thread visibility.
private volatile boolean allowPets;
private volatile boolean allowPetsEat;
private boolean allowWalkthrough; private boolean allowWalkthrough;
private boolean allowBotsWalk; private volatile boolean allowBotsWalk;
private boolean allowEffects; private boolean allowEffects;
private boolean hideWall; private boolean hideWall;
private int chatMode; private int chatMode;
@@ -26,6 +26,7 @@ public class RoomTrade {
private final List<RoomTradeUser> users; private final List<RoomTradeUser> users;
private final Room room; private final Room room;
private boolean completed = false;
public RoomTrade(Habbo userOne, Habbo userTwo, Room room) { public RoomTrade(Habbo userOne, Habbo userTwo, Room room) {
this.users = new ArrayList<>(); this.users = new ArrayList<>();
@@ -54,7 +55,7 @@ public class RoomTrade {
this.sendMessageToUsers(new TradeStartComposer(this)); 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); RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo);
if (user.getItems().contains(item)) if (user.getItems().contains(item))
@@ -67,7 +68,7 @@ public class RoomTrade {
this.updateWindow(); this.updateWindow();
} }
public void offerMultipleItems(Habbo habbo, THashSet<HabboItem> items) { public synchronized void offerMultipleItems(Habbo habbo, THashSet<HabboItem> items) {
RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo);
for (HabboItem item : items) { for (HabboItem item : items) {
@@ -81,7 +82,7 @@ public class RoomTrade {
this.updateWindow(); this.updateWindow();
} }
public void removeItem(Habbo habbo, HabboItem item) { public synchronized void removeItem(Habbo habbo, HabboItem item) {
RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo);
if (!user.getItems().contains(item)) if (!user.getItems().contains(item))
@@ -94,7 +95,7 @@ public class RoomTrade {
this.updateWindow(); this.updateWindow();
} }
public void accept(Habbo habbo, boolean value) { public synchronized void accept(Habbo habbo, boolean value) {
RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo);
user.setAccepted(value); 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); RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo);
user.confirm(); user.confirm();
@@ -122,6 +129,8 @@ public class RoomTrade {
accepted = false; accepted = false;
} }
if (accepted) { if (accepted) {
this.completed = true;
if (this.tradeItems()) { if (this.tradeItems()) {
this.closeWindow(); this.closeWindow();
this.sendMessageToUsers(new TradeCompleteComposer()); this.sendMessageToUsers(new TradeCompleteComposer());
@@ -55,6 +55,11 @@ public class HabboInfo implements Runnable {
private RideablePet riding; private RideablePet riding;
private Class<? extends Game> currentGame; private Class<? extends Game> currentGame;
private TIntIntHashMap currencies; 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 GamePlayer gamePlayer;
private int photoRoomId; private int photoRoomId;
private int photoTimestamp; private int photoTimestamp;
@@ -123,11 +128,16 @@ public class HabboInfo implements Runnable {
} }
private void saveCurrencies() { private void saveCurrencies() {
List<int[]> entries = new ArrayList<>(this.currencies.size()); // 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<int[]> entries;
synchronized (this.currencyLock) {
entries = new ArrayList<>(this.currencies.size());
this.currencies.forEachEntry((type, amount) -> { this.currencies.forEachEntry((type, amount) -> {
entries.add(new int[]{type, amount}); entries.add(new int[]{type, amount});
return true; return true;
}); });
}
try { try {
SqlQueries.batchUpdate( SqlQueries.batchUpdate(
@@ -238,20 +248,26 @@ public class HabboInfo implements Runnable {
} }
public int getCurrencyAmount(int type) { public int getCurrencyAmount(int type) {
synchronized (this.currencyLock) {
return this.currencies.get(type); return this.currencies.get(type);
} }
}
public TIntIntHashMap getCurrencies() { public TIntIntHashMap getCurrencies() {
return this.currencies; return this.currencies;
} }
public void addCurrencyAmount(int type, int amount) { public void addCurrencyAmount(int type, int amount) {
synchronized (this.currencyLock) {
this.currencies.adjustOrPutValue(type, amount, amount); this.currencies.adjustOrPutValue(type, amount, amount);
}
this.run(); this.run();
} }
public void setCurrencyAmount(int type, int amount) { public void setCurrencyAmount(int type, int amount) {
synchronized (this.currencyLock) {
this.currencies.put(type, amount); this.currencies.put(type, amount);
}
this.run(); this.run();
} }
@@ -384,16 +400,22 @@ public class HabboInfo implements Runnable {
} }
public int getCredits() { public int getCredits() {
synchronized (this.currencyLock) {
return this.credits; return this.credits;
} }
}
public void setCredits(int credits) { public void setCredits(int credits) {
synchronized (this.currencyLock) {
this.credits = credits; this.credits = credits;
}
this.run(); this.run();
} }
public void addCredits(int credits) { public void addCredits(int credits) {
synchronized (this.currencyLock) {
this.credits += credits; this.credits += credits;
}
this.run(); this.run();
} }
@@ -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() { public int getRentedTimeEnd() {
return this.rentedTimeEnd; return this.rentedTimeEnd;
} }
@@ -53,6 +53,15 @@ public class CatalogBuyItemEvent extends MessageHandler {
String extraData = this.packet.readString(); String extraData = this.packet.readString();
int count = this.packet.readInt(); 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 { try {
if (this.client.getHabbo().getInventory().getItemsComponent().itemCount() > HabboInventory.MAXIMUM_ITEMS) { if (this.client.getHabbo().getInventory().getItemsComponent().itemCount() > HabboInventory.MAXIMUM_ITEMS) {
this.client.sendResponse(new AlertPurchaseFailedComposer(AlertPurchaseFailedComposer.SERVER_ERROR).compose()); this.client.sendResponse(new AlertPurchaseFailedComposer(AlertPurchaseFailedComposer.SERVER_ERROR).compose());
@@ -12,6 +12,7 @@ import java.sql.SQLException;
public class HousekeepingGiveCreditsEvent extends MessageHandler { public class HousekeepingGiveCreditsEvent extends MessageHandler {
private static final String ACTION_KEY = "user.give_credits"; private static final String ACTION_KEY = "user.give_credits";
private static final int MAX_GRANT = 1_000_000_000;
@Override @Override
public int getRatelimit() { public int getRatelimit() {
@@ -27,7 +28,7 @@ public class HousekeepingGiveCreditsEvent extends MessageHandler {
int userId = this.packet.readInt(); int userId = this.packet.readInt();
int amount = 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")); this.client.sendResponse(new HousekeepingActionResultComposer(ACTION_KEY, false, 0, "housekeeping.error.invalid_input"));
return; return;
} }
@@ -18,6 +18,7 @@ import java.sql.SQLException;
*/ */
public class HousekeepingGiveCurrencyEvent extends MessageHandler { public class HousekeepingGiveCurrencyEvent extends MessageHandler {
private static final int CURRENCY_DUCKETS = 0; private static final int CURRENCY_DUCKETS = 0;
private static final int MAX_GRANT = 1_000_000_000;
@Override @Override
public int getRatelimit() { public int getRatelimit() {
@@ -36,7 +37,7 @@ public class HousekeepingGiveCurrencyEvent extends MessageHandler {
String actionKey = "user.give_currency_" + currencyType; 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")); this.client.sendResponse(new HousekeepingActionResultComposer(actionKey, false, 0, "housekeeping.error.invalid_input"));
return; return;
} }
@@ -11,8 +11,34 @@ public final class AuthRateLimiter {
private static final Map<String, AtomicReference<State>> STATE = new ConcurrentHashMap<>(); private static final Map<String, AtomicReference<State>> STATE = new ConcurrentHashMap<>();
private static final Map<String, AtomicReference<ProbeState>> PROBE_STATE = new ConcurrentHashMap<>(); private static final Map<String, AtomicReference<ProbeState>> 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 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) { public static boolean isLocked(String ip) {
if (!isEnabled() || ip == null || ip.isEmpty()) return false; if (!isEnabled() || ip == null || ip.isEmpty()) return false;
@@ -38,6 +64,7 @@ public final class AuthRateLimiter {
if (!isEnabled() || ip == null || ip.isEmpty()) return; if (!isEnabled() || ip == null || ip.isEmpty()) return;
long now = System.currentTimeMillis(); long now = System.currentTimeMillis();
maybeSweep(now);
long windowMs = configInt("login.ratelimit.window_sec", 60) * 1000L; long windowMs = configInt("login.ratelimit.window_sec", 60) * 1000L;
int maxAttempts = configInt("login.ratelimit.max_attempts", 5); int maxAttempts = configInt("login.ratelimit.max_attempts", 5);
long lockoutMs = configInt("login.ratelimit.lockout_sec", 120) * 1000L; long lockoutMs = configInt("login.ratelimit.lockout_sec", 120) * 1000L;
@@ -64,6 +91,7 @@ public final class AuthRateLimiter {
if (isLocked(ip)) return false; if (isLocked(ip)) return false;
long now = System.currentTimeMillis(); long now = System.currentTimeMillis();
maybeSweep(now);
long windowMs = configInt("login.probe.window_sec", 60) * 1000L; long windowMs = configInt("login.probe.window_sec", 60) * 1000L;
int maxAttempts = configInt("login.probe.max_attempts", 20); int maxAttempts = configInt("login.probe.max_attempts", 20);
@@ -23,7 +23,12 @@ public class GameMessageRateLimit extends MessageToMessageDecoder<ClientMessage>
protected void decode(ChannelHandlerContext ctx, ClientMessage message, List<Object> out) throws Exception { protected void decode(ChannelHandlerContext ctx, ClientMessage message, List<Object> out) throws Exception {
GameClient client = ctx.channel().attr(GameServerAttributes.CLIENT).get(); 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) { if (client == null) {
message.release();
return; return;
} }
@@ -42,6 +47,7 @@ public class GameMessageRateLimit extends MessageToMessageDecoder<ClientMessage>
} }
if (count > MAX_COUNTER) { if (count > MAX_COUNTER) {
message.release();
return; return;
} }
@@ -53,6 +59,7 @@ public class GameMessageRateLimit extends MessageToMessageDecoder<ClientMessage>
LOGGER.warn("Global packet rate limit exceeded for {} ({} packets/sec) — dropping excess packets", LOGGER.warn("Global packet rate limit exceeded for {} ({} packets/sec) — dropping excess packets",
username, globalCount); username, globalCount);
} }
message.release();
return; return;
} }