From 3b3e1a62c59b0a5e5eeec4ea2ffd16db715a09db Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Tue, 11 Aug 2026 11:48:48 +0700 Subject: [PATCH 1/3] common/mmap, db/seg: poison a closed file's mapping instead of unmapping it A slice borrowed from a .kv mmap that outlives its file has two possible endings, and the harmless-looking one is the dangerous one. If the address stays vacant the next read faults and the process dies. If a later file is mapped over it the read returns that file's bytes and nothing complains -- which is how a lifetime bug surfaces as an eth_getProof diff rather than a crash. MMAP_POISON=true makes Decompressor.Close mprotect(PROT_NONE) the range and keep it, so the address can never be handed to another file and every stale read faults at the read that misuses it. The range leaks for the life of the process, so this is a diagnostic switch, not a default. The fault arrives as SIGSEGV/SEGV_ACCERR on Linux and SIGBUS on Darwin, so the test pins the code only where it runs in production. --- common/dbg/experiments.go | 7 ++- common/mmap/mmap_unix.go | 12 ++++ common/mmap/mmap_windows.go | 14 +++++ common/mmap/poison_test.go | 103 +++++++++++++++++++++++++++++++++++ db/seg/decompress.go | 6 +- db/seg/poison_wiring_test.go | 58 ++++++++++++++++++++ 6 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 common/mmap/poison_test.go create mode 100644 db/seg/poison_wiring_test.go diff --git a/common/dbg/experiments.go b/common/dbg/experiments.go index 0ff10651dc2..ae42ce990ef 100644 --- a/common/dbg/experiments.go +++ b/common/dbg/experiments.go @@ -137,7 +137,12 @@ var ( UseCodeStore = EnvBool("USE_CODE_STORE", true) DisableAdaptivePin = EnvBool("DISABLE_ADAPTIVE_PIN", false) AssertStateCache = EnvBool("ASSERT_STATE_CACHE", false) - ReadAhead = EnvBool("READ_AHEAD", true) + // MmapPoison makes a closed file revoke its mapping instead of unmapping it, + // turning a read through a slice that outlived its file into a fault at that + // read. Without it the address is free for the next file, and the stale read + // silently returns another file's bytes. Leaks address space — diagnostics only. + MmapPoison = EnvBool("MMAP_POISON", false) + ReadAhead = EnvBool("READ_AHEAD", true) // FilesAsyncIO warms cold state .kv pages via io_uring before the mmap read, so // a would-be blocking page fault becomes a non-blocking read that releases the // goroutine's P. Linux + io_uring only; self-disables (reads use ordinary faults) diff --git a/common/mmap/mmap_unix.go b/common/mmap/mmap_unix.go index c5ae2d56c7b..39d7be76690 100644 --- a/common/mmap/mmap_unix.go +++ b/common/mmap/mmap_unix.go @@ -91,3 +91,15 @@ func Munmap(mmapHandle1 []byte, _ *[MaxMapSize]byte) error { err := unix.Munmap(mmapHandle1) return err } + +// Poison revokes access to the range but deliberately leaves it mapped, so the +// address can never be reused. A slice that outlived its file then faults at +// the read that misuses it, instead of silently returning whichever file the +// kernel mapped there next. Diagnostics only: the range leaks for the life of +// the process. +func Poison(mmapHandle1 []byte, _ *[MaxMapSize]byte) error { + if mmapHandle1 == nil { + return nil + } + return unix.Mprotect(mmapHandle1, unix.PROT_NONE) +} diff --git a/common/mmap/mmap_windows.go b/common/mmap/mmap_windows.go index e29d6fc2c4b..b33fef1a434 100644 --- a/common/mmap/mmap_windows.go +++ b/common/mmap/mmap_windows.go @@ -66,3 +66,17 @@ func Munmap(_ []byte, mmapHandle2 *[MaxMapSize]byte) error { } return nil } + +// Poison revokes access to the range but deliberately leaves it mapped, so the +// address can never be reused. A slice that outlived its file then faults at +// the read that misuses it, instead of silently returning whichever file the +// kernel mapped there next. Diagnostics only: the range leaks for the life of +// the process. +func Poison(mmapHandle1 []byte, mmapHandle2 *[MaxMapSize]byte) error { + if mmapHandle2 == nil { + return nil + } + addr := (uintptr)(unsafe.Pointer(&mmapHandle2[0])) + var old uint32 + return windows.VirtualProtect(addr, uintptr(len(mmapHandle1)), windows.PAGE_NOACCESS, &old) +} diff --git a/common/mmap/poison_test.go b/common/mmap/poison_test.go new file mode 100644 index 00000000000..f9543f70007 --- /dev/null +++ b/common/mmap/poison_test.go @@ -0,0 +1,103 @@ +//go:build !windows + +package mmap + +import ( + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// A read through a poisoned range kills the process, so the read has to happen +// in a child. The parent asserts on how the child died. +const poisonChildEnv = "MMAP_POISON_TEST_CHILD" + +func TestMain(m *testing.M) { + if os.Getenv(poisonChildEnv) == "" { + os.Exit(m.Run()) + } + poisonChild() +} + +func poisonChild() { + f, err := os.CreateTemp("", "poison") + if err != nil { + os.Exit(3) + } + defer os.Remove(f.Name()) + if err := f.Truncate(4096); err != nil { + os.Exit(3) + } + data, handle, err := Mmap(f, 4096) + if err != nil { + os.Exit(3) + } + borrowed := data[:8] + _ = borrowed[0] // readable while the file is open + if err := Poison(data, handle); err != nil { + os.Exit(3) + } + // The file is "closed" now. A stale borrow must not survive this. + if borrowed[0] == 0xFF { + os.Exit(4) + } + os.Exit(5) // reached only if the read did not fault +} + +func TestPoisonFaultsOnStaleRead(t *testing.T) { + exe, err := os.Executable() + require.NoError(t, err) + + cmd := exec.Command(exe, "-test.run", "TestPoisonFaultsOnStaleRead") + cmd.Env = append(os.Environ(), poisonChildEnv+"=1") + out, err := cmd.CombinedOutput() + + require.Error(t, err, "reading a poisoned range must kill the child, got clean exit:\n%s", out) + require.Contains(t, string(out), "fatal error: fault", + "child should die on a memory fault, not exit normally:\n%s", out) + + // Which signal carries the fault is the OS's choice: Linux reports + // SIGSEGV/SEGV_ACCERR, Darwin reports SIGBUS for a protection violation on a + // file-backed mapping. Only Linux runs in production, so pin the code there. + if runtime.GOOS == "linux" { + require.Contains(t, string(out), "SIGSEGV") + require.Contains(t, string(out), "code=0x2", + "the range must still be mapped (SEGV_ACCERR), not unmapped (SEGV_MAPERR):\n%s", out) + } +} + +// TestPoisonKeepsAddressReserved is the property that makes poisoning useful: +// the range stays owned, so no later file can be handed the same address and +// answer a stale read with plausible bytes. +func TestPoisonKeepsAddressReserved(t *testing.T) { + dir := t.TempDir() + first, err := os.Create(filepath.Join(dir, "a.bin")) + require.NoError(t, err) + defer first.Close() + require.NoError(t, first.Truncate(4096)) + + data, handle, err := Mmap(first, 4096) + require.NoError(t, err) + addr := &data[0] + + require.NoError(t, Poison(data, handle)) + + // Map enough further files to make address reuse likely had the range + // been released. + for i := range 32 { + f, err := os.Create(filepath.Join(dir, strings.Repeat("b", i+1)+".bin")) + require.NoError(t, err) + require.NoError(t, f.Truncate(4096)) + next, _, err := Mmap(f, 4096) + require.NoError(t, err) + require.NotSame(t, addr, &next[0], + "a later file was handed the poisoned address — a stale read there would "+ + "return this file's bytes instead of faulting") + f.Close() + } +} diff --git a/db/seg/decompress.go b/db/seg/decompress.go index c1f94c91151..dded5e6a160 100644 --- a/db/seg/decompress.go +++ b/db/seg/decompress.go @@ -578,7 +578,11 @@ func (d *Decompressor) Close() { rb.stop() // join the refresh goroutine before munmap so no mincore touches freed memory d.residency.Store(nil) } - if err := mmap.Munmap(d.mmapHandle1, d.mmapHandle2); err != nil { + unmap := mmap.Munmap + if dbg.MmapPoison { + unmap = mmap.Poison + } + if err := unmap(d.mmapHandle1, d.mmapHandle2); err != nil { log.Log(dbg.FileCloseLogLevel, "unmap", "err", err, "file", d.FileName(), "stack", dbg.Stack()) } if err := d.f.Close(); err != nil { diff --git a/db/seg/poison_wiring_test.go b/db/seg/poison_wiring_test.go new file mode 100644 index 00000000000..66b65c3efee --- /dev/null +++ b/db/seg/poison_wiring_test.go @@ -0,0 +1,58 @@ +//go:build linux + +package seg + +import ( + "fmt" + "os" + "strconv" + "strings" + "testing" + "unsafe" + + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/common/dbg" +) + +// mappedAt reports whether the calling process still owns a mapping covering addr. +func mappedAt(t *testing.T, addr uintptr) bool { + t.Helper() + maps, err := os.ReadFile("/proc/self/maps") + require.NoError(t, err) + for line := range strings.SplitSeq(string(maps), "\n") { + fields := strings.Fields(line) + if len(fields) == 0 { + continue + } + lo, hi, ok := strings.Cut(fields[0], "-") + if !ok { + continue + } + start, err1 := strconv.ParseUint(lo, 16, 64) + end, err2 := strconv.ParseUint(hi, 16, 64) + if err1 != nil || err2 != nil { + continue + } + if addr >= uintptr(start) && addr < uintptr(end) { + return true + } + } + return false +} + +// TestPoisonWiring proves the dbg flag reaches Decompressor.Close: with it set +// the closed file keeps its address range (revoked, never reusable); without it +// the range is released back to the kernel. +func TestPoisonWiring(t *testing.T) { + d := prepareLoremDict(t) + addr := uintptr(unsafe.Pointer(&d.mmapHandle1[0])) + require.True(t, mappedAt(t, addr), "mapped while open") + + d.Close() + + still := mappedAt(t, addr) + fmt.Printf("MMAP_POISON=%v -> address %#x still mapped after Close: %v\n", dbg.MmapPoison, addr, still) + require.Equal(t, dbg.MmapPoison, still, + "poison must retain the range so it cannot be handed to another file; plain Close must release it") +} From 5591f0f6c4a2000d35189353dad8ed418a1cf69f Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Tue, 11 Aug 2026 12:55:37 +0700 Subject: [PATCH 2/3] common/mmap: unlink the poison test's file the way a merge does Removes the useless defer (os.Exit skips it) and routes the unlink through common/dir, which is what the ruleguard asks for. Unlinking while mapped also matches what a merge does to a superseded file. --- common/mmap/poison_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/common/mmap/poison_test.go b/common/mmap/poison_test.go index f9543f70007..024711e397c 100644 --- a/common/mmap/poison_test.go +++ b/common/mmap/poison_test.go @@ -11,6 +11,8 @@ import ( "testing" "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/common/dir" ) // A read through a poisoned range kills the process, so the read has to happen @@ -29,7 +31,6 @@ func poisonChild() { if err != nil { os.Exit(3) } - defer os.Remove(f.Name()) if err := f.Truncate(4096); err != nil { os.Exit(3) } @@ -39,6 +40,11 @@ func poisonChild() { } borrowed := data[:8] _ = borrowed[0] // readable while the file is open + + // Unlink while mapped, the way a merge disposes of a superseded file. + if err := dir.RemoveFile(f.Name()); err != nil { + os.Exit(3) + } if err := Poison(data, handle); err != nil { os.Exit(3) } From d2a81c6d4736068fd399bb39c0119e741bd0a6fc Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Tue, 11 Aug 2026 13:34:07 +0700 Subject: [PATCH 3/3] db/seg: ask whether the file is still mapped, not whether the address is The wiring test checked /proc/self/maps for the address the mapping used to occupy. A released address is reused almost immediately -- db/seg's own tests mmap heavily -- so an unrelated mapping landing there would read as 'the poison kept it', failing the unpoisoned case for the wrong reason. Match on the file instead. --- db/seg/poison_wiring_test.go | 42 +++++++++++++----------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/db/seg/poison_wiring_test.go b/db/seg/poison_wiring_test.go index 66b65c3efee..a8c3e25024e 100644 --- a/db/seg/poison_wiring_test.go +++ b/db/seg/poison_wiring_test.go @@ -3,38 +3,27 @@ package seg import ( - "fmt" "os" - "strconv" + "path/filepath" "strings" "testing" - "unsafe" "github.com/stretchr/testify/require" "github.com/erigontech/erigon/common/dbg" ) -// mappedAt reports whether the calling process still owns a mapping covering addr. -func mappedAt(t *testing.T, addr uintptr) bool { +// fileStillMapped reports whether this process still holds a mapping of path. +// Asking by file rather than by address matters: a released address is reused +// almost immediately, so "something is mapped here" says nothing about whether +// our file's mapping survived. +func fileStillMapped(t *testing.T, path string) bool { t.Helper() maps, err := os.ReadFile("/proc/self/maps") require.NoError(t, err) + base := filepath.Base(path) for line := range strings.SplitSeq(string(maps), "\n") { - fields := strings.Fields(line) - if len(fields) == 0 { - continue - } - lo, hi, ok := strings.Cut(fields[0], "-") - if !ok { - continue - } - start, err1 := strconv.ParseUint(lo, 16, 64) - end, err2 := strconv.ParseUint(hi, 16, 64) - if err1 != nil || err2 != nil { - continue - } - if addr >= uintptr(start) && addr < uintptr(end) { + if strings.HasSuffix(line, base) || strings.HasSuffix(line, base+" (deleted)") { return true } } @@ -42,17 +31,16 @@ func mappedAt(t *testing.T, addr uintptr) bool { } // TestPoisonWiring proves the dbg flag reaches Decompressor.Close: with it set -// the closed file keeps its address range (revoked, never reusable); without it -// the range is released back to the kernel. +// the closed file keeps its range (revoked, never reusable); without it the +// range is handed back to the kernel. func TestPoisonWiring(t *testing.T) { d := prepareLoremDict(t) - addr := uintptr(unsafe.Pointer(&d.mmapHandle1[0])) - require.True(t, mappedAt(t, addr), "mapped while open") + path := d.FilePath() + require.True(t, fileStillMapped(t, path), "mapped while open") d.Close() - still := mappedAt(t, addr) - fmt.Printf("MMAP_POISON=%v -> address %#x still mapped after Close: %v\n", dbg.MmapPoison, addr, still) - require.Equal(t, dbg.MmapPoison, still, - "poison must retain the range so it cannot be handed to another file; plain Close must release it") + require.Equal(t, dbg.MmapPoison, fileStillMapped(t, path), + "poison must retain the mapping so the address cannot be handed to another "+ + "file; a plain Close must release it") }