The PaletteContainer#copy() method does not properly deep-copy, although it looks like it should. This currently does not have an impact on the game because all copies are only used for reading, but if that were to change, worlds could easily get corrupted.
When using this method, the internal storage is properly deep-copied. However, the palette copy (created by using Palette#copy()) keeps a reference to the original PaletteContainer's ResizeHandler. When triggering a resize by writing on a copy provided by PaletteContainer#copy(), the original container will be resized instead of the correct one. This can result in data corruption.
The problematic code can be seen below:
(#copy() calls Palette#copy() when creating a new Data object without providing a resizeHandler)
public PalettedContainer<T> copy() {
return new PalettedContainer<>(this.registry, this.strategy, new PalettedContainer.Data<>(this.data.configuration(), this.data.storage().copy(), this.data.palette().copy()));
}(all palette types share the resizeHandler when copying because they have no other one to provide)
# LinearPalette
@Override
public Palette<T> copy() {
return new LinearPalette<>(this.registry, (T[])((Object[])this.values.clone()), this.resizeHandler, this.bits, this.size);
}# HashMapPalette
@Override
public Palette<T> copy() {
return new HashMapPalette<>(this.registry, this.bits, this.resizeHandler, this.values.copy());
}# SingleValuePalette
@Override
public Palette<T> copy() {
if (this.value == null) {
throw new IllegalStateException("Use of an uninitialized palette");
} else {
return this;
}
}Comments 4
Yarn - 1.18.2
Can confirm, I was able to incorrectly change a block between 2 palettes. Although there is something missing about this bug report.
It's not possible to do this within the game. The palette#copy() is only used in one place.
RenderedChunk's Initializer. Which does not even use the listener nor update its data. The only concern would have been the copy resizing its data and corrupting the client-side palette although RenderedChunk does not modify the data, it only reads from it for rendering.
Still, if this copy was ever used without knowing that it could corrupt the palette, it would be a disaster. So here's how you fix it: I will be using ArrayPalette as my example
net.minecraft.world.chunk.ArrayPalette.java
// Original
public Palette<T> copy() {
return new ArrayPalette<>(this.idList, (T[])((Object[])this.array.clone()), this.listener, this.indexBits, this.size);
}
// New
// Use PaletteContainers's dummy Listener as the example for this
private final PaletteResizeListener<T> dummyListener = (newSize, added) -> 0;
public Palette<T> copy() {
return new ArrayPalette<>(this.idList, (T[])((Object[])this.array.clone()), dummyListener, this.indexBits, this.size);
}
public Palette<T> copy(PaletteResizeListener<T> listener) {
return new ArrayPalette<>(this.idList, (T[])((Object[])this.array.clone()), listener, this.indexBits, this.size);
}You modify the current copy method to use the dummyListener to ensure nothing actually gets modified, and make a new method which accepts a listener if you want to use one.
For flexibility you might also want to add that option within PaletteContainer
net.minecraft.world.chunk.PaletteContainer.java
// Original
public PalettedContainer<T> copy() {
return new PalettedContainer<>(
this.idList,
this.paletteProvider,
new PalettedContainer.Data<>(
this.data.configuration(),
this.data.storage().copy(),
this.data.palette().copy()
)
);
}
// New
public PalettedContainer<T> copy() {
return copy(dummyListener);
}
public PalettedContainer<T> copy(PaletteResizeListener<T> listener) {
return new PalettedContainer<>(
this.idList,
this.paletteProvider,
new PalettedContainer.Data<>(
this.data.configuration(),
this.data.storage().copy(),
this.data.palette().copy(listener)
)
);
}My main concern is modding, some mods might use the copy function in the Palettes without knowing about this issue, causing many problems.
Please attach the gameplay impact to the title or bug description. We do not take in bugs which are only code reviews.
Plausible based on provided code analysis.