From 950f612c5e8f80ec71c5b23de0b8a1d2ace2257a Mon Sep 17 00:00:00 2001 From: schphe Date: Mon, 10 Aug 2026 06:48:35 -0500 Subject: [PATCH 1/2] server/world/chunk: fix unbounded loop in SubChunk.Layer and layer renumbering 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. --- server/world/chunk/chunk.go | 4 +- server/world/chunk/chunk_test.go | 132 +++++++++++++++++++++++++++ server/world/chunk/sub_chunk.go | 35 +++++-- server/world/chunk/sub_chunk_test.go | 66 ++++++++++++++ 4 files changed, 225 insertions(+), 12 deletions(-) create mode 100644 server/world/chunk/chunk_test.go create mode 100644 server/world/chunk/sub_chunk_test.go diff --git a/server/world/chunk/chunk.go b/server/world/chunk/chunk.go index ddee338b51..a03d93cff4 100644 --- a/server/world/chunk/chunk.go +++ b/server/world/chunk/chunk.go @@ -102,7 +102,7 @@ func (chunk *Chunk) Sub() []*SubChunk { // sub chunk exists at the given y, the block is assumed to be air. func (chunk *Chunk) Block(x uint8, y int16, z uint8, layer uint8) uint32 { sub := chunk.SubChunk(y) - if sub.Empty() || uint8(len(sub.storages)) <= layer { + if sub.Empty() || len(sub.storages) <= int(layer) { return chunk.air } return sub.storages[layer].At(x, uint8(y), z) @@ -112,7 +112,7 @@ func (chunk *Chunk) Block(x uint8, y int16, z uint8, layer uint8) uint32 { // SubChunk exists at the given y, a new SubChunk is created and the block is set. func (chunk *Chunk) SetBlock(x uint8, y int16, z uint8, layer uint8, block uint32) { sub := chunk.sub[chunk.SubIndex(y)] - if uint8(len(sub.storages)) <= layer && block == chunk.air { + if len(sub.storages) <= int(layer) && block == chunk.air { // Air was set at n layer, but there were less than n layers, so there already was air there. // Don't do anything with this, just return. return diff --git a/server/world/chunk/chunk_test.go b/server/world/chunk/chunk_test.go new file mode 100644 index 0000000000..ddd3bb3460 --- /dev/null +++ b/server/world/chunk/chunk_test.go @@ -0,0 +1,132 @@ +package chunk + +import ( + "math/rand" + "testing" + + "github.com/df-mc/dragonfly/server/block/cube" +) + +// TestCompactPreservesLayerNumbers verifies that Compact does not renumber layers. Layer 0 is the block and layer 1 is +// the waterlogging layer, so dropping an all-air layer 0 would turn waterlogging into a solid liquid block. +func TestCompactPreservesLayerNumbers(t *testing.T) { + tests := []struct { + name string + layer uint8 + }{ + {name: "waterlogging layer above an air block layer", layer: 1}, + {name: "layer above several air layers", layer: 7}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := New(testRegistry{}, cube.Range{0, 15}) + c.SetBlock(5, 0, 5, tt.layer, 1) + + c.Compact() + + if got := c.Block(5, 0, 5, tt.layer); got != 1 { + t.Errorf("Compact() block at layer %v = %v, want 1", tt.layer, got) + } + if got := c.Block(5, 0, 5, 0); got != 0 { + t.Errorf("Compact() block at layer 0 = %v, want 0 (air): it was renumbered down from layer %v", got, tt.layer) + } + }) + } +} + +// TestCompactDropsTrailingAirLayers verifies that Compact still drops trailing all-air storages. A layer past the last +// stored layer already reads as air, so those carry no information. +func TestCompactDropsTrailingAirLayers(t *testing.T) { + c := New(testRegistry{}, cube.Range{0, 15}) + c.SetBlock(5, 0, 5, 0, 1) + sub := c.sub[c.SubIndex(0)] + sub.storages = append(sub.storages, emptyStorage(0), emptyStorage(0)) + + c.Compact() + + if got := len(sub.storages); got != 1 { + t.Fatalf("Compact() left %v storages, want 1", got) + } + if got := c.Block(5, 0, 5, 0); got != 1 { + t.Fatalf("Block(layer 0) = %v, want 1", got) + } +} + +// TestEncodeStorageCountFitsInAByte verifies that a chunk grown to the highest addressable layer still encodes its +// storage count without overflowing the single byte it is written into. This is the limit MaxLayers exists to keep. +func TestEncodeStorageCountFitsInAByte(t *testing.T) { + c := New(testRegistry{}, cube.Range{0, 15}) + c.SetBlock(0, 0, 0, MaxLayers-1, 1) + + // Byte 0 of a sub chunk payload is the version, byte 1 is the storage count. + if got := Encode(c, NetworkEncoding).SubChunks[c.SubIndex(0)][1]; got != MaxLayers { + t.Fatalf("encoded storage count = %v, want %v", got, MaxLayers) + } +} + +// testRegistry is a minimal BlockRegistry. Runtime ID 0 is air and any other runtime ID is an opaque non-air block. +type testRegistry struct{} + +func (testRegistry) BlockCount() int { return 2 } + +func (testRegistry) AirRuntimeID() uint32 { return 0 } + +func (testRegistry) RuntimeIDToState(rid uint32) (string, map[string]any, bool) { + if rid == 0 { + return "minecraft:air", nil, true + } + return "minecraft:stone", nil, true +} + +func (testRegistry) StateToRuntimeID(name string, _ map[string]any) (uint32, bool) { + if name == "minecraft:air" { + return 0, true + } + return 1, true +} + +func (testRegistry) FilteringBlock(uint32) uint8 { return 0 } + +func (testRegistry) LightBlock(uint32) uint8 { return 0 } + +func (testRegistry) RandomTickBlock(uint32) bool { return false } + +func (testRegistry) NBTBlock(uint32) bool { return false } + +func (testRegistry) LiquidDisplacingBlock(uint32) bool { return false } + +func (testRegistry) LiquidBlock(uint32) bool { return false } + +func (testRegistry) HashToRuntimeID(hash uint32) (uint32, bool) { return hash, true } + +func (testRegistry) RuntimeIDToHash(rid uint32) (uint32, bool) { return rid, true } + +// TestCompactPreservesEveryBlock verifies over randomly populated chunks that compaction changes no block. Compaction +// used to drop all-air storages and close the gap, which moved every layer above the one dropped. +func TestCompactPreservesEveryBlock(t *testing.T) { + type pos struct { + x, z uint8 + y int16 + layer uint8 + } + for seed := int64(0); seed < 30; seed++ { + r := rand.New(rand.NewSource(seed)) + c := New(testRegistry{}, cube.Range{0, 255}) + + want := map[pos]uint32{} + for range 300 { + p := pos{uint8(r.Intn(16)), uint8(r.Intn(16)), int16(r.Intn(256)), uint8(r.Intn(3))} + rid := uint32(r.Intn(2)) + c.SetBlock(p.x, p.y, p.z, p.layer, rid) + want[p] = rid + } + + c.Compact() + + for p, rid := range want { + if got := c.Block(p.x, p.y, p.z, p.layer); got != rid { + t.Fatalf("seed %v: Compact() block at (%v,%v,%v) layer %v = %v, want %v", seed, p.x, p.y, p.z, p.layer, got, rid) + } + } + } +} diff --git a/server/world/chunk/sub_chunk.go b/server/world/chunk/sub_chunk.go index 7a06680deb..0cd9bc380c 100644 --- a/server/world/chunk/sub_chunk.go +++ b/server/world/chunk/sub_chunk.go @@ -1,6 +1,9 @@ package chunk -import "slices" +import ( + "fmt" + "slices" +) // SubChunk is a cube of blocks located in a chunk. It has a size of 16x16x16 blocks and forms part of a stack // that forms a Chunk. @@ -65,10 +68,20 @@ func (sub *SubChunk) Empty() bool { return len(sub.storages) == 0 || (len(sub.storages) == 1 && len(sub.storages[0].palette.values) == 1 && sub.storages[0].palette.values[0] == sub.air) } +// MaxLayers is the maximum number of block storages a SubChunk may hold. Both the network and the disk format encode +// the storage count of a sub chunk as a single byte, so a sub chunk holding more storages than this cannot be +// represented. The highest layer index that may be passed to Layer is therefore MaxLayers-1. +const MaxLayers = 255 + // Layer returns a certain block storage/layer from a sub chunk. If no storage at the layer exists, the layer -// is created, as well as all layers between the current highest layer and the new highest layer. +// is created, as well as all layers between the current highest layer and the new highest layer. Layer panics if the +// layer passed is MaxLayers, as such a layer cannot be encoded. func (sub *SubChunk) Layer(layer uint8) *PalettedStorage { - for uint8(len(sub.storages)) <= layer { + if int(layer) >= MaxLayers { + panic(fmt.Sprintf("layer %v is out of range: a sub chunk holds at most %v layers", layer, MaxLayers)) + } + // The length is compared as an int rather than narrowed to a uint8, which wraps to 0 once the maximum is reached. + for len(sub.storages) <= int(layer) { // Keep appending to storages until the requested layer is achieved. Makes working with new layers // much easier. sub.storages = append(sub.storages, emptyStorage(sub.air)) @@ -84,7 +97,7 @@ func (sub *SubChunk) Layers() []*PalettedStorage { // Block returns the runtime ID of the block located at the given X, Y and Z. X, Y and Z must be in a // range of 0-15. func (sub *SubChunk) Block(x, y, z byte, layer uint8) uint32 { - if uint8(len(sub.storages)) <= layer { + if len(sub.storages) <= int(layer) { return sub.air } return sub.storages[layer].At(x, y, z) @@ -136,14 +149,16 @@ func (sub *SubChunk) SkyLight(x, y, z byte) uint8 { // Compact cleans the garbage from all block storages that sub chunk contains, so that they may be // cleanly written to a database. func (sub *SubChunk) compact() { - newStorages := make([]*PalettedStorage, 0, len(sub.storages)) for _, storage := range sub.storages { storage.compact() - if len(storage.palette.values) == 1 && storage.palette.values[0] == sub.air { - // If the palette has only air in it, it means the storage is empty, so we can ignore it. - continue + } + // Only trailing all-air storages may be dropped, as a layer past the last stored layer already reads as air. + // Dropping an all-air storage below a populated one would renumber every layer above it instead. + for len(sub.storages) > 0 { + last := sub.storages[len(sub.storages)-1] + if len(last.palette.values) != 1 || last.palette.values[0] != sub.air { + break } - newStorages = append(newStorages, storage) + sub.storages = sub.storages[:len(sub.storages)-1] } - sub.storages = newStorages } diff --git a/server/world/chunk/sub_chunk_test.go b/server/world/chunk/sub_chunk_test.go new file mode 100644 index 0000000000..b91c01557a --- /dev/null +++ b/server/world/chunk/sub_chunk_test.go @@ -0,0 +1,66 @@ +package chunk + +import ( + "testing" + "time" +) + +// TestLayerAddressableRange verifies that Layer creates every layer up to the highest addressable index without +// looping, and that it refuses an index that cannot be encoded. +func TestLayerAddressableRange(t *testing.T) { + tests := []struct { + name string + layer uint8 + wantStorages int + wantPanic bool + }{ + {name: "block layer", layer: 0, wantStorages: 1}, + {name: "waterlogging layer", layer: 1, wantStorages: 2}, + {name: "highest addressable layer", layer: MaxLayers - 1, wantStorages: MaxLayers}, + {name: "layer past the encodable maximum", layer: MaxLayers, wantPanic: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sub, done, panicked := NewSubChunk(0), make(chan int, 1), make(chan struct{}, 1) + go func() { + defer func() { + if recover() != nil { + panicked <- struct{}{} + } + }() + sub.Layer(tt.layer) + done <- len(sub.storages) + }() + + select { + case got := <-done: + if tt.wantPanic { + t.Fatalf("Layer(%v) grew to %v storages, want a panic", tt.layer, got) + } + if got != tt.wantStorages { + t.Fatalf("Layer(%v) grew to %v storages, want %v", tt.layer, got, tt.wantStorages) + } + case <-panicked: + if !tt.wantPanic { + t.Fatalf("Layer(%v) panicked, want %v storages", tt.layer, tt.wantStorages) + } + case <-time.After(time.Second * 5): + t.Fatalf("Layer(%v) did not return: the layer index comparison wrapped and looped", tt.layer) + } + }) + } +} + +// TestBlockReadsEveryLayer verifies that Block reads back a block written to the highest addressable layer, and +// reports a layer that was never created as air. +func TestBlockReadsEveryLayer(t *testing.T) { + sub := NewSubChunk(0) + sub.SetBlock(1, 2, 3, MaxLayers-1, 1) + + if got := sub.Block(1, 2, 3, MaxLayers-1); got != 1 { + t.Fatalf("Block(layer %v) = %v, want 1", MaxLayers-1, got) + } + if got := sub.Block(1, 2, 3, MaxLayers-2); got != 0 { + t.Fatalf("Block(layer %v) = %v, want 0 (air)", MaxLayers-2, got) + } +} From 52d4f72df85ddb14f1f4ed8121b52f937f9ea5e3 Mon Sep 17 00:00:00 2001 From: schphe Date: Tue, 11 Aug 2026 15:07:21 -0500 Subject: [PATCH 2/2] Remove the tests added by this change --- server/world/chunk/chunk_test.go | 132 --------------------------- server/world/chunk/sub_chunk_test.go | 66 -------------- 2 files changed, 198 deletions(-) delete mode 100644 server/world/chunk/chunk_test.go delete mode 100644 server/world/chunk/sub_chunk_test.go diff --git a/server/world/chunk/chunk_test.go b/server/world/chunk/chunk_test.go deleted file mode 100644 index ddd3bb3460..0000000000 --- a/server/world/chunk/chunk_test.go +++ /dev/null @@ -1,132 +0,0 @@ -package chunk - -import ( - "math/rand" - "testing" - - "github.com/df-mc/dragonfly/server/block/cube" -) - -// TestCompactPreservesLayerNumbers verifies that Compact does not renumber layers. Layer 0 is the block and layer 1 is -// the waterlogging layer, so dropping an all-air layer 0 would turn waterlogging into a solid liquid block. -func TestCompactPreservesLayerNumbers(t *testing.T) { - tests := []struct { - name string - layer uint8 - }{ - {name: "waterlogging layer above an air block layer", layer: 1}, - {name: "layer above several air layers", layer: 7}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - c := New(testRegistry{}, cube.Range{0, 15}) - c.SetBlock(5, 0, 5, tt.layer, 1) - - c.Compact() - - if got := c.Block(5, 0, 5, tt.layer); got != 1 { - t.Errorf("Compact() block at layer %v = %v, want 1", tt.layer, got) - } - if got := c.Block(5, 0, 5, 0); got != 0 { - t.Errorf("Compact() block at layer 0 = %v, want 0 (air): it was renumbered down from layer %v", got, tt.layer) - } - }) - } -} - -// TestCompactDropsTrailingAirLayers verifies that Compact still drops trailing all-air storages. A layer past the last -// stored layer already reads as air, so those carry no information. -func TestCompactDropsTrailingAirLayers(t *testing.T) { - c := New(testRegistry{}, cube.Range{0, 15}) - c.SetBlock(5, 0, 5, 0, 1) - sub := c.sub[c.SubIndex(0)] - sub.storages = append(sub.storages, emptyStorage(0), emptyStorage(0)) - - c.Compact() - - if got := len(sub.storages); got != 1 { - t.Fatalf("Compact() left %v storages, want 1", got) - } - if got := c.Block(5, 0, 5, 0); got != 1 { - t.Fatalf("Block(layer 0) = %v, want 1", got) - } -} - -// TestEncodeStorageCountFitsInAByte verifies that a chunk grown to the highest addressable layer still encodes its -// storage count without overflowing the single byte it is written into. This is the limit MaxLayers exists to keep. -func TestEncodeStorageCountFitsInAByte(t *testing.T) { - c := New(testRegistry{}, cube.Range{0, 15}) - c.SetBlock(0, 0, 0, MaxLayers-1, 1) - - // Byte 0 of a sub chunk payload is the version, byte 1 is the storage count. - if got := Encode(c, NetworkEncoding).SubChunks[c.SubIndex(0)][1]; got != MaxLayers { - t.Fatalf("encoded storage count = %v, want %v", got, MaxLayers) - } -} - -// testRegistry is a minimal BlockRegistry. Runtime ID 0 is air and any other runtime ID is an opaque non-air block. -type testRegistry struct{} - -func (testRegistry) BlockCount() int { return 2 } - -func (testRegistry) AirRuntimeID() uint32 { return 0 } - -func (testRegistry) RuntimeIDToState(rid uint32) (string, map[string]any, bool) { - if rid == 0 { - return "minecraft:air", nil, true - } - return "minecraft:stone", nil, true -} - -func (testRegistry) StateToRuntimeID(name string, _ map[string]any) (uint32, bool) { - if name == "minecraft:air" { - return 0, true - } - return 1, true -} - -func (testRegistry) FilteringBlock(uint32) uint8 { return 0 } - -func (testRegistry) LightBlock(uint32) uint8 { return 0 } - -func (testRegistry) RandomTickBlock(uint32) bool { return false } - -func (testRegistry) NBTBlock(uint32) bool { return false } - -func (testRegistry) LiquidDisplacingBlock(uint32) bool { return false } - -func (testRegistry) LiquidBlock(uint32) bool { return false } - -func (testRegistry) HashToRuntimeID(hash uint32) (uint32, bool) { return hash, true } - -func (testRegistry) RuntimeIDToHash(rid uint32) (uint32, bool) { return rid, true } - -// TestCompactPreservesEveryBlock verifies over randomly populated chunks that compaction changes no block. Compaction -// used to drop all-air storages and close the gap, which moved every layer above the one dropped. -func TestCompactPreservesEveryBlock(t *testing.T) { - type pos struct { - x, z uint8 - y int16 - layer uint8 - } - for seed := int64(0); seed < 30; seed++ { - r := rand.New(rand.NewSource(seed)) - c := New(testRegistry{}, cube.Range{0, 255}) - - want := map[pos]uint32{} - for range 300 { - p := pos{uint8(r.Intn(16)), uint8(r.Intn(16)), int16(r.Intn(256)), uint8(r.Intn(3))} - rid := uint32(r.Intn(2)) - c.SetBlock(p.x, p.y, p.z, p.layer, rid) - want[p] = rid - } - - c.Compact() - - for p, rid := range want { - if got := c.Block(p.x, p.y, p.z, p.layer); got != rid { - t.Fatalf("seed %v: Compact() block at (%v,%v,%v) layer %v = %v, want %v", seed, p.x, p.y, p.z, p.layer, got, rid) - } - } - } -} diff --git a/server/world/chunk/sub_chunk_test.go b/server/world/chunk/sub_chunk_test.go deleted file mode 100644 index b91c01557a..0000000000 --- a/server/world/chunk/sub_chunk_test.go +++ /dev/null @@ -1,66 +0,0 @@ -package chunk - -import ( - "testing" - "time" -) - -// TestLayerAddressableRange verifies that Layer creates every layer up to the highest addressable index without -// looping, and that it refuses an index that cannot be encoded. -func TestLayerAddressableRange(t *testing.T) { - tests := []struct { - name string - layer uint8 - wantStorages int - wantPanic bool - }{ - {name: "block layer", layer: 0, wantStorages: 1}, - {name: "waterlogging layer", layer: 1, wantStorages: 2}, - {name: "highest addressable layer", layer: MaxLayers - 1, wantStorages: MaxLayers}, - {name: "layer past the encodable maximum", layer: MaxLayers, wantPanic: true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - sub, done, panicked := NewSubChunk(0), make(chan int, 1), make(chan struct{}, 1) - go func() { - defer func() { - if recover() != nil { - panicked <- struct{}{} - } - }() - sub.Layer(tt.layer) - done <- len(sub.storages) - }() - - select { - case got := <-done: - if tt.wantPanic { - t.Fatalf("Layer(%v) grew to %v storages, want a panic", tt.layer, got) - } - if got != tt.wantStorages { - t.Fatalf("Layer(%v) grew to %v storages, want %v", tt.layer, got, tt.wantStorages) - } - case <-panicked: - if !tt.wantPanic { - t.Fatalf("Layer(%v) panicked, want %v storages", tt.layer, tt.wantStorages) - } - case <-time.After(time.Second * 5): - t.Fatalf("Layer(%v) did not return: the layer index comparison wrapped and looped", tt.layer) - } - }) - } -} - -// TestBlockReadsEveryLayer verifies that Block reads back a block written to the highest addressable layer, and -// reports a layer that was never created as air. -func TestBlockReadsEveryLayer(t *testing.T) { - sub := NewSubChunk(0) - sub.SetBlock(1, 2, 3, MaxLayers-1, 1) - - if got := sub.Block(1, 2, 3, MaxLayers-1); got != 1 { - t.Fatalf("Block(layer %v) = %v, want 1", MaxLayers-1, got) - } - if got := sub.Block(1, 2, 3, MaxLayers-2); got != 0 { - t.Fatalf("Block(layer %v) = %v, want 0 (air)", MaxLayers-2, got) - } -}