[MC-206338] Potential CME in skeleton trap code

All code references are using official mappings for 20w46a.

The skeleton trap code currently relies on the fact that

the goal used for the skeleton traps is always the last in the LinkedHashSet used for GoalSelector.availableGoals and
the (current) iterator implementation for LinkedHashSet only checks for concurrent modifications in next, but not in hasNext

GoalSelector.tick calls getRunningGoals().forEach(WrappedGoal::tick);. SkeletonTrapGoal.tick calls SkeletonHorse.setTrap, which directly modifies GoalSelector.availableGoals, the underlying set for the stream returned by getRunningGoals(). This is generally bad style, and in this case relies on undocumented behavior of LinkedHashSet iterators (the second point above): As the last element in the set is the one modifying the set, Iterator.next is not called after the modification, only Iterator.hasNext is. If any goals are added to the set after the trap goal, triggering the trap will crash the server (this happened on a modded server, that's how I found the issue). This never happens in vanilla, but I would still consider it a vanilla bug as it relies on undocumented behavior that could change in any Java update.

The cleanest fix for this would probably be to add a second boolean to SkeletonHorse indicating whether the horse should remain a trap, and to update the goals in SkeletonHorse.tick if this boolean does not match isTrap.

Comments 1

BugTracker_ 2026-03-11T12:34:25Z

This is still present in 1.21.11, GoalSelector.a(boolean) (goalTick) iterates this.c via for each and calls WrappedGoal.tick() ($$2.a()) on each running goal. SkeletonTrapGoal.tick() calls setTrap(false) (this.a.x(false)) mid iteration. setTrap(false) calls removeGoal() (this.cs.a(this.cv)), which calls removeIf directly on this.c, the set currently being iterated:

for (cqe $$2 : this.c) {
    if (!$$2.h() || !$$0 && !$$2.X_()) continue;
    $$2.a();
}

public void a() {
    axf $$0 = (axf)this.a.ao();
    cda $$1 = $$0.c(this.a.dK());
    this.a.x(false);
    ...
}

public void x(boolean $$0) {
    if ($$0 == this.cB) {
        return;
    }
    this.cB = $$0;
    if ($$0) {
        this.cs.a(1, this.cv);
    } else {
        this.cs.a(this.cv);
    }
}

public void a(cop $$0) {
    for (cqe $$12 : this.c) {
        if ($$12.k() != $$0 || !$$12.h()) continue;
        $$12.e();
    }
    this.c.removeIf($$1 -> $$1.k() == $$0);
}

private final Set<cqe> c = new ObjectLinkedOpenHashSet();

However, based on my analysis, it is worth noting that the structural difference from the original report: this.c is now ObjectLinkedOpenHashSet (fastutil) rather than java.util.LinkedHashSet. The original safe in vanilla assumption relied on LinkedHashSet's iterator only checking modCount in next(), not hasNext(). With ObjectLinkedOpenHashSet, modification during for each iteration produces undefined behavior rather than a guaranteed CME.