[SPIGOT-7637] Bad logic in checking nullability of AttributeModifier slots Created: 25/Apr/24  Updated: 25/Dec/24  Resolved: 25/Apr/24

Status: Resolved
Project: Spigot
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Minor
Reporter: William278 Assignee: Marvin Rieple
Resolution: Fixed Votes: 0
Labels: None
Environment:

Java 21


Version: 1.20.5-R0.1-SNAPSHOT
Guidelines Read: Yes

 Description   

AttributeModifier changed from storing a Nullable EquipmentSlot to a NotNull EquipmentSlotGroup at some point (I think with 1.20.5?), but there's a nullability flaw with the API implementation here.

  1. Instantiating AttributeModifier without an EquipmentSlotGroup or EquipmentSlot will invoke the fat constructor with `(EquipmentSlotGroup) null` - despite the EquipmentSlotGroup arg there being NotNull annotated [1]
  2. Since there's no Preconditions.checkArgument for passing null to this Notnull-annotated argument, the constructor works and instantiates an AttriuteModifier with slot as null
  3. This causes calls to the deprecated getSlot [2] method to throw NullPointerException, since that method tries to invoke getExample() on slot (which is null)

Suggested fix: Add a Preconditions check preventing slot from being passed as null and change the constructor call here to pass null to the other constructor which accepts an EquipmentSlot (which correctly converts EquipmentSlot to EquipmentSlotGroup in a ternary).

Thanks for your hard work.

  1. https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/browse/src/main/java/org/bukkit/attribute/AttributeModifier.java#30-32
  2. https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/browse/src/main/java/org/bukkit/attribute/AttributeModifier.java#97-99


 Comments   
Comment by Marvin Rieple [ 25/Apr/24 ]

Made a PR for this: bukkit#1000

You can test it with BuildTools:

java -jar BuildTools.jar --rev 4123 --compile spigot --pr bukkit:1000
Generated at Sun Mar 30 02:41:54 UTC 2025 using Jira 10.3.3#10030003-sha1:d220e3fefc8dfc6d47f522d3b9a20c1455e12b7b.