From 00d4a206f75a6ae5b51bf60e8bcadb2b33662c6d Mon Sep 17 00:00:00 2001 From: Julien Laurenceau Date: Mon, 12 Oct 2026 17:02:02 +0200 Subject: [PATCH 1/4] fix: zero EcM when unmarshaling legacy xl.meta V2 headers The legacy header decode paths zeroed EcN twice (z.EcN, z.EcN = 0, 0) instead of EcN and EcM, so EcM retained its previous value when the same structure was reused across decodes. Decoding an xl.meta written by an older release would then leak the erasure metadata (data/parity block counts) of a previously decoded object into it, potentially corrupting quorum and healing decisions. Also zero the fields on the V1 header path, which had no reset at all. Signed-off-by: Julien Laurenceau --- cmd/xl-storage-format-v2-legacy.go | 5 ++- cmd/xl-storage-format-v2_test.go | 66 ++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/cmd/xl-storage-format-v2-legacy.go b/cmd/xl-storage-format-v2-legacy.go index 9d682487b..188b299d3 100644 --- a/cmd/xl-storage-format-v2-legacy.go +++ b/cmd/xl-storage-format-v2-legacy.go @@ -40,6 +40,7 @@ func (x *xlMetaV2VersionHeader) unmarshalV(v uint8, bts []byte) (o []byte, err e // unmarshalV1 decodes version 1, never released. func (x *xlMetaV2VersionHeader) unmarshalV1(bts []byte) (o []byte, err error) { + x.EcN, x.EcM = 0, 0 var zb0001 uint32 zb0001, bts, err = msgp.ReadArrayHeaderBytes(bts) if err != nil { @@ -134,7 +135,7 @@ type xlMetaV2VersionHeaderV2 struct { // UnmarshalMsg implements msgp.Unmarshaler func (z *xlMetaV2VersionHeaderV2) UnmarshalMsg(bts []byte) (o []byte, err error) { - z.EcN, z.EcN = 0, 0 + z.EcN, z.EcM = 0, 0 var zb0001 uint32 zb0001, bts, err = msgp.ReadArrayHeaderBytes(bts) if err != nil { @@ -184,7 +185,7 @@ func (z *xlMetaV2VersionHeaderV2) UnmarshalMsg(bts []byte) (o []byte, err error) // DecodeMsg implements msgp.Decodable func (z *xlMetaV2VersionHeaderV2) DecodeMsg(dc *msgp.Reader) (err error) { - z.EcN, z.EcN = 0, 0 + z.EcN, z.EcM = 0, 0 var zb0001 uint32 zb0001, err = dc.ReadArrayHeader() if err != nil { diff --git a/cmd/xl-storage-format-v2_test.go b/cmd/xl-storage-format-v2_test.go index 6a9b15a3f..21e76eb76 100644 --- a/cmd/xl-storage-format-v2_test.go +++ b/cmd/xl-storage-format-v2_test.go @@ -38,6 +38,7 @@ import ( "github.com/klauspost/compress/zstd" "github.com/minio/minio/internal/bucket/lifecycle" xhttp "github.com/minio/minio/internal/http" + "github.com/tinylib/msgp/msgp" ) func TestReadXLMetaNoData(t *testing.T) { @@ -1190,3 +1191,68 @@ func BenchmarkToFileInfoNoParts(b *testing.B) { func BenchmarkToFileInfoWithParts(b *testing.B) { benchmarkManyPartsOptionally(b, true) } + +// TestXLMetaV2HeaderLegacyUnmarshalResetsEc checks that decoding legacy +// version headers, which carry no erasure metadata fields, cannot leak +// stale EcN/EcM values from a previously decoded object into the same +// structure. +func TestXLMetaV2HeaderLegacyUnmarshalResetsEc(t *testing.T) { + base := xlMetaV2VersionHeader{ + VersionID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, + ModTime: time.Now().Unix(), + Signature: [4]byte{1, 2, 3, 4}, + Type: ObjectType, + Flags: xlFlagInlineData, + } + + // Legacy V2 header wire format: 5 fields, no EcN/EcM. + var legacy []byte + legacy = msgp.AppendArrayHeader(legacy, 5) + legacy = msgp.AppendBytes(legacy, base.VersionID[:]) + legacy = msgp.AppendInt64(legacy, base.ModTime) + legacy = msgp.AppendBytes(legacy, base.Signature[:]) + legacy = msgp.AppendUint8(legacy, uint8(base.Type)) + legacy = msgp.AppendUint8(legacy, uint8(base.Flags)) + + header := base + header.EcN, header.EcM = 4, 8 + x2 := xlMetaV2VersionHeaderV2{xlMetaV2VersionHeader: &header} + if _, err := x2.UnmarshalMsg(legacy); err != nil { + t.Fatalf("legacy V2 header unmarshal failed: %v", err) + } + if header.EcN != 0 || header.EcM != 0 { + t.Fatalf("stale erasure metadata leaked across objects: EcN %d, EcM %d", header.EcN, header.EcM) + } + + // V1 headers (4 fields, never released) must zero the fields too. + var v1 []byte + v1 = msgp.AppendArrayHeader(v1, 4) + v1 = msgp.AppendBytes(v1, base.VersionID[:]) + v1 = msgp.AppendInt64(v1, base.ModTime) + v1 = msgp.AppendUint8(v1, uint8(base.Type)) + v1 = msgp.AppendUint8(v1, uint8(base.Flags)) + + header = base + header.EcN, header.EcM = 4, 8 + if _, err := header.unmarshalV1(v1); err != nil { + t.Fatalf("legacy V1 header unmarshal failed: %v", err) + } + if header.EcN != 0 || header.EcM != 0 { + t.Fatalf("stale erasure metadata leaked on V1 decode: EcN %d, EcM %d", header.EcN, header.EcM) + } + + // Current headers must keep their erasure metadata. + current := base + current.EcN, current.EcM = 4, 8 + wire, err := current.MarshalMsg(nil) + if err != nil { + t.Fatalf("header marshal failed: %v", err) + } + decoded := base + if _, err := decoded.UnmarshalMsg(wire); err != nil { + t.Fatalf("header unmarshal failed: %v", err) + } + if decoded.EcN != 4 || decoded.EcM != 8 { + t.Fatalf("erasure metadata lost on current decode: EcN %d, EcM %d", decoded.EcN, decoded.EcM) + } +} From a54d0c43545c5707e3dd09d9b10c4614511a423a Mon Sep 17 00:00:00 2001 From: Julien Laurenceau Date: Mon, 12 Oct 2026 17:02:02 +0200 Subject: [PATCH 2/4] fix: reset pooled NSScanner RPC responses to avoid stale usage data NSScanner responses stream either a dataUsageEntry update or the final dataUsageCache. The grid framework recycles stream responses into a pool without resetting them, and the handler set only one field per response - so a recycled response still carried the Final (or Update) payload of an earlier scan. A remote node reading such a response accepted the stale final cache, corrupting bucket usage data across scanner cycles. Honor Recycler types when pooling stream responses, implement it for nsScannerResp, and clear the unset field at the response construction sites for defense in depth. Signed-off-by: Julien Laurenceau --- cmd/storage-rest-common.go | 10 +++++++++ cmd/storage-rest-server.go | 4 ++++ cmd/storage-rest-server_test.go | 36 +++++++++++++++++++++++++++++++++ internal/grid/handlers.go | 5 +++++ 4 files changed, 55 insertions(+) create mode 100644 cmd/storage-rest-server_test.go diff --git a/cmd/storage-rest-common.go b/cmd/storage-rest-common.go index d8a28ae29..17bcc5f69 100644 --- a/cmd/storage-rest-common.go +++ b/cmd/storage-rest-common.go @@ -83,3 +83,13 @@ type nsScannerResp struct { Update *dataUsageEntry `msg:"u"` Final *dataUsageCache `msg:"f"` } + +// Recycle resets the response, so that recycled responses cannot +// leak stale scanner data into a subsequent scan. +func (r *nsScannerResp) Recycle() { + if r == nil { + return + } + r.Update = nil + r.Final = nil +} diff --git a/cmd/storage-rest-server.go b/cmd/storage-rest-server.go index f20e4e99f..17d274bca 100644 --- a/cmd/storage-rest-server.go +++ b/cmd/storage-rest-server.go @@ -290,6 +290,9 @@ func (s *storageRESTServer) NSScannerHandler(ctx context.Context, params *nsScan for update := range updates { resp := storageNSScannerRPC.NewResponse() resp.Update = &update + // Pooled responses may carry fields from a previous + // scan, always clear the fields not being set. + resp.Final = nil out <- resp } }() @@ -301,6 +304,7 @@ func (s *storageRESTServer) NSScannerHandler(ctx context.Context, params *nsScan // Send final response. resp := storageNSScannerRPC.NewResponse() resp.Final = &ui + resp.Update = nil out <- resp return nil } diff --git a/cmd/storage-rest-server_test.go b/cmd/storage-rest-server_test.go new file mode 100644 index 000000000..354348882 --- /dev/null +++ b/cmd/storage-rest-server_test.go @@ -0,0 +1,36 @@ +// Copyright (c) 2015-2023 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import "testing" + +// TestNSScannerRespPooling verifies that recycled NSScanner stream +// responses cannot leak stale fields from a previous scan into new +// responses, which would otherwise produce incorrect bucket usage +// data. +func TestNSScannerRespPooling(t *testing.T) { + dirty := storageNSScannerRPC.NewResponse() + dirty.Update = &dataUsageEntry{} + dirty.Final = &dataUsageCache{} + storageNSScannerRPC.PutResponse(dirty) + + resp := storageNSScannerRPC.NewResponse() + if resp.Update != nil || resp.Final != nil { + t.Fatalf("pooled nsScannerResp leaked stale fields: Update %v, Final %v", resp.Update, resp.Final) + } +} diff --git a/internal/grid/handlers.go b/internal/grid/handlers.go index 1d20fa9b9..b0ca9e324 100644 --- a/internal/grid/handlers.go +++ b/internal/grid/handlers.go @@ -698,6 +698,11 @@ func (h *StreamTypeHandler[Payload, Req, Resp]) PutRequest(r Req) { // These should be returned by the caller. func (h *StreamTypeHandler[Payload, Req, Resp]) PutResponse(r Resp) { if r != h.nilResp { + if rc, ok := any(r).(Recycler); ok { + // Recycled stream responses must not leak fields + // from a previous use into the next response. + rc.Recycle() + } //nolint:staticcheck // SA6002 IT IS A GENERIC VALUE! (and always a pointer) h.respPool.Put(r) } From ba6e8c8d6912ba3d7f8a147e1b0710e1d1c26344 Mon Sep 17 00:00:00 2001 From: Julien Laurenceau Date: Mon, 12 Oct 2026 17:12:49 +0200 Subject: [PATCH 3/4] fix: reset Signature when unmarshaling legacy V1 xl.meta headers The V1 header wire format has no Signature field, so decoding into a reused header leaked the previous object's signature. Reset it along with EcN/EcM so a decode fully replaces the header contents. Signed-off-by: Julien Laurenceau --- cmd/xl-storage-format-v2-legacy.go | 3 +++ cmd/xl-storage-format-v2_test.go | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/cmd/xl-storage-format-v2-legacy.go b/cmd/xl-storage-format-v2-legacy.go index 188b299d3..06201693b 100644 --- a/cmd/xl-storage-format-v2-legacy.go +++ b/cmd/xl-storage-format-v2-legacy.go @@ -40,6 +40,9 @@ func (x *xlMetaV2VersionHeader) unmarshalV(v uint8, bts []byte) (o []byte, err e // unmarshalV1 decodes version 1, never released. func (x *xlMetaV2VersionHeader) unmarshalV1(bts []byte) (o []byte, err error) { + // Fields absent from the V1 wire format must be reset since the + // header may be reused across decodes. + x.Signature = [4]byte{} x.EcN, x.EcM = 0, 0 var zb0001 uint32 zb0001, bts, err = msgp.ReadArrayHeaderBytes(bts) diff --git a/cmd/xl-storage-format-v2_test.go b/cmd/xl-storage-format-v2_test.go index 21e76eb76..0076c8046 100644 --- a/cmd/xl-storage-format-v2_test.go +++ b/cmd/xl-storage-format-v2_test.go @@ -1234,12 +1234,16 @@ func TestXLMetaV2HeaderLegacyUnmarshalResetsEc(t *testing.T) { header = base header.EcN, header.EcM = 4, 8 + header.Signature = [4]byte{9, 9, 9, 9} if _, err := header.unmarshalV1(v1); err != nil { t.Fatalf("legacy V1 header unmarshal failed: %v", err) } if header.EcN != 0 || header.EcM != 0 { t.Fatalf("stale erasure metadata leaked on V1 decode: EcN %d, EcM %d", header.EcN, header.EcM) } + if header.Signature != ([4]byte{}) { + t.Fatalf("stale signature leaked on V1 decode: %v", header.Signature) + } // Current headers must keep their erasure metadata. current := base From 86c5a92eef3d5667c83bdbc59a033b955bdbf146 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 15 Oct 2026 09:48:16 +0800 Subject: [PATCH 4/4] fix: keep scanner response resets local and strengthen regression tests Keep the legacy header resets and clear the unused NSScanner response field at each construction site. Remove the generic stream Recycler call: Recycler transfers resource ownership and cannot be treated as a reset hook followed by another insertion into the handler's pool. Exercise the actual NSScanner handler across repeated scans with dirty response allocations, so the regression test does not depend on sync.Pool reusing an object. Check both response variants after wire serialization. Cover legacy DecodeMsg and reused indexed metadata headers as well. Retain the original contributor's commits and DCO sign-offs. Signed-off-by: Feng Ruohang --- cmd/storage-rest-common.go | 10 ---- cmd/storage-rest-server_test.go | 88 ++++++++++++++++++++++++++------ cmd/xl-storage-format-v2_test.go | 38 ++++++++++---- internal/grid/handlers.go | 5 -- 4 files changed, 102 insertions(+), 39 deletions(-) diff --git a/cmd/storage-rest-common.go b/cmd/storage-rest-common.go index 17bcc5f69..d8a28ae29 100644 --- a/cmd/storage-rest-common.go +++ b/cmd/storage-rest-common.go @@ -83,13 +83,3 @@ type nsScannerResp struct { Update *dataUsageEntry `msg:"u"` Final *dataUsageCache `msg:"f"` } - -// Recycle resets the response, so that recycled responses cannot -// leak stale scanner data into a subsequent scan. -func (r *nsScannerResp) Recycle() { - if r == nil { - return - } - r.Update = nil - r.Final = nil -} diff --git a/cmd/storage-rest-server_test.go b/cmd/storage-rest-server_test.go index 354348882..93ce35803 100644 --- a/cmd/storage-rest-server_test.go +++ b/cmd/storage-rest-server_test.go @@ -1,6 +1,6 @@ -// Copyright (c) 2015-2023 MinIO, Inc. +// Copyright (c) 2026 Julien Laurenceau and Feng Ruohang // -// This file is part of MinIO Object Storage stack +// This file is part of Silo Object Storage. // // This program is free software: you can redistribute it and/or modify // it under the terms of the GNU Affero General Public License as published by @@ -17,20 +17,78 @@ package cmd -import "testing" +import ( + "context" + "testing" -// TestNSScannerRespPooling verifies that recycled NSScanner stream -// responses cannot leak stale fields from a previous scan into new -// responses, which would otherwise produce incorrect bucket usage -// data. -func TestNSScannerRespPooling(t *testing.T) { - dirty := storageNSScannerRPC.NewResponse() - dirty.Update = &dataUsageEntry{} - dirty.Final = &dataUsageCache{} - storageNSScannerRPC.PutResponse(dirty) + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/grid" +) - resp := storageNSScannerRPC.NewResponse() - if resp.Update != nil || resp.Final != nil { - t.Fatalf("pooled nsScannerResp leaked stale fields: Update %v, Final %v", resp.Update, resp.Final) +type scannerResponseTestDisk struct { + StorageAPI +} + +func (scannerResponseTestDisk) NSScanner(_ context.Context, cache dataUsageCache, updates chan<- dataUsageEntry, _ madmin.HealScanMode, _ func() bool) (dataUsageCache, error) { + defer close(updates) + updates <- dataUsageEntry{Size: int64(cache.Info.NextCycle)} + cache.Info.NextCycle++ + return cache, nil +} + +func TestNSScannerHandlerClearsStaleResponses(t *testing.T) { + endpoint, err := NewEndpoint(t.TempDir()) + if err != nil { + t.Fatal(err) + } + globalLocalDrivesMu.Lock() + oldDrives, oldSets := globalLocalDrivesMap, globalLocalSetDrives + globalLocalDrivesMap = map[string]StorageAPI{endpoint.String(): scannerResponseTestDisk{}} + globalLocalSetDrives = nil + globalLocalDrivesMu.Unlock() + t.Cleanup(func() { + globalLocalDrivesMu.Lock() + globalLocalDrivesMap, globalLocalSetDrives = oldDrives, oldSets + globalLocalDrivesMu.Unlock() + }) + + oldRPC := storageNSScannerRPC + t.Cleanup(func() { storageNSScannerRPC = oldRPC }) + // Every new allocation is dirty too: the check must not depend on + // sync.Pool returning a previously used object. + storageNSScannerRPC = grid.NewStream[*nsScannerOptions, grid.NoPayload, *nsScannerResp](grid.HandlerNSScanner, + func() *nsScannerOptions { return &nsScannerOptions{} }, nil, + func() *nsScannerResp { + return &nsScannerResp{Update: &dataUsageEntry{Size: -1}, Final: &dataUsageCache{}} + }) + server := storageRESTServer{endpoint: endpoint} + for cycle := uint32(1); cycle <= 3; cycle++ { + out := make(chan *nsScannerResp, 2) + params := &nsScannerOptions{Cache: &dataUsageCache{Info: dataUsageCacheInfo{Name: "bucket", NextCycle: cycle}}} + if err := server.NSScannerHandler(t.Context(), params, out); err != nil { + t.Fatal(err) + } + if len(out) != 2 { + t.Fatalf("cycle %d: got %d responses, want an update and a final cache", cycle, len(out)) + } + for i := 0; i < 2; i++ { + resp := <-out + wire, err := resp.MarshalMsg(nil) + storageNSScannerRPC.PutResponse(resp) + if err != nil { + t.Fatal(err) + } + var decoded nsScannerResp + if _, err := decoded.UnmarshalMsg(wire); err != nil { + t.Fatal(err) + } + if i == 0 { + if decoded.Update == nil || decoded.Final != nil || decoded.Update.Size != int64(cycle) { + t.Errorf("cycle %d: invalid update response: %+v", cycle, decoded) + } + } else if decoded.Update != nil || decoded.Final == nil || decoded.Final.Info.NextCycle != cycle+1 { + t.Errorf("cycle %d: invalid final response: %+v", cycle, decoded) + } + } } } diff --git a/cmd/xl-storage-format-v2_test.go b/cmd/xl-storage-format-v2_test.go index 0076c8046..f5f682500 100644 --- a/cmd/xl-storage-format-v2_test.go +++ b/cmd/xl-storage-format-v2_test.go @@ -1214,14 +1214,34 @@ func TestXLMetaV2HeaderLegacyUnmarshalResetsEc(t *testing.T) { legacy = msgp.AppendUint8(legacy, uint8(base.Type)) legacy = msgp.AppendUint8(legacy, uint8(base.Flags)) - header := base - header.EcN, header.EcM = 4, 8 - x2 := xlMetaV2VersionHeaderV2{xlMetaV2VersionHeader: &header} - if _, err := x2.UnmarshalMsg(legacy); err != nil { - t.Fatalf("legacy V2 header unmarshal failed: %v", err) - } - if header.EcN != 0 || header.EcM != 0 { - t.Fatalf("stale erasure metadata leaked across objects: EcN %d, EcM %d", header.EcN, header.EcM) + for _, decoder := range []string{"UnmarshalMsg", "DecodeMsg", "loadIndexed"} { + t.Run(decoder, func(t *testing.T) { + header := base + header.EcN, header.EcM = 4, 8 + x2 := xlMetaV2VersionHeaderV2{xlMetaV2VersionHeader: &header} + var err error + switch decoder { + case "UnmarshalMsg": + _, err = x2.UnmarshalMsg(legacy) + case "DecodeMsg": + err = x2.DecodeMsg(msgp.NewReader(bytes.NewReader(legacy))) + case "loadIndexed": + indexed := msgp.AppendUint(nil, 2) + indexed = msgp.AppendUint(indexed, xlMetaVersion) + indexed = msgp.AppendInt(indexed, 1) + indexed = msgp.AppendBytes(indexed, legacy) + indexed = msgp.AppendBytes(indexed, nil) + xl := xlMetaV2{versions: []xlMetaV2ShallowVersion{{header: header}}} + err = xl.loadIndexed(indexed, nil) + header = xl.versions[0].header + } + if err != nil { + t.Fatal(err) + } + if header != base { + t.Fatalf("legacy header: got %+v, want %+v", header, base) + } + }) } // V1 headers (4 fields, never released) must zero the fields too. @@ -1232,7 +1252,7 @@ func TestXLMetaV2HeaderLegacyUnmarshalResetsEc(t *testing.T) { v1 = msgp.AppendUint8(v1, uint8(base.Type)) v1 = msgp.AppendUint8(v1, uint8(base.Flags)) - header = base + header := base header.EcN, header.EcM = 4, 8 header.Signature = [4]byte{9, 9, 9, 9} if _, err := header.unmarshalV1(v1); err != nil { diff --git a/internal/grid/handlers.go b/internal/grid/handlers.go index b0ca9e324..1d20fa9b9 100644 --- a/internal/grid/handlers.go +++ b/internal/grid/handlers.go @@ -698,11 +698,6 @@ func (h *StreamTypeHandler[Payload, Req, Resp]) PutRequest(r Req) { // These should be returned by the caller. func (h *StreamTypeHandler[Payload, Req, Resp]) PutResponse(r Resp) { if r != h.nilResp { - if rc, ok := any(r).(Recycler); ok { - // Recycled stream responses must not leak fields - // from a previous use into the next response. - rc.Recycle() - } //nolint:staticcheck // SA6002 IT IS A GENERIC VALUE! (and always a pointer) h.respPool.Put(r) }