Integer overflow in required claimblock calculation when using /claim
smallclock12 opened this issue · 9 comments
Observed Behavior
When specifying a radius greater than 23170 the amount of claim blocks required overflows and starts going down.
Some notable numbers are 46340 which only requires 166k claim blocks to create a 92k x 92k claim, 258015 for only ~20k claim blocks and 1047551 for only 6k. I haven't been able to process these larger numbers.
Running these commands can cause out of memory exceptions or if the server is able to process it, it will be unresponsive while doing so. /claim 46340 took about 60 seconds to process for my server.
I haven't been able to test this in a more real world scenario with claims to collide with and smaller world borders there's a good chance these stop the processing early.
Expected Behavior
No overflow, no processing crazy claim sizes for very few claim blocks
Reproduction steps
/claim 23170and then/claim 23171to see the required claimblocks reducing/claim 46340with 170k claim blocks, will either cause a heap exception or if the server can process it, it will result in a 92k x 92k claim for -166k claimblocks (note the minus)
Stack trace or error log
[21:50:49 INFO]: eepykanna issued server command: /claim 46340
[21:50:59 ERROR]: --- DO NOT REPORT THIS TO PAPER - THIS IS NOT A BUG OR A CRASH - 1.20.6-150-e61b73f (MC: 1.20.6) ---
[21:50:59 ERROR]: The server has not responded for 10 seconds! Creating thread dump
[21:50:59 ERROR]: ------------------------------
[21:50:59 ERROR]: Server thread dump (Look for plugins here before reporting to Paper!):
[21:50:59 ERROR]: ------------------------------
[21:50:59 ERROR]: Current Thread: Server thread
[21:50:59 ERROR]: PID: 62 | Suspended: false | Native: false | State: RUNNABLE
[21:50:59 ERROR]: Stack:
[21:50:59 ERROR]: [email protected]/java.util.concurrent.ConcurrentHashMap$TreeBin.balanceInsertion(Unknown Source)
[21:50:59 ERROR]: [email protected]/java.util.concurrent.ConcurrentHashMap$TreeBin.putTreeVal(Unknown Source)
[21:50:59 ERROR]: [email protected]/java.util.concurrent.ConcurrentHashMap.putVal(Unknown Source)
[21:50:59 ERROR]: [email protected]/java.util.concurrent.ConcurrentHashMap.put(Unknown Source)
[21:50:59 ERROR]: GriefPrevention.jar//me.ryanhamshire.GriefPrevention.DataStore.addToChunkClaimMap(DataStore.java:498)
[21:50:59 ERROR]: GriefPrevention.jar//me.ryanhamshire.GriefPrevention.DataStore.addClaim(DataStore.java:469)
[21:50:59 ERROR]: GriefPrevention.jar//me.ryanhamshire.GriefPrevention.DataStore.createClaim(DataStore.java:1013)
[21:50:59 ERROR]: GriefPrevention.jar//me.ryanhamshire.GriefPrevention.DataStore.createClaim(DataStore.java:863)
[21:50:59 ERROR]: GriefPrevention.jar//me.ryanhamshire.GriefPrevention.GriefPrevention.onCommand(GriefPrevention.java:1117)
[21:50:59 ERROR]: org.bukkit.command.PluginCommand.execute(PluginCommand.java:45)
[21:50:59 ERROR]: io.papermc.paper.command.brigadier.bukkit.BukkitCommandNode$BukkitBrigCommand.run(BukkitCommandNode.java:91)
[21:50:59 ERROR]: com.mojang.brigadier.context.ContextChain.runExecutable(ContextChain.java:73)
[21:50:59 ERROR]: net.minecraft.commands.execution.tasks.ExecuteCommand.execute(ExecuteCommand.java:31)
[21:50:59 ERROR]: net.minecraft.commands.execution.tasks.ExecuteCommand.execute(ExecuteCommand.java:19)
[21:50:59 ERROR]: net.minecraft.commands.execution.UnboundEntryAction.lambda$bind$0(UnboundEntryAction.java:8)
[21:50:59 ERROR]: net.minecraft.commands.execution.UnboundEntryAction$$Lambda/0x000073ffad3f23f8.execute(Unknown Source)
[21:50:59 ERROR]: net.minecraft.commands.execution.CommandQueueEntry.execute(CommandQueueEntry.java:5)
[21:50:59 ERROR]: net.minecraft.commands.execution.ExecutionContext.runCommandQueue(ExecutionContext.java:103)
[21:50:59 ERROR]: net.minecraft.commands.Commands.executeCommandInContext(Commands.java:448)
[21:50:59 ERROR]: net.minecraft.commands.Commands.performCommand(Commands.java:355)
[21:50:59 ERROR]: net.minecraft.commands.Commands.performCommand(Commands.java:342)
[21:50:59 ERROR]: net.minecraft.commands.Commands.performCommand(Commands.java:337)
[21:50:59 ERROR]: net.minecraft.server.network.ServerGamePacketListenerImpl.performUnsignedChatCommand(ServerGamePacketListenerImpl.java:2200)
[21:50:59 ERROR]: net.minecraft.server.network.ServerGamePacketListenerImpl.lambda$handleChatCommand$11(ServerGamePacketListenerImpl.java:2174)
[21:50:59 ERROR]: net.minecraft.server.network.ServerGamePacketListenerImpl$$Lambda/0x000073ffad3ef1f0.run(Unknown Source)
[21:50:59 ERROR]: net.minecraft.server.TickTask.run(TickTask.java:18)
[21:50:59 ERROR]: net.minecraft.util.thread.BlockableEventLoop.doRunTask(BlockableEventLoop.java:151)
[21:50:59 ERROR]: net.minecraft.util.thread.ReentrantBlockableEventLoop.doRunTask(ReentrantBlockableEventLoop.java:24)
[21:50:59 ERROR]: net.minecraft.server.MinecraftServer.doRunTask(MinecraftServer.java:1511)
[21:50:59 ERROR]: net.minecraft.server.MinecraftServer.doRunTask(MinecraftServer.java:195)
[21:50:59 ERROR]: net.minecraft.util.thread.BlockableEventLoop.pollTask(BlockableEventLoop.java:125)
[21:50:59 ERROR]: net.minecraft.server.MinecraftServer.pollTaskInternal(MinecraftServer.java:1488)
[21:50:59 ERROR]: net.minecraft.server.MinecraftServer.pollTask(MinecraftServer.java:1411)
[21:50:59 ERROR]: net.minecraft.util.thread.BlockableEventLoop.managedBlock(BlockableEventLoop.java:135)
[21:50:59 ERROR]: net.minecraft.server.MinecraftServer.waitUntilNextTick(MinecraftServer.java:1377)
[21:50:59 ERROR]: net.minecraft.server.MinecraftServer.runServer(MinecraftServer.java:1238)
[21:50:59 ERROR]: net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:323)
[21:50:59 ERROR]: net.minecraft.server.MinecraftServer$$Lambda/0x000073ffacbf8968.run(Unknown Source)
[21:50:59 ERROR]: [email protected]/java.lang.Thread.runWith(Unknown Source)
[21:50:59 ERROR]: [email protected]/java.lang.Thread.run(Unknown Source)Server version
> version
This server is running Paper version 1.20.6-150-ver/1.20.6@e61b73f (2024-09-16T19:18:19Z) (Implementing API version 1.20.6-R0.1-SNAPSHOT)
You are running the latest versionGriefPrevention version
> version GriefPrevention
GriefPrevention version 16.18.4Configuration
# Default values are perfect for most servers. If you want to customize and have a question, look for the answer here first: http://dev.bukkit.org/bukkit-plugins/grief-prevention/pages/setup-and-configuration/
GriefPrevention:
SeaLevelOverrides:
world: -1
world_nether: -1
world_the_end: -1
Claims:
Mode:
world_nether: Disabled
world: Survival
world_the_end: Disabled
PreventGlobalMonsterEggs: true
PreventTheft: true
ProtectCreatures: true
PreventButtonsSwitches: true
LockWoodenDoors: false
LockTrapDoors: false
LockFenceGates: true
EnderPearlsRequireAccessTrust: true
RaidTriggersRequireBuildTrust: true
ProtectHorses: true
ProtectDonkeys: true
ProtectLlamas: true
InitialBlocks: 100
Claim Blocks Accrued Per Hour:
Default: 100
Max Accrued Claim Blocks:
Default: 80000
Accrued Idle Threshold: 0
AccruedIdlePercent: 0
AbandonReturnRatio: 1.0
AutomaticNewPlayerClaimsRadius: 4
AutomaticNewPlayerClaimsRadiusMinimum: 0
ExtendIntoGroundDistance: 5
MinimumWidth: 5
MinimumArea: 100
MaximumDepth: -2147483648
InvestigationTool: STICK
ModificationTool: GOLDEN_SHOVEL
Expiration:
ChestClaimDays: 7
UnusedClaimDays: 14
AllClaims:
DaysInactive: 60
ExceptWhenOwnerHasTotalClaimBlocks: 10000
ExceptWhenOwnerHasBonusClaimBlocks: 5000
AutomaticNatureRestoration:
SurvivalWorlds: false
AllowTrappedInAdminClaims: false
MaximumNumberOfClaimsPerPlayer: 0
CreationRequiresWorldGuardBuildPermission: true
VillagerTradingRequiresPermission: true
CommandsRequiringAccessTrust: /sethome
DeliverManuals: true
ManualDeliveryDelaySeconds: 30
RavagersBreakBlocks: true
FireSpreadsInClaims: false
FireDamagesInClaims: false
LecternReadingRequiresAccessTrust: true
Spam:
Enabled: true
LoginCooldownSeconds: 60
LoginLogoutNotificationsPerMinute: 5
ChatSlashCommands: /me;/global;/local
WhisperSlashCommands: /tell;/pm;/r;/whisper;/msg
WarningMessage: Please reduce your noise level. Spammers will be banned.
BanOffenders: true
BanMessage: Banned for spam.
AllowedIpAddresses: 1.2.3.4; 5.6.7.8
DeathMessageCooldownSeconds: 120
Logout Message Delay In Seconds: 0
PvP:
RulesEnabledInWorld:
world: true
world_nether: true
world_the_end: true
ProtectFreshSpawns: true
PunishLogout: true
CombatTimeoutSeconds: 15
AllowCombatItemDrop: false
BlockedSlashCommands: /home;/vanish;/spawn;/tpa
ProtectPlayersInLandClaims:
PlayerOwnedClaims: true
AdministrativeClaims: true
AdministrativeSubdivisions: true
AllowLavaDumpingNearOtherPlayers:
PvPWorlds: true
NonPvPWorlds: false
AllowFlintAndSteelNearOtherPlayers:
PvPWorlds: true
NonPvPWorlds: false
ProtectPetsOutsideLandClaims: false
Economy:
ClaimBlocksMaxBonus: 0
ClaimBlocksPurchaseCost: 0.0
ClaimBlocksSellValue: 0.0
ProtectItemsDroppedOnDeath:
PvPWorlds: false
NonPvPWorlds: true
BlockLandClaimExplosions: true
BlockSurfaceCreeperExplosions: true
BlockSurfaceOtherExplosions: true
LimitSkyTrees: true
LimitTreeGrowth: false
PistonMovement: CLAIMS_ONLY
PistonExplosionSound: true
FireSpreads: false
FireDestroys: false
AdminsGetWhispers: true
AdminsGetSignNotifications: true
VisualizationAntiCheatCompatMode: false
SmartBan: true
Mute New Players Using Banned Words: true
MaxPlayersPerIpAddress: 3
SilenceBans: true
Siege:
Worlds: []
BreakableBlocks:
- PINK_WOOL
- LIGHT_GRAY_WOOL
- BLACK_WOOL
- PURPLE_WOOL
- LIME_WOOL
- MAGENTA_WOOL
- BLUE_WOOL
- GLASS
- SPRUCE_PLANKS
- DEAD_BUSH
- WHITE_WOOL
- GRAY_WOOL
- OAK_PLANKS
- GLASS_PANE
- CYAN_WOOL
- GREEN_WOOL
- FERN
- BIRCH_PLANKS
- RED_WOOL
- DARK_OAK_PLANKS
- SAND
- COBBLESTONE
- ACACIA_PLANKS
- SHORT_GRASS
- GRAVEL
- BROWN_WOOL
- SNOW
- YELLOW_WOOL
- LIGHT_BLUE_WOOL
- JUNGLE_PLANKS
- ORANGE_WOOL
- DIRT
- GRASS_BLOCK
DoorsOpenDelayInSeconds: 300
CooldownEndInMinutes: 60
EndermenMoveBlocks: false
SilverfishBreakBlocks: false
CreaturesTrampleCrops: false
RabbitsEatCrops: true
HardModeZombiesBreakDoors: false
MobProjectilesChangeBlocks: false
Database:
URL: ''
UserName: ''
Password: ''
UseBanCommand: false
BanCommandPattern: ban %name% %reason%
Advanced:
fixNegativeClaimblockAmounts: true
ClaimExpirationCheckRate: 60
OfflinePlayer_cache_days: 90
Abridged Logs:
Days To Keep: 7
Included Entry Types:
Social Activity: true
Suspicious Activity: true
Administrative Activity: false
Debug: false
Muted Chat Messages: false
ConfigVersion: 1Plugin list
> plugins
Server Plugins (1):
§xBukkit Plugins:
- GriefPreventionRunning without GriefPrevention
- I attempted running the server without GriefPrevention installed.
- The problem does not occur when GriefPrevention is removed from the server.
Running with only GriefPrevention
- I attempted running only GriefPrevention on the server.
- The issue still occurs when GriefPrevention is the only plugin running.
Running on a fresh, clean server installation
- I attempted testing for the issue on a new server.
- The issue still occurs on a new server.
Using unmodified client
- I attempted testing for the issue with the vanilla client.
- The issue still occurs when using the vanilla client.
We appreciate you taking the time to fill out a bug report!
- I searched for similar issues before submitting this bug report.
I don't disagree with the priority, it seems once another claim or the world border gets in the way it doesn't get to the addToChunkClaimMap function which is where the performance takes a big hit (which is what that PR seems to address).
we don't find it reasonable for any player to have the claimblocks to create such a large claim.
I do just want to re-iterate incase it was missed, you only need ~6000 claim blocks to run /claim 1047551 so you really don't need many claimblocks to try and run it.
I thought we had limited /claim and /extendclaim to 1000 blocks, but that code clearly isn't in place, so I'm wondering if that was a discussion we had somewhere and never implemented. Either way, an easy enough safeguard to put in place, though eventually we should do more to prevent edge cases causing values to wrap.
I think part of this is to be addressed with #2295 which will likely be addressed in v18 when we do some more major changes/removals. In general, the priority for this is low as we don't find it reasonable for any player to have the claimblocks to create such a large claim.
I thought we had limited /claim and /extendclaim to 1000 blocks
Never heard of this limitation ever at all.
I do just want to re-iterate incase it was missed, you only need ~6000 claim blocks to run /claim 1047551 so you really don't need many claimblocks to try and run it.
Ah, I was thinking then of issues with very skinny, long claims.
Never heard of this limitation ever at all.
Managed to find what I was thinking of - it was a single bullet point in a post I'd made after a couple of paragraphs about problems in this vein, only mentioned /extendclaim, and didn't specify an amount. Explains why it was in my head, it was one of my ideas but we ended up going a different route to handle that particular overflow issue.
Since it's come up again, how do we feel about a limit of 1000? Maybe only have it apply to people not in ignore claims mode, or maybe 5000/10000 would be more appropriate to keep supporting niche admin usage? It would be pretty simple to apply to legacy, and we can work on a more addon-interference-proof solution (like totaling claim amounts into a long and using safe math ops) for future versions.
I think 1000 is a pretty low limit. If i have a 100x100 claim, expanding 11 blocks already breaks the 1000 limit, unless we're referring to limiting the command input argument to1000 blocks, which seems ok to me. (Should this limit be a config option?)
As for admins who have access to /ignoreclaims, we could just have a confirmation message similarly to how there's a confirmation message when abandoning all claims
"Are you sure you want to expand 100000 blocks? Expanding such a high amount can cause errors that can lead to a server crash. If so, type /extendclaim 100000 confirm" (or something similar)
I think the limit is for the input argument afaik, yea.
I'm ok with a config option in legacy, in the Advanced section. But not so for master (or rather, as jikoo mentioned, a proper fix for master).
As for a warning message, if it's low maintenance sure, but given that a proper fix would be in master, I don't see a huge need for it.
I think 1000 is a pretty low limit. If i have a 100x100 claim, expanding 11 blocks already breaks the 1000 limit, unless we're referring to limiting the command input argument to1000 blocks, which seems ok to me. (Should this limit be a config option?)
Just a cap on the parameter which is block distance in one dimension, not on area. Personally no need for config as long as we leave it high enough, players are unlikely to have enough blocks to make expanding area at those rates a regular thing. /claim 1000 would create a 2000x2000 claim, which is 4m claim blocks.
I like the idea of a confirmation on bypass because GP really can't handle claims of that size well right now.