From 1e91a99f79139d3fd8453d9354429ed6a0cf85b3 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 26 May 2026 16:56:49 -0700 Subject: [PATCH] fix(volume): avoid nil-deref when needle map loader errors (#9694) (#9697) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(volume): avoid nil-deref when needle map loader errors A corrupt .idx whose size is not a multiple of NeedleMapEntrySize sends the read-only load path into NewSortedFileNeedleMap, which returns (*SortedFileNeedleMap)(nil) when reverseWalkIndexFile rejects the file. The multi-value assignment `v.nm, err = NewSortedFileNeedleMap(...)` parks that typed-nil pointer in the v.nm NeedleMapper interface, so the subsequent `v.nm != nil` guard still passes — and the post-load MaxNeedleEnd structural check dispatches through the promoted mapMetric accessor on a nil receiver, segfaulting the whole volume server at load time. Reset v.nm explicitly after every loader failure so the interface is truly nil, and skip the MaxNeedleEnd check when err is non-nil since the value would come from a partial walk anyway. NewLevelDbNeedleMap has the same typed-nil-on-error shape and is fixed the same way. * fix(volume): close indexFile when needle map load errors Pre-fix the typed-nil v.nm path either leaked indexFile silently (SortedFileNeedleMap.Close had a nil-receiver early return) or crashed (LevelDbNeedleMap.Close had no such guard). With v.nm cleared to nil on error, the defer cleanup no longer calls Close at all, so the LoadCompactNeedleMap success-with-error path now also leaks indexFile. Close indexFile explicitly on each loader error to keep ownership balanced. * trim comments --- weed/storage/volume_loading.go | 18 +++++++- .../volume_loading_corrupt_idx_test.go | 41 +++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 weed/storage/volume_loading_corrupt_idx_test.go diff --git a/weed/storage/volume_loading.go b/weed/storage/volume_loading.go index 70ab51df8..6c5cf2284 100644 --- a/weed/storage/volume_loading.go +++ b/weed/storage/volume_loading.go @@ -279,9 +279,14 @@ func (v *Volume) load(alsoLoadIndex bool, createDatIfMissing bool, needleMapKind // (issue #8928). The check piggybacks on MaxNeedleEnd, which the load // walks below populate without a second linear scan. + // Loaders can return a typed-nil pointer with err set; assigning that + // to v.nm yields a non-nil interface over a nil receiver. Clear v.nm + // and close indexFile so the defer cleanup keys off v.nm cleanly. if v.noWriteOrDelete || v.noWriteCanDelete { if v.nm, err = NewSortedFileNeedleMap(v.IndexFileName(), indexFile, v.Version()); err != nil { glog.V(0).Infof("loading sorted db %s error: %v", v.FileName(".sdx"), err) + v.nm = nil + indexFile.Close() } } else { switch needleMapKind { @@ -293,6 +298,8 @@ func (v *Volume) load(alsoLoadIndex bool, createDatIfMissing bool, needleMapKind glog.V(2).Infoln("loading memory index", v.FileName(".idx"), "to memory") if v.nm, err = LoadCompactNeedleMap(indexFile, v.Version()); err != nil { glog.V(0).Infof("loading index %s to memory error: %v", v.FileName(".idx"), err) + v.nm = nil + indexFile.Close() } } case NeedleMapLevelDb: @@ -309,6 +316,8 @@ func (v *Volume) load(alsoLoadIndex bool, createDatIfMissing bool, needleMapKind glog.V(0).Infoln("loading leveldb index", v.FileName(".ldb")) if v.nm, err = NewLevelDbNeedleMap(v.FileName(".ldb"), indexFile, opts, v.ldbTimeout, v.Version()); err != nil { glog.V(0).Infof("loading leveldb %s error: %v", v.FileName(".ldb"), err) + v.nm = nil + indexFile.Close() } } case NeedleMapLevelDbMedium: @@ -325,6 +334,8 @@ func (v *Volume) load(alsoLoadIndex bool, createDatIfMissing bool, needleMapKind glog.V(0).Infoln("loading leveldb medium index", v.FileName(".ldb")) if v.nm, err = NewLevelDbNeedleMap(v.FileName(".ldb"), indexFile, opts, v.ldbTimeout, v.Version()); err != nil { glog.V(0).Infof("loading leveldb %s error: %v", v.FileName(".ldb"), err) + v.nm = nil + indexFile.Close() } } case NeedleMapLevelDbLarge: @@ -341,6 +352,8 @@ func (v *Volume) load(alsoLoadIndex bool, createDatIfMissing bool, needleMapKind glog.V(0).Infoln("loading leveldb large index", v.FileName(".ldb")) if v.nm, err = NewLevelDbNeedleMap(v.FileName(".ldb"), indexFile, opts, v.ldbTimeout, v.Version()); err != nil { glog.V(0).Infof("loading leveldb %s error: %v", v.FileName(".ldb"), err) + v.nm = nil + indexFile.Close() } } } @@ -351,8 +364,9 @@ func (v *Volume) load(alsoLoadIndex bool, createDatIfMissing bool, needleMapKind // MaximumNeedleEnd, so this is just a numeric comparison — no extra // disk I/O. A violation marks the volume read-only so a corrupt // .idx left over from a crashed batched write does not silently - // power vacuum to drop reachable data. See issue #8928. - if !v.HasRemoteFile() && v.nm != nil && v.DataBackend != nil { + // power vacuum to drop reachable data. See issue #8928. err == nil + // guards against a partial-walk MaximumNeedleEnd. + if err == nil && !v.HasRemoteFile() && v.nm != nil && v.DataBackend != nil { if datSize, _, statErr := v.DataBackend.GetStat(); statErr == nil && datSize > 0 { if maxEnd := v.nm.MaxNeedleEnd(); maxEnd > datSize { v.noWriteOrDelete = true diff --git a/weed/storage/volume_loading_corrupt_idx_test.go b/weed/storage/volume_loading_corrupt_idx_test.go new file mode 100644 index 000000000..ca9d02e1b --- /dev/null +++ b/weed/storage/volume_loading_corrupt_idx_test.go @@ -0,0 +1,41 @@ +package storage + +import ( + "os" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/storage/needle" + "github.com/seaweedfs/seaweedfs/weed/storage/super_block" +) + +// Corrupt .idx made NewSortedFileNeedleMap return a typed-nil into v.nm; +// the post-load MaxNeedleEnd check then segfaulted on the nil receiver. +func TestLoad_CorruptIdx_NoSegfault(t *testing.T) { + dir := t.TempDir() + + v, err := NewVolume(dir, dir, "", 1, NeedleMapInMemory, &super_block.ReplicaPlacement{}, &needle.TTL{}, 0, needle.GetCurrentVersion(), 0, 0) + if err != nil { + t.Fatalf("create volume: %v", err) + } + if _, _, _, err := v.writeNeedle2(newRandomNeedle(1), true, false); err != nil { + t.Fatalf("seed write: %v", err) + } + v.PersistReadOnly(true) // reload goes through SortedFileNeedleMap + v.Close() + + // Truncate .idx to a non-aligned size so the walk rejects it. + idxPath := VolumeFileName(dir, "", 1) + ".idx" + st, err := os.Stat(idxPath) + if err != nil { + t.Fatalf("stat idx: %v", err) + } + if err := os.Truncate(idxPath, st.Size()-1); err != nil { + t.Fatalf("truncate idx: %v", err) + } + + // Pre-fix this panicked inside (*mapMetric).MaxNeedleEnd. + v2, err := NewVolume(dir, dir, "", 1, NeedleMapInMemory, &super_block.ReplicaPlacement{}, &needle.TTL{}, 0, needle.GetCurrentVersion(), 0, 0) + if err == nil { + v2.Close() + } +}