Repository navigation
fix: prevent clipboard on disk file from growing on every load - #3720
Feniksovich wants to merge 5 commits into
Conversation
loadNBTFromFileFooter passed the total file length as the size argument
of FileChannel#map, mapping a region past the end of the file. As the
channel is writable, this silently extended the file by the size of the
block and biome data on every load. On close, writeNBTToDisk then
appended a new NBT footer after the existing one.
As a result, clipboards containing tile entities or entities grew on
every rejoin until they exceeded Integer.MAX_VALUE bytes and could no
longer be loaded ("Size exceeds Integer.MAX_VALUE").
Map only the footer when loading and overwrite the existing footer when
saving.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dordsor21
left a comment
dordsor21
left a comment
There was a problem hiding this comment.
Potentially worth not writing out the clipboard file again if we've not changed I guess as well
|
I've made close() skip rewriting the NBT footer if nothing changed since the clipboard was loaded. I'm not sure what the best way to do that check is tbh. Right now |
|
@Feniksovich Is this ready to be reviewed again? Just checking if everything you need to do is finished. |
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🟡 Changes recommended
A moderate active issue remains in DiskOptimizedClipboard.java where memory-mapping uses potentially overflowing int-based offset math and an unchecked non-positive mapped length, which can trigger IllegalArgumentException when loading large or truncated clipboards.
1 open issue
Review effort: Balanced
| int biomeBlocksLength = headerSize + (getVolume() << 1) + (hasBiomes ? (int) biomeLength : 0); | ||
| MappedByteBuffer tmp = fileChannel.map(FileChannel.MapMode.READ_ONLY, biomeBlocksLength, braf.length()); | ||
| // FileChannel#map takes a size, not an end position. Mapping past the end of the file would silently grow it. | ||
| MappedByteBuffer tmp = fileChannel.map( | ||
| FileChannel.MapMode.READ_ONLY, | ||
| biomeBlocksLength, |
@MattBDev Yes I think so, should we add a guard precondition as Copilot suggested? |
Overview
Fixes #3719
A player's clipboard file on disk (
clipboard/<UUID>.bd) grew every time the clipboard was loaded and closed, e.g. on every rejoin, if the clipboard contained tile entities or entities. After enough rejoins the file exceeded 2 GB (Integer.MAX_VALUE). From then on the clipboard could no longer be loaded, and every join logged:Error loading clipboard from disk
java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Size exceeds Integer.MAX_VALUE
Description
Actual behavior before fixes:
loadNBTFromFileFooterpassed the total file length as the size argument ofFileChannel#map, instead of the footer length. The mapped region ran past the end of the file. Because the channel is writable, the JDK silently extended the file by the size of the block and biome data (this happens even withMapMode.READ_ONLY).writeNBTToDiskalways appended the footer at the current end of the file. It never replaced the existing footer, and since the loader always reads the first footer, every footer appended after it was dead data.This PR:
braf.length() - biomeBlocksLength) when loading;Submitter Checklist
@since TODO.