fix: Paper 26.2 compatibility for SpigotEntityIdProvider#70
Conversation
Three fixes for the entity ID provider used on Paper 26.2+: 1. Add reflection guard for UnsafeValues.nextEntityId() - Paper 26.2+ removed this method; verify via reflection before calling - Falls through to AtomicInteger reflection path when absent 2. Fix ArrayIndexOutOfBoundsException in getEntityClass() - Paper 26.2+ has no version suffix in the CraftServer package name - Skip package name parsing entirely for 1.17+ (flattened) servers 3. Fix IllegalAccessException on legacy entity counter - Skip static final fields (e.g. CURRENT_LEVEL) - Add findMutableStaticIntField helper with isFinal check - Fall back to local AtomicInteger when no mutable field is found - Also search for ENTITY_COUNTER in AtomicInteger field names
📝 WalkthroughWalkthrough
ChangesSpigotEntityIdProvider Resilience
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java`:
- Around line 110-113: The fallback counter in the `SpigotEntityIdProvider`
class is seeded at `Integer.MAX_VALUE - 100000`, causing it to overflow into
negative values after approximately 100k entity creations. Replace the seed
value in the `AtomicInteger` initialization to use a lower value that maintains
sufficient distance from the server's normal allocation range (which starts at
1) while leaving adequate headroom before `Integer.MAX_VALUE` to prevent
overflow, such as a value in the range of 1,000,000 or higher depending on
expected entity count limits, or alternatively implement bounds checking or
wrapping logic to handle the overflow gracefully.
- Around line 142-150: The wildcard fallback logic in the field iteration for
non-final static int fields (lines 142-150) blindly returns the first matching
field without validation, risking mutation of unrelated NMS static integers when
resolveLegacySupplier calls setInt on this field. Instead of returning the first
match found by getDeclaredFields(), add validation to ensure you are selecting
the correct field: either collect all matching candidates and verify exactly one
exists before returning it, or add a warning log if the fallback is being used,
or implement additional checks to distinguish legitimate entity counter fields
from unrelated static ints. This prevents silent corruption of server state from
mutating the wrong field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92fe61a6-c265-47d5-b654-c3e8a5b4b550
📒 Files selected for processing (1)
platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java
| // Last resort: local high-offset counter. Entity ID collision is unlikely | ||
| // since the server allocates from 1 upward. | ||
| final AtomicInteger fallback = new AtomicInteger(Integer.MAX_VALUE - 100000); | ||
| return fallback::incrementAndGet; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fallback counter overflows into negative IDs after ~100k entities.
Seeding at Integer.MAX_VALUE - 100000 means incrementAndGet reaches Integer.MAX_VALUE after ~100k calls and then wraps to Integer.MIN_VALUE, producing negative entity IDs that can collide or misbehave on clients. Consider seeding lower (well above the server's allocation range but far from overflow), or documenting/bounding the wrap behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java`
around lines 110 - 113, The fallback counter in the `SpigotEntityIdProvider`
class is seeded at `Integer.MAX_VALUE - 100000`, causing it to overflow into
negative values after approximately 100k entity creations. Replace the seed
value in the `AtomicInteger` initialization to use a lower value that maintains
sufficient distance from the server's normal allocation range (which starts at
1) while leaving adequate headroom before `Integer.MAX_VALUE` to prevent
overflow, such as a value in the range of 1,000,000 or higher depending on
expected entity count limits, or alternatively implement bounds checking or
wrapping logic to handle the overflow gracefully.
| // Wildcard fallback: any non-final static int field | ||
| for (final Field field : clazz.getDeclaredFields()) { | ||
| if (Modifier.isStatic(field.getModifiers()) | ||
| && field.getType() == Integer.TYPE | ||
| && !Modifier.isFinal(field.getModifiers())) { | ||
| return field; | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the resolved legacy field is mutated
rg -nP -C3 '\bsetInt\s*\(' platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.javaRepository: Tofaa2/EntityLib
Length of output: 552
🏁 Script executed:
#!/bin/bash
# Get the full context of findMutableStaticIntField method
sed -n '100,160p' platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.javaRepository: Tofaa2/EntityLib
Length of output: 2937
🏁 Script executed:
#!/bin/bash
# Check if there are name-based filters or constraints in the field resolution
rg -nP -B10 -A10 'findMutableStaticIntField' platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java | head -60Repository: Tofaa2/EntityLib
Length of output: 2275
🏁 Script executed:
#!/bin/bash
# Search for any additional validation or filtering of the resolved field
rg -nP 'entityLegacyField|resolveLegacySupplier' platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java | head -30Repository: Tofaa2/EntityLib
Length of output: 620
Wildcard fallback selects the first non-final static int field, risking mutation of an unrelated field.
The method first attempts known field names ("entityCount", "b", "c") matching Minecraft entity counters, but if those fail, the wildcard fallback at lines 142–150 iterates getDeclaredFields() and returns the first non-final static int without name-based validation. Since getDeclaredFields() field ordering is not guaranteed across JVM implementations and obfuscation tools, this selection is non-deterministic. resolveLegacySupplier then calls setInt(null, entityId + 1) on this field (line 119), so selecting the wrong field mutates an unrelated NMS static int, corrupting server state.
While a fallback AtomicInteger exists if no field is found, the wildcard fallback should be restricted (e.g., require exactly one candidate, or log a warning) rather than blindly mutating any match.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java`
around lines 142 - 150, The wildcard fallback logic in the field iteration for
non-final static int fields (lines 142-150) blindly returns the first matching
field without validation, risking mutation of unrelated NMS static integers when
resolveLegacySupplier calls setInt on this field. Instead of returning the first
match found by getDeclaredFields(), add validation to ensure you are selecting
the correct field: either collect all matching candidates and verify exactly one
exists before returning it, or add a warning log if the fallback is being used,
or implement additional checks to distinguish legitimate entity counter fields
from unrelated static ints. This prevents silent corruption of server state from
mutating the wrong field.
|
Overall I think this class should be removed and spigot should just use SpigotReflectionUtil#813 and SpigotReflectionUtil#826 depending on whether the version is 26.2+ |
2 participants
Three fixes for the entity ID provider used on Paper 26.2+:
Add reflection guard for UnsafeValues.nextEntityId()
Fix ArrayIndexOutOfBoundsException in getEntityClass()
Fix IllegalAccessException on legacy entity counter
Summary by CodeRabbit