diff --git a/AGENTS.md b/AGENTS.md index cf9502d..54ffd75 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -82,8 +82,10 @@ entry point is `NewStream` with `Submit`/`Close`/`Wait`; `Run` and `BatchSource` read-only device, and a disk-usage refresh keeps the reservations of copies still in flight. - A file system without the managed signature attribute stores no cache at all, which is a no-op rather than a recorded failure. -- `mmap` returns `io.EOF` at the end of the content, releases a mapping whose `madvise` failed, - closes idempotently on every platform, validates a slice range before allocating it, and retains +- `mmap` returns `io.EOF` at the end of the content, reports an open mapping of an empty file as + empty rather than closed, releases a mapping whose `madvise` failed, issues the Linux sequential + hint and prefetch as one call each (an madvise advice is a value, not a flag bit), closes + idempotently on every platform, validates a slice range before allocating it, and retains the descriptor it opened: `Close` removes the mapping and then closes that descriptor. - One source descriptor serves a whole item. The stored hash is read through the descriptor that reads the content, and the computed hash is published through it, synchronously, after the hash diff --git a/README.md b/README.md index 85cb368..35f6497 100644 --- a/README.md +++ b/README.md @@ -288,7 +288,7 @@ preallocated, what metadata can be restored, and how a mapped read is implemente | Platform | Source access time | Managed signature attribute | Target preallocation | Metadata restore | Mapped read | | --- | --- | --- | --- | --- | --- | -| Linux | suppressed: a buffered source opens with `O_NOATIME`, falling back to an ordinary open when the flag is refused | `user.acp.signature`, in the `user.` namespace | `fallocate` reserves the whole target size | xattrs, mode, owner as root, times | `syscall.Mmap`, plus `MADV_SEQUENTIAL\|MADV_WILLNEED` for files up to 16 MiB | +| Linux | suppressed: a buffered source opens with `O_NOATIME`, falling back to an ordinary open when the flag is refused | `user.acp.signature`, in the `user.` namespace | `fallocate` reserves the whole target size | xattrs, mode, owner as root, times | `syscall.Mmap`, plus `MADV_SEQUENTIAL` and `MADV_WILLNEED` (one advice per call) for files up to 16 MiB | | Darwin | not suppressed | `acp.signature` | `Truncate` | xattrs, mode, owner as root, times | `syscall.Mmap` | | FreeBSD | not suppressed | `acp.signature`, in the user extended-attribute namespace | `Truncate` | mode and times only | no mapping: the descriptor is read with `ReadAt` | | Windows | not suppressed | none: the cache is a silent no-op | `Truncate` | mode and times only | `CreateFileMapping` plus `MapViewOfFile` | @@ -399,7 +399,8 @@ document keep their shape, and the shell fills one terminal row per item. ## Divergence from upstream `mmap` This module started from the Go project's `mmap` package and is maintained here now: reads run -through the descriptor ACP owns, `Close` is idempotent and removes the mapping before it closes +through the descriptor ACP owns, the end of the content is `io.EOF` and an open mapping of an empty +file is empty rather than closed, `Close` is idempotent and removes the mapping before it closes that descriptor, a slice range is validated before anything is allocated, and a platform without a mapping reads through the descriptor instead. Upstream changes are not merged automatically. diff --git a/mmap/mmap_darwin.go b/mmap/mmap_darwin.go index ba0c27b..d186dc2 100644 --- a/mmap/mmap_darwin.go +++ b/mmap/mmap_darwin.go @@ -128,8 +128,9 @@ func Open(filename string) (*ReaderAt, error) { return nil, fmt.Errorf("mmap: file %q is too large", filename) } - // An empty file has no mapping, but it still has the descriptor this reader owns. - var data []byte + // An empty file has no mapping, but it still has the descriptor this reader owns. The mapping + // of a zero-length file is an empty, non-nil slice, so only a closed reader reports a nil one. + data := make([]byte, 0) if size != 0 { data, err = syscall.Mmap(int(f.Fd()), 0, int(size), syscall.PROT_READ, syscall.MAP_SHARED) if err != nil { diff --git a/mmap/mmap_linux.go b/mmap/mmap_linux.go index 6a70be7..61805dc 100644 --- a/mmap/mmap_linux.go +++ b/mmap/mmap_linux.go @@ -21,6 +21,19 @@ const ( prefetchMaxSize = 16 * 1024 * 1024 ) +// linuxPrefetchAdvice is the advice a file that fits the prefetch window receives. An madvise call +// takes exactly one advice: these are values, not flag bits, so `MADV_SEQUENTIAL|MADV_WILLNEED` is +// simply `MADV_WILLNEED` and the sequential hint would be dropped. The read is sequential and the +// whole mapping should be faulted in up front, so both are issued, one per call. +var linuxPrefetchAdvice = []int{syscall.MADV_SEQUENTIAL, syscall.MADV_WILLNEED} + +// madvise and munmap are indirected so a test can drive the prefetch failure path, which otherwise +// needs syscall injection. +var ( + madvise = syscall.Madvise + munmap = syscall.Munmap +) + // debug is whether to print debugging messages for manual testing. // // The runtime.SetFinalizer documentation says that, "The finalizer for x is @@ -132,8 +145,9 @@ func Open(filename string) (*ReaderAt, error) { return nil, fmt.Errorf("mmap: file %q is too large", filename) } - // An empty file has no mapping, but it still has the descriptor this reader owns. - var data []byte + // An empty file has no mapping, but it still has the descriptor this reader owns. The mapping + // of a zero-length file is an empty, non-nil slice, so only a closed reader reports a nil one. + data := make([]byte, 0) if size != 0 { data, err = syscall.Mmap(int(f.Fd()), 0, int(size), syscall.PROT_READ, syscall.MAP_SHARED) if err != nil { @@ -141,12 +155,14 @@ func Open(filename string) (*ReaderAt, error) { return nil, fmt.Errorf("create mmap fail, %q, %w", filename, err) } if size <= prefetchMaxSize { - if err := syscall.Madvise(data, syscall.MADV_SEQUENTIAL|syscall.MADV_WILLNEED); err != nil { - // The mapping is not reachable through a ReaderAt yet, so nothing else can release - // it: the error path owns the unmapping and the descriptor. - _ = syscall.Munmap(data) - _ = f.Close() - return nil, fmt.Errorf("madvise fail, %q, %w", filename, err) + for _, advice := range linuxPrefetchAdvice { + if err := madvise(data, advice); err != nil { + // The mapping is not reachable through a ReaderAt yet, so nothing else can + // release it: the error path owns the unmapping and the descriptor. + _ = munmap(data) + _ = f.Close() + return nil, fmt.Errorf("madvise fail, %q, %w", filename, err) + } } } } diff --git a/mmap/mmap_linux_test.go b/mmap/mmap_linux_test.go new file mode 100644 index 0000000..f109b7a --- /dev/null +++ b/mmap/mmap_linux_test.go @@ -0,0 +1,118 @@ +//go:build linux + +package mmap + +import ( + "errors" + "os" + "syscall" + "testing" +) + +// recordPrefetch replaces the madvise syscall with a spy that records every advice the prefetch +// issues, so a test can pin the advice sequence and the error path without syscall injection. +func recordPrefetch(t *testing.T, fail error) *[]int { + t.Helper() + + previous := madvise + advices := new([]int) + madvise = func(_ []byte, advice int) error { + *advices = append(*advices, advice) + return fail + } + t.Cleanup(func() { madvise = previous }) + + return advices +} + +// TestPrefetchIssuesEveryAdvice pins the Linux prefetch. An madvise call takes exactly one advice: +// these are values, not flag bits, so passing `MADV_SEQUENTIAL|MADV_WILLNEED` keeps only the +// prefetch and silently drops the sequential hint, which is why the two hints are two calls. +func TestPrefetchIssuesEveryAdvice(t *testing.T) { + if syscall.MADV_SEQUENTIAL|syscall.MADV_WILLNEED != syscall.MADV_WILLNEED { + t.Fatalf("this test assumes the two constants are not flag bits on this platform") + } + + advices := recordPrefetch(t, nil) + reader, err := Open(writeFixture(t, "prefetch.bin", []byte("0123456789"))) + if err != nil { + t.Fatalf("Open: %v", err) + } + t.Cleanup(func() { _ = reader.Close() }) + + if len(*advices) != len(linuxPrefetchAdvice) { + t.Fatalf("prefetch issued advice %v, want %v", *advices, linuxPrefetchAdvice) + } + for index, want := range linuxPrefetchAdvice { + if (*advices)[index] != want { + t.Fatalf("prefetch issued advice %v, want %v", *advices, linuxPrefetchAdvice) + } + } +} + +// TestPrefetchSkipsAFileAboveTheWindow pins the documented threshold: a mapping above +// prefetchMaxSize receives no advice at all, so a large copy does not fault in its whole content. +func TestPrefetchSkipsAFileAboveTheWindow(t *testing.T) { + advices := recordPrefetch(t, nil) + + path := writeFixture(t, "large.bin", nil) + if err := os.Truncate(path, prefetchMaxSize+1); err != nil { + t.Fatalf("grow the fixture: %v", err) + } + reader, err := Open(path) + if err != nil { + t.Fatalf("Open: %v", err) + } + t.Cleanup(func() { _ = reader.Close() }) + + if len(*advices) != 0 { + t.Fatalf("a file above the prefetch window received advice %v", *advices) + } +} + +// TestPrefetchFailureReleasesTheMapping pins the documented error path: a mapping whose madvise +// failed is released before the error returns, and the descriptor opened for it is closed with it, +// so a host that refuses the advice cannot leak either. +func TestPrefetchFailureReleasesTheMapping(t *testing.T) { + failure := errors.New("madvise fixture") + recordPrefetch(t, failure) + + previousUnmap := munmap + unmapped := 0 + munmap = func(data []byte) error { + unmapped++ + return previousUnmap(data) + } + t.Cleanup(func() { munmap = previousUnmap }) + + path := writeFixture(t, "advice-failure.bin", []byte("0123456789")) + before := descriptorCount(t) + + const attempts = 8 + for index := 0; index < attempts; index++ { + reader, err := Open(path) + if reader != nil || err == nil { + t.Fatalf("Open with a failing madvise = (%v, %v), want a failure", reader, err) + } + if !errors.Is(err, failure) { + t.Fatalf("Open error = %v, want %v", err, failure) + } + } + if unmapped != attempts { + t.Fatalf("released %d mappings, want one per failed open", unmapped) + } + if after := descriptorCount(t); after > before { + t.Fatalf("descriptors leaked: %d -> %d", before, after) + } +} + +// descriptorCount counts this process's open descriptors. +func descriptorCount(t *testing.T) int { + t.Helper() + + entries, err := os.ReadDir("/proc/self/fd") + if err != nil { + t.Skipf("cannot count descriptors: %v", err) + } + return len(entries) +} diff --git a/mmap/mmap_other.go b/mmap/mmap_other.go index 9fd64fb..b91fdb0 100644 --- a/mmap/mmap_other.go +++ b/mmap/mmap_other.go @@ -12,6 +12,7 @@ import ( "errors" "fmt" "os" + "runtime" ) // ReaderAt reads a memory-mapped file. @@ -24,8 +25,8 @@ type ReaderAt struct { } // Close closes the reader. It is idempotent: a reader that is already closed reports success, -// because the file it owned is already gone. A closed reader reports an empty mapping, exactly -// like the mapping-backed platforms, so a reader that reads after Close sees io.EOF. +// because the file it owned is already gone. A closed reader reports an empty mapping on every +// platform, so Len() is zero and a stream over it ends with io.EOF instead of erroring. func (r *ReaderAt) Close() error { if r.file == nil { return nil @@ -114,8 +115,13 @@ func Open(filename string) (*ReaderAt, error) { return nil, fmt.Errorf("mmap: file %q is too large", filename) } - return &ReaderAt{ + r := &ReaderAt{ file: f, len: int(fi.Size()), - }, nil + } + + // A reader that is abandoned without Close releases its descriptor on collection, exactly like + // the mapping-backed platforms, so this fallback cannot leak one either. + runtime.SetFinalizer(r, (*ReaderAt).Close) + return r, nil } diff --git a/mmap/mmap_test.go b/mmap/mmap_test.go index 8745901..139b281 100644 --- a/mmap/mmap_test.go +++ b/mmap/mmap_test.go @@ -94,6 +94,41 @@ func TestReaderAtContract(t *testing.T) { } } +// TestOpenEmptyMappingIsNotEmpty pins the empty mapping against the closed-reader sentinel: an open +// reader over a zero-length file is empty, not closed, so it behaves like an empty *os.File or +// bytes.Reader. Reporting it as closed breaks every io.ReaderAt consumer that probes an empty +// source, such as archive/zip. +func TestOpenEmptyMappingIsNotClosed(t *testing.T) { + reader, err := Open(writeFixture(t, "empty-open.bin", nil)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if reader.Len() != 0 { + t.Fatalf("Len() = %d, want 0", reader.Len()) + } + + if n, err := reader.ReadAt(make([]byte, 4), 0); n != 0 || err != io.EOF { + t.Fatalf("ReadAt(4, 0) = (%d, %v), want (0, %v)", n, err, io.EOF) + } + if n, err := reader.ReadAt(nil, 0); n != 0 || err != nil { + t.Fatalf("ReadAt(nil, 0) = (%d, %v), want (0, nil)", n, err) + } + if window, err := reader.Slice(0, 4); err != nil || len(window) != 0 { + t.Fatalf("Slice(0, 4) = (%d bytes, %v), want an empty window", len(window), err) + } + + // Only a closed reader reports itself as closed. + if err := reader.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if _, err := reader.ReadAt(make([]byte, 1), 0); err == nil || errors.Is(err, io.EOF) { + t.Fatalf("ReadAt after Close = %v, want a closed-reader error", err) + } + if _, err := reader.Slice(0, 1); err == nil || errors.Is(err, io.EOF) { + t.Fatalf("Slice after Close = %v, want a closed-reader error", err) + } +} + // TestOpenEmptyFile pins the empty mapping: it has no content, it closes cleanly, and a reader // over it reports the end of the content instead of a broken reader. func TestOpenEmptyFile(t *testing.T) { diff --git a/mmap/mmap_windows.go b/mmap/mmap_windows.go index 33007f9..733a8e3 100644 --- a/mmap/mmap_windows.go +++ b/mmap/mmap_windows.go @@ -126,8 +126,9 @@ func Open(filename string) (*ReaderAt, error) { return nil, fmt.Errorf("mmap: file %q is too large", filename) } - // An empty file has no mapping, but it still has the descriptor this reader owns. - var data []byte + // An empty file has no mapping, but it still has the descriptor this reader owns. The mapping + // of a zero-length file is an empty, non-nil slice, so only a closed reader reports a nil one. + data := make([]byte, 0) if size != 0 { low, high := uint32(size), uint32(size>>32) fmap, err := syscall.CreateFileMapping(syscall.Handle(f.Fd()), nil, syscall.PAGE_READONLY, high, low, nil)