seg: poison a closed file's mmap - #23161
Draft
AskAlexSharov wants to merge 3 commits into
Draft
Conversation
…ing 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.
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.
AskAlexSharov
changed the title
AskAlexSharov
changed the title
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.
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
1 participant
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.
A slice borrowed from a
.kvmmap 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 gets mapped over it, the read returns that file's bytes and nothing complains — which is how a file-lifetime bug surfaces as aneth_getProofdiff instead of a crash.ERIGON_MMAP_POISON=truemakesDecompressor.Closemprotect(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 — diagnostic switch, not a default.Demonstrated on Linux 6.8, showing the two endings of the same stale borrow:
Tests: a child process proves a poisoned range faults; a second test maps 32 further files and asserts none is given the poisoned address; a
/proc/self/mapstest pins that the flag actually reachesDecompressor.Close(off → range released, on → retained).The fault arrives as
SIGSEGV/SEGV_ACCERRon Linux andSIGBUSon Darwin, so the test pins the code only where it runs in production.Scope:
seg/.kvmappings only. MDBX owns its own mapping and has a separate borrow contract (GetOnepasses the mmap view straight through, valid only while the read txn lives), which poisoning cannot reach.Motivation:
TrieContext.Branchclones every returned slice — ~631 MB/s of alloc traffic at tip per #21524, which removed it and was reverted by #21630 after widespread RPC proof/trace corruption with no root cause identified. This makes a second attempt testable instead of a gamble.