fix: address bug-hunt findings across security, concurrency, trade & wired

Security
- HousekeepingSetUserRankEvent: add rank-ceiling guard (reject granting a
  rank above the operator's own and modifying a higher-ranked target),
  mirroring GiveRankCommand — closes a privilege-escalation path.

Trade integrity
- RoomTrade.clearAccepted now also resets confirmed; a stale confirmed=true
  let a user strip their side and still complete once the partner re-confirms.

Concurrency
- RoomCycleManager: iterate the synchronized bot/pet maps under their own
  monitor (lock order stays one-directional vs addBot/addPet — no deadlock).
- RoomSpecialTypes: synchronize nest/petDrink/petFood/petToy/petTree writers
  on the same monitor their getters already use.
- HabboStats: synchronize achievement-progress map accessors.
- RebugKickBallAction: drop redundant direct mutation of the shared tile-cache
  sets (updateTile invalidates them right after) — removes a data race.

Robustness
- Wired legacy parsers (HabboCount, NotHabboCount, MatchStatePosition,
  MoveRotateFurni): guard length/format so one malformed row no longer aborts
  the whole room's wired load.
- RoomLayout: fill malformed/short heightmap rows with INVALID tiles instead
  of leaving nulls, and bounds-check door coordinates.
- FurnidataWatcher: defer (instead of drop) a throttled delta so furni-name
  changes are never lost between broadcasts.
- GuildManager.getGuildMembers: fix LIMIT row-count (page size 14, not
  offset+14) so member pages no longer overlap from page 1 on.
This commit is contained in:
simoleo89
2026-06-08 22:12:52 +00:00
parent 48fcd3f78b
commit da1fd01074
14 changed files with 230 additions and 111 deletions
@@ -423,7 +423,7 @@ public class GuildManager {
statement.setInt(1, guild.getId());
statement.setString(2, "%" + query + "%");
statement.setInt(3, page * 14);
statement.setInt(4, (page * 14) + 14);
statement.setInt(4, 14);
try (ResultSet set = statement.executeQuery()) {
while (set.next()) {
@@ -115,32 +115,39 @@ public class FurnidataWatcher {
}
}
private void onChange() {
private void onChange() throws InterruptedException {
// Re-index under the shared furnidata lock so the watcher and editor
// writes never swap the index concurrently. The lock is released before
// the throttle/broadcast below so a slow broadcast can't stall editor saves.
List<FurnidataEntry> delta;
FurnidataLock.LOCK.lock();
try {
Path source = this.provider.getSource();
if (source == null) return;
List<FurnidataEntry> delta = this.provider.reindex(new FurnidataReader(source, this.maxBytes).read());
if (delta.isEmpty()) return;
long now = System.currentTimeMillis();
if (now - this.lastBroadcast < this.minIntervalMs) {
LOGGER.info("FurnidataWatcher: {} changes indexed but broadcast skipped (min interval) — clients update on next change or reconnect", delta.size());
return;
}
this.lastBroadcast = now;
FurnitureDataReloadComposer composer = (delta.size() > this.deltaCap)
? new FurnitureDataReloadComposer(FurnitureDataReloadComposer.MODE_RELOAD_HINT, List.of())
: new FurnitureDataReloadComposer(FurnitureDataReloadComposer.MODE_DELTA, delta);
broadcast(composer);
LOGGER.info("FurnidataWatcher: broadcast {} ({} entries)",
delta.size() > this.deltaCap ? "reload-hint" : "delta", delta.size());
delta = this.provider.reindex(new FurnidataReader(source, this.maxBytes).read());
} finally {
FurnidataLock.LOCK.unlock();
}
if (delta.isEmpty()) return;
// Min-interval throttle: the index has already been swapped, so we must
// not drop this delta (the next reindex would diff against the updated
// index and never re-emit it). Instead, defer the broadcast until the
// interval elapses. Running on a dedicated daemon thread, sleeping is
// safe; file events arriving meanwhile coalesce into the next cycle.
long sinceLast = System.currentTimeMillis() - this.lastBroadcast;
if (sinceLast < this.minIntervalMs) {
Thread.sleep(this.minIntervalMs - sinceLast);
}
this.lastBroadcast = System.currentTimeMillis();
FurnitureDataReloadComposer composer = (delta.size() > this.deltaCap)
? new FurnitureDataReloadComposer(FurnitureDataReloadComposer.MODE_RELOAD_HINT, List.of())
: new FurnitureDataReloadComposer(FurnitureDataReloadComposer.MODE_DELTA, delta);
broadcast(composer);
LOGGER.info("FurnidataWatcher: broadcast {} ({} entries)",
delta.size() > this.deltaCap ? "reload-hint" : "delta", delta.size());
}
private void broadcast(FurnitureDataReloadComposer composer) {
@@ -65,8 +65,14 @@ public class WiredConditionHabboCount extends InteractionWiredCondition {
} else {
String[] data = wiredData.split(":");
this.lowerLimit = Integer.parseInt(data[0]);
this.upperLimit = Integer.parseInt(data[1]);
if (data.length >= 2) {
try {
this.lowerLimit = Integer.parseInt(data[0].trim());
this.upperLimit = Integer.parseInt(data[1].trim());
} catch (NumberFormatException ignored) {
// malformed legacy data — keep the constructed defaults
}
}
this.userSource = WiredSourceUtil.SOURCE_TRIGGER;
}
}
@@ -263,22 +263,29 @@ public class WiredConditionMatchStatePosition extends InteractionWiredCondition
} else {
String[] data = wiredData.split(":");
int itemCount = Integer.parseInt(data[0]);
if (data.length >= 5) {
try {
int itemCount = Integer.parseInt(data[0]);
String[] items = data[1].split(";");
String[] items = data[1].split(";");
for (int i = 0; i < itemCount; i++) {
String[] stuff = items[i].split("-");
for (int i = 0; i < itemCount && i < items.length; i++) {
String[] stuff = items[i].split("-");
if (stuff.length >= 6)
this.settings.add(new WiredMatchFurniSetting(Integer.parseInt(stuff[0]), stuff[1], Integer.parseInt(stuff[2]), Integer.parseInt(stuff[3]), Integer.parseInt(stuff[4]), Double.parseDouble(stuff[5])));
else if (stuff.length >= 5)
this.settings.add(new WiredMatchFurniSetting(Integer.parseInt(stuff[0]), stuff[1], Integer.parseInt(stuff[2]), Integer.parseInt(stuff[3]), Integer.parseInt(stuff[4])));
if (stuff.length >= 6)
this.settings.add(new WiredMatchFurniSetting(Integer.parseInt(stuff[0]), stuff[1], Integer.parseInt(stuff[2]), Integer.parseInt(stuff[3]), Integer.parseInt(stuff[4]), Double.parseDouble(stuff[5])));
else if (stuff.length >= 5)
this.settings.add(new WiredMatchFurniSetting(Integer.parseInt(stuff[0]), stuff[1], Integer.parseInt(stuff[2]), Integer.parseInt(stuff[3]), Integer.parseInt(stuff[4])));
}
this.state = data[2].equals("1");
this.direction = data[3].equals("1");
this.position = data[4].equals("1");
} catch (NumberFormatException ignored) {
// malformed legacy data — keep whatever was parsed plus defaults
}
}
this.state = data[2].equals("1");
this.direction = data[3].equals("1");
this.position = data[4].equals("1");
this.altitude = false;
this.furniSource = this.settings.isEmpty() ? WiredSourceUtil.SOURCE_TRIGGER : WiredSourceUtil.SOURCE_SELECTED;
this.quantifier = QUANTIFIER_ALL;
@@ -64,8 +64,14 @@ public class WiredConditionNotHabboCount extends InteractionWiredCondition {
this.userSource = data.userSource;
} else {
String[] data = wiredData.split(":");
this.lowerLimit = Integer.parseInt(data[0]);
this.upperLimit = Integer.parseInt(data[1]);
if (data.length >= 2) {
try {
this.lowerLimit = Integer.parseInt(data[0].trim());
this.upperLimit = Integer.parseInt(data[1].trim());
} catch (NumberFormatException ignored) {
// malformed legacy data — keep the constructed defaults
}
}
this.userSource = WiredSourceUtil.SOURCE_TRIGGER;
}
}
@@ -190,10 +190,15 @@ public class WiredEffectMoveRotateFurni extends InteractionWiredEffect implement
}
for (String s : data[3].split("\r")) {
HabboItem item = room.getHabboItem(Integer.parseInt(s));
if (s.trim().isEmpty()) continue;
try {
HabboItem item = room.getHabboItem(Integer.parseInt(s.trim()));
if (item != null)
this.items.add(item);
if (item != null)
this.items.add(item);
} catch (NumberFormatException ignored) {
// skip malformed furni id token
}
}
}
this.furniSource = this.items.isEmpty() ? WiredSourceUtil.SOURCE_TRIGGER : WiredSourceUtil.SOURCE_SELECTED;
@@ -300,32 +300,37 @@ public class RoomCycleManager {
return;
}
TIntObjectIterator<Bot> botIterator = currentBots.iterator();
for (int i = currentBots.size(); i-- > 0; ) {
try {
final Bot bot;
// currentBots is a TCollections.synchronizedMap; the iterator is not
// safe against concurrent put/remove from IO threads (bot place/pickup),
// so we must hold the map's monitor for the whole traversal.
synchronized (currentBots) {
TIntObjectIterator<Bot> botIterator = currentBots.iterator();
for (int i = currentBots.size(); i-- > 0; ) {
try {
botIterator.advance();
bot = botIterator.value();
} catch (Exception e) {
final Bot bot;
try {
botIterator.advance();
bot = botIterator.value();
} catch (Exception e) {
break;
}
if (!this.room.isAllowBotsWalk() && bot.getRoomUnit().isWalking()) {
bot.getRoomUnit().stopWalking();
updatedUnit.add(bot.getRoomUnit());
continue;
}
bot.cycle(this.room.isAllowBotsWalk());
if (this.cycleRoomUnit(bot.getRoomUnit(), RoomUnitType.BOT)) {
updatedUnit.add(bot.getRoomUnit());
}
} catch (NoSuchElementException e) {
LOGGER.error("Caught exception", e);
break;
}
if (!this.room.isAllowBotsWalk() && bot.getRoomUnit().isWalking()) {
bot.getRoomUnit().stopWalking();
updatedUnit.add(bot.getRoomUnit());
continue;
}
bot.cycle(this.room.isAllowBotsWalk());
if (this.cycleRoomUnit(bot.getRoomUnit(), RoomUnitType.BOT)) {
updatedUnit.add(bot.getRoomUnit());
}
} catch (NoSuchElementException e) {
LOGGER.error("Caught exception", e);
break;
}
}
}
@@ -339,31 +344,35 @@ public class RoomCycleManager {
return;
}
TIntObjectIterator<Pet> petIterator = currentPets.iterator();
for (int i = currentPets.size(); i-- > 0; ) {
try {
petIterator.advance();
} catch (NoSuchElementException e) {
LOGGER.error("Caught exception", e);
break;
}
// currentPets is a TCollections.synchronizedMap; hold its monitor for the
// whole traversal to stay safe against concurrent pet place/pickup.
synchronized (currentPets) {
TIntObjectIterator<Pet> petIterator = currentPets.iterator();
for (int i = currentPets.size(); i-- > 0; ) {
try {
petIterator.advance();
} catch (NoSuchElementException e) {
LOGGER.error("Caught exception", e);
break;
}
Pet pet = petIterator.value();
if (this.cycleRoomUnit(pet.getRoomUnit(), RoomUnitType.PET)) {
updatedUnit.add(pet.getRoomUnit());
}
Pet pet = petIterator.value();
if (this.cycleRoomUnit(pet.getRoomUnit(), RoomUnitType.PET)) {
updatedUnit.add(pet.getRoomUnit());
}
pet.cycle();
pet.cycle();
if (pet.packetUpdate) {
updatedUnit.add(pet.getRoomUnit());
pet.packetUpdate = false;
}
if (pet.packetUpdate) {
updatedUnit.add(pet.getRoomUnit());
pet.packetUpdate = false;
}
if (pet.getRoomUnit().isWalking() && pet.getRoomUnit().getPath().size() == 1
&& pet.getRoomUnit().hasStatus(RoomUnitStatus.GESTURE)) {
pet.getRoomUnit().removeStatus(RoomUnitStatus.GESTURE);
updatedUnit.add(pet.getRoomUnit());
if (pet.getRoomUnit().isWalking() && pet.getRoomUnit().getPath().size() == 1
&& pet.getRoomUnit().hasStatus(RoomUnitStatus.GESTURE)) {
pet.getRoomUnit().removeStatus(RoomUnitStatus.GESTURE);
updatedUnit.add(pet.getRoomUnit());
}
}
}
}
@@ -130,13 +130,16 @@ public class RoomLayout {
this.roomTiles = new RoomTile[this.mapSizeX][this.mapSizeY];
for (short y = 0; y < this.mapSizeY; y++) {
if (modelTemp[y].isEmpty() || modelTemp[y].equalsIgnoreCase("\r")) {
continue;
}
// A row shorter/longer than the model width (or empty) cannot be parsed
// per-square. Previously such tiles were left null while tileExists()
// still reported them present, causing NPEs in the coordinate accessors.
// Fill them with INVALID tiles so every in-bounds coordinate is non-null.
boolean validRow = !modelTemp[y].isEmpty() && modelTemp[y].length() == this.mapSizeX;
for (short x = 0; x < this.mapSizeX; x++) {
if (modelTemp[y].length() != this.mapSizeX) {
break;
if (!validRow) {
this.roomTiles[x][y] = new RoomTile(x, y, (short) 0, RoomTileState.INVALID, true);
continue;
}
String square = modelTemp[y].substring(x, x + 1).trim().toLowerCase();
@@ -159,7 +162,9 @@ public class RoomLayout {
}
}
this.doorTile = this.roomTiles[this.doorX][this.doorY];
this.doorTile = (this.doorX >= 0 && this.doorX < this.mapSizeX && this.doorY >= 0 && this.doorY < this.mapSizeY)
? this.roomTiles[this.doorX][this.doorY]
: null;
if (this.doorTile != null) {
this.doorTile.setAllowStack(false);
@@ -156,11 +156,17 @@ public class RoomSpecialTypes {
}
public void addNest(InteractionNest item) {
this.nests.put(item.getId(), item); this.specialItemsById.put(item.getId(), item);
synchronized (this.nests) {
this.nests.put(item.getId(), item);
}
this.specialItemsById.put(item.getId(), item);
}
public void removeNest(InteractionNest item) {
this.nests.remove(item.getId()); this.specialItemsById.remove(item.getId());
synchronized (this.nests) {
this.nests.remove(item.getId());
}
this.specialItemsById.remove(item.getId());
}
public THashSet<InteractionNest> getNests() {
@@ -178,11 +184,17 @@ public class RoomSpecialTypes {
}
public void addPetDrink(InteractionPetDrink item) {
this.petDrinks.put(item.getId(), item); this.specialItemsById.put(item.getId(), item);
synchronized (this.petDrinks) {
this.petDrinks.put(item.getId(), item);
}
this.specialItemsById.put(item.getId(), item);
}
public void removePetDrink(InteractionPetDrink item) {
this.petDrinks.remove(item.getId()); this.specialItemsById.remove(item.getId());
synchronized (this.petDrinks) {
this.petDrinks.remove(item.getId());
}
this.specialItemsById.remove(item.getId());
}
public THashSet<InteractionPetDrink> getPetDrinks() {
@@ -200,11 +212,17 @@ public class RoomSpecialTypes {
}
public void addPetFood(InteractionPetFood item) {
this.petFoods.put(item.getId(), item); this.specialItemsById.put(item.getId(), item);
synchronized (this.petFoods) {
this.petFoods.put(item.getId(), item);
}
this.specialItemsById.put(item.getId(), item);
}
public void removePetFood(InteractionPetFood petFood) {
this.petFoods.remove(petFood.getId()); this.specialItemsById.remove(petFood.getId());
synchronized (this.petFoods) {
this.petFoods.remove(petFood.getId());
}
this.specialItemsById.remove(petFood.getId());
}
public THashSet<InteractionPetFood> getPetFoods() {
@@ -222,11 +240,17 @@ public class RoomSpecialTypes {
}
public void addPetToy(InteractionPetToy item) {
this.petToys.put(item.getId(), item); this.specialItemsById.put(item.getId(), item);
synchronized (this.petToys) {
this.petToys.put(item.getId(), item);
}
this.specialItemsById.put(item.getId(), item);
}
public void removePetToy(InteractionPetToy petToy) {
this.petToys.remove(petToy.getId()); this.specialItemsById.remove(petToy.getId());
synchronized (this.petToys) {
this.petToys.remove(petToy.getId());
}
this.specialItemsById.remove(petToy.getId());
}
public THashSet<InteractionPetToy> getPetToys() {
@@ -244,11 +268,17 @@ public class RoomSpecialTypes {
}
public void addPetTree(InteractionPetTree item) {
this.petTrees.put(item.getId(), item); this.specialItemsById.put(item.getId(), item);
synchronized (this.petTrees) {
this.petTrees.put(item.getId(), item);
}
this.specialItemsById.put(item.getId(), item);
}
public void removePetTree(InteractionPetTree petTree) {
this.petTrees.remove(petTree.getId()); this.specialItemsById.remove(petTree.getId());
synchronized (this.petTrees) {
this.petTrees.remove(petTree.getId());
}
this.specialItemsById.remove(petTree.getId());
}
public THashSet<InteractionPetTree> getPetTrees() {
@@ -264,6 +264,10 @@ public class RoomTrade {
protected void clearAccepted() {
for (RoomTradeUser user : this.users) {
user.setAccepted(false);
// Any change to the offered items invalidates a prior confirmation;
// without this a stale confirmed=true lets a user strip their side
// and still complete the trade once the partner re-confirms.
user.setConfirmed(false);
}
}
@@ -51,6 +51,10 @@ public class RoomTradeUser {
this.confirmed = true;
}
public void setConfirmed(boolean value) {
this.confirmed = value;
}
public void addItem(HabboItem item) {
this.items.add(item);
}
@@ -448,14 +448,16 @@ public class HabboStats implements Runnable {
return 0;
}
if (this.achievementProgress.containsKey(achievement))
return this.achievementProgress.get(achievement);
return -1;
synchronized (this.achievementProgress) {
Integer progress = this.achievementProgress.get(achievement);
return progress != null ? progress : -1;
}
}
public void setProgress(Achievement achievement, int progress) {
this.achievementProgress.put(achievement, progress);
synchronized (this.achievementProgress) {
this.achievementProgress.put(achievement, progress);
}
}
public int getRentedTimeEnd() {
@@ -11,6 +11,7 @@ import com.eu.habbo.messages.outgoing.users.UserPermissionsComposer;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
public class HousekeepingSetUserRankEvent extends MessageHandler {
@@ -44,6 +45,43 @@ public class HousekeepingSetUserRankEvent extends MessageHandler {
Rank rank = permissions.getRank(rankId);
// Rank-ceiling guard: an operator must never be able to grant a rank
// above their own, nor modify a user who already outranks them. This
// mirrors GiveRankCommand and prevents privilege escalation through
// the housekeeping path (including self-promotion).
int operatorRankId = this.client.getHabbo().getHabboInfo().getRank().getId();
if (rank.getId() > operatorRankId) {
this.client.sendResponse(new HousekeepingActionResultComposer(ACTION_KEY, false, 0, "housekeeping.error.rank_too_high"));
return;
}
Habbo online = Emulator.getGameEnvironment().getHabboManager().getHabbo(userId);
int targetRankId;
if (online != null) {
targetRankId = online.getHabboInfo().getRank().getId();
} else {
targetRankId = 0;
try (Connection connection = Emulator.getDatabase().getDataSource().getConnection();
PreparedStatement statement = connection.prepareStatement("SELECT rank FROM users WHERE id = ? LIMIT 1")) {
statement.setInt(1, userId);
try (ResultSet set = statement.executeQuery()) {
if (set.next()) {
targetRankId = set.getInt("rank");
}
}
} catch (SQLException e) {
this.client.sendResponse(new HousekeepingActionResultComposer(ACTION_KEY, false, 0, "housekeeping.error.db_failed"));
return;
}
}
if (targetRankId > operatorRankId) {
this.client.sendResponse(new HousekeepingActionResultComposer(ACTION_KEY, false, 0, "housekeeping.error.rank_too_high"));
return;
}
// Persist for the offline path. Online users get their in-memory
// HabboInfo.rank rebound below so server-side hasPermission()
// checks land on the new permission set without a relogin.
@@ -57,8 +95,6 @@ public class HousekeepingSetUserRankEvent extends MessageHandler {
return;
}
Habbo online = Emulator.getGameEnvironment().getHabboManager().getHabbo(userId);
if (online != null) {
online.getHabboInfo().setRank(rank);
// Ship the refreshed permissions snapshot — same payload the
@@ -165,12 +165,10 @@ public class RebugKickBallAction implements Runnable {
this.dead = true;
}
THashSet<HabboItem> oldItems = this.room.getItemsAt(oldTile);
if (oldItems != null && !oldItems.isEmpty()) {
oldItems.remove(this.ball);
}
this.room.getItemsAt(nextTile).add(this.ball);
// updateTile() below removes both tiles from the item cache (rebuilt
// lazily from the ball's already-updated position), so mutating the
// shared cached THashSets here is both redundant and a data race
// against the room-cycle/IO threads iterating those same sets.
this.room.updateTile(oldTile);
this.room.updateTile(nextTile);