-
-
Notifications
You must be signed in to change notification settings - Fork 879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix int overflow when using fillr in negative y-coordinates #2082
Fix int overflow when using fillr in negative y-coordinates #2082
Conversation
dordsor21
commented
Apr 20, 2022
•
edited
Loading
edited
- Also actually check the depth arg against the radius limit
- Fixes //fillr with negativ y coordinates #973
- Also actually check the depth arg against the radius limit
If checking the depth size against max allowed radius should be done for fillr then it should also be done for fill, otherwise, the checkmaxradius in fillr should be removed, and the old default behaviour of "infinite depth" be re-added if no depth parameter is given |
I don't understand how this fixes a negative y-coord issue. It seems like you only changed the value used if |
The depth is subtracted from the height of the player in the fillXZ method in editsession https://github.com/EngineHub/WorldEdit/blob/master/worldedit-core/src/main/java/com/sk89q/worldedit/EditSession.java#L1007. Using Integer.MAX_VALUE causes an overflow with negative values, so changing the default depth fixes it. It makes sense to also ensure that the depth does not exceed the max radius set, as it is effectively a radius in itself |
Sounds like something that should be fixed in the method itself, because it's API -- people can still call it with |
Separate to that (which would just be a bound to Integer.MIN_VALUE I suppose), should the depth be able to be larger than the max radius configured? There are also other places overflows can occur, e.g. https://github.com/EngineHub/WorldEdit/blob/master/worldedit-core/src/main/java/com/sk89q/worldedit/EditSession.java#L1682 |
@@ -101,6 +101,7 @@ public int fill(Actor actor, LocalSession session, EditSession editSession, | |||
radius = Math.max(1, radius); | |||
we.checkMaxRadius(radius); | |||
depth = Math.max(1, depth); | |||
we.checkMaxRadius(depth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So from looking at this, it doesn't actually appear that any of the changes in UtilityCommands.java are actually required here. And these do modify the behaviour of the commands.
Can this PR please be modified to only include the changes within EditSession.java?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All done!