server/world/chunk: fix unbounded loop in SubChunk.Layer and layer renumbering in compact - #1422
Open
schphe wants to merge 2 commits into
Open
server/world/chunk: fix unbounded loop in SubChunk.Layer and layer renumbering in compact#1422schphe wants to merge 2 commits into
schphe wants to merge 2 commits into
Conversation
schphe
force-pushed
the
fix/chunk-layer-handling
branch
4 times, most recently
from
August 18, 2026 00:45
0c4212a to
9c34b17
Compare
…numbering in compact Neither of these is reachable from a packet, a world save or in-game play. Both are reachable from the package's own API, which is exported. SubChunk.Layer compared uint8(len(sub.storages)) against the layer index. At 256 storages that narrowing wraps to 0, which is <= every uint8, so the loop never terminated and appended a 4096-entry storage until the process ran out of memory. Layer(255) on a fresh sub chunk is enough; layer 254 terminates. Every SetBlock call in dragonfly passes a literal layer of 0 or 1, and the decode path never calls Layer at all, so only a caller outside the package can reach it. A sub chunk's storage count is encoded as a single byte in both the network and disk formats, which also caps a decoded sub chunk at 255 storages. A 256th storage cannot be represented at all: encoding one would write a count of 0 followed by 256 storages of payload. Layer now rejects layers at or above the new MaxLayers rather than building a sub chunk that cannot be written out. The three other places that narrowed a length to compare it against a layer index are compared as ints for the same reason. SubChunk.compact dropped every all-air storage and closed the gap, which moved each surviving layer down. Layer numbers are semantic: layer 0 is the block and layer 1 is Bedrock's waterlogging layer, so a sub chunk with air in layer 0 and water in layer 1 came out with the water in layer 0. Waterlogging always writes a block to layer 0, and removing that block promotes the liquid back to layer 0, so a chunk built through Tx never holds that state; it is reachable through SetOpts.DisableLiquidDisplacement and through a Structure that returns a liquid with no block. Only trailing all-air storages carry no information, so only those are dropped now.
schphe
force-pushed
the
fix/chunk-layer-handling
branch
from
August 20, 2026 07:51
9c34b17 to
52d4f72
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Neither of these is reachable from a packet, a world save or in-game play. Both are reachable from the package's own exported API. Filing them as hardening, not as live bugs.
The loop
SubChunk.Layercompareduint8(len(sub.storages))against the layer index. At 256 storages that narrowing wraps to 0, which is<=everyuint8, so the loop never terminated and appended a 4096-entry storage until the process ran out of memory.Layer(255)on a fresh sub chunk is enough; layer 254 terminates.Every
SetBlockcall in dragonfly passes a literal layer of 0 or 1, and the decode path never callsLayerat all, so only a caller outside the package can reach it.A sub chunk's storage count is encoded as a single byte in both the network and disk formats, which also caps a decoded sub chunk at 255 storages. A 256th cannot be represented at all, encoding one would write a count of 0 followed by 256 storages of payload.
Layernow rejects layers at or above the newMaxLayersrather than building a sub chunk that cannot be written out. The three other places that narrowed a length to compare against a layer index are compared as ints for the same reason.The renumbering
SubChunk.compactdropped every all-air storage and closed the gap, moving each surviving layer down. Layer numbers are semantic, layer 0 is the block, layer 1 is Bedrock's waterlogging layer, so a sub chunk with air in layer 0 and water in layer 1 came out with the water in layer 0.Waterlogging always writes a block to layer 0, and removing that block promotes the liquid back to layer 0, so a chunk built through
Txnever holds that state. It is reachable throughSetOpts.DisableLiquidDisplacementand through aStructurereturning a liquid with no block. Only trailing all-air storages carry no information, so only those are dropped now.Verification
Regression tests fail without the fix:
Layer(255)never returns within 5s, andCompact() block at layer 0 = 1, want 0 (air): it was renumbered down from layer 7. A randomised test over 30 seeds also asserts compaction changes no block at any layer, that one was written after a round-trip fuzz rediscovered the renumbering independently.