From 5be0952efaec59e8a0598e5731c98abd4e12ba12 Mon Sep 17 00:00:00 2001 From: Samuel Cui Date: Sat, 19 Sep 2026 18:31:29 +0800 Subject: [PATCH] docs: state the mmap contract and pin the xattr rule on the copy path Three review follow-ups that the last release review asked for: - TESTING.md no longer claims a formulation vet accepts for the Windows mapped slice conversion; measured on go1.26.8/windows-amd64, the unsafe.Slice call, a typed pointer in a variable and an array cast are all flagged, and the one accepted shape is the deprecated reflect.SliceHeader field assignment, which trades a false positive for a less safe conversion. - The report chapter drops the byte-identity claim about rows written without full_path and signature_cache_hit and states what the fields are instead. - The mmap divergence note and File's own comment now say that ReadAt past the end reports an invalid offset rather than io.EOF, and that a caller keeping the descriptor must keep the reader reachable because the finalizer owns it. - TestManagedSignatureIsNotOrdinaryCopiedXattr now exercises the copy path: a decoy managed entry on the source and an ordinary control attribute prove the metadata restore ran, the target carries no managed attribute, and the source keeps its own entry. --- README.md | 14 ++++-- TESTING.md | 12 +++-- mmap/mmap_reader.go | 4 +- signature_xattr_copy_test.go | 95 ++++++++++++++++++++++++++++++------ 4 files changed, 98 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 2e5490d..a81684d 100644 --- a/README.md +++ b/README.md @@ -363,11 +363,10 @@ c.Wait() repeated relative path inside one job option already ends the run, and a library caller that submits overlapping job options should treat the shared row as a known limit of the report surface. -- A row keeps the `af05f05c` fields: `base` is the directory the source-relative `path` segments - are resolved against, and `path` is an array. `full_path` names the source as one whole path - and `signature_cache_hit` reports where the hash came from; both are additive fields, so a row - written without them is the document `af05f05c` wrote. `NewReportGetter` still keys a row by - the joined relative path. +- A row carries `base`, the directory the source-relative `path` segments are resolved against, and + `path` as an array. `full_path` names the source as one whole path and `signature_cache_hit` + reports where the hash came from; both are additive fields that a row written before them does not + have. `NewReportGetter` keys a row by the joined relative path. - Two behaviours differ from `af05f05c` and are deliberate. First, an error the pipeline reports for a path — a walk that could not read a directory, a target that could not be removed after its metadata failed — is a run error, so `WaitErr` returns it and `cmd/acp` exits non-zero; @@ -408,6 +407,11 @@ file is empty rather than closed, `Close` is idempotent and removes the mapping 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. +Two details of that contract are worth stating: a `ReadAt` that starts past the end of the content +reports an invalid offset rather than `io.EOF`, because only reaching the end of the content is the +end of the file; and `File` hands out the descriptor the reader owns, so a caller that keeps it must +keep the reader reachable, since the finalizer closes the descriptor once the reader is gone. + # Install ``` # Install acp diff --git a/TESTING.md b/TESTING.md index 1edef7f..057ce8b 100644 --- a/TESTING.md +++ b/TESTING.md @@ -267,11 +267,13 @@ GOOS=windows GOARCH=amd64 go test -c -o /tmp/acp-windows.test . ``` `make check` builds for Windows but vets only the host platform. `GOOS=windows GOARCH=amd64 go -vet ./...` reports `possible misuse of unsafe.Pointer` in `mmap/mmap_windows.go`, on the -`unsafe.Slice` call that turns the address `syscall.MapViewOfFile` returned into the mapped slice. -That conversion is the supported way to read a mapped address, vet's `unsafeptr` check cannot know -that a syscall result is a valid pointer, and the only formulation it accepts is the deprecated -`reflect.SliceHeader` pattern, so the diagnostic is accepted instead of worked around. +vet ./...` reports the conversion that turns the address `syscall.MapViewOfFile` returned into the +mapped slice, and it flags that conversion in every formulation this package would use: the +`unsafe.Slice` call in `mmap/mmap_windows.go`, a typed pointer kept in a variable, an array cast, +and a `reflect.SliceHeader` literal, which gets its own `possible misuse of reflect.SliceHeader` +message. The one shape vet accepts is the deprecated `(*reflect.SliceHeader)(unsafe.Pointer(&data))` +field assignment, which trades a false positive for a less safe conversion, so the diagnostic is +accepted rather than worked around. ## Change checklist diff --git a/mmap/mmap_reader.go b/mmap/mmap_reader.go index af78f37..916a760 100644 --- a/mmap/mmap_reader.go +++ b/mmap/mmap_reader.go @@ -12,7 +12,9 @@ type Reader struct { // File returns the descriptor the mapping was created from, or nil once the reader is closed. The // reader owns that descriptor for its whole lifecycle: Close removes the mapping and then closes -// it, so a caller that still needs the descriptor keeps the reader open until it is done. +// it, so a caller that still needs the descriptor keeps the reader open until it is done. It must +// also keep the reader reachable, because the finalizer closes the descriptor once the reader is +// unreachable, and holding only the returned *os.File does not keep the reader alive. func (r *ReaderAt) File() *os.File { return r.file } diff --git a/signature_xattr_copy_test.go b/signature_xattr_copy_test.go index 156e8cd..ccd3ce8 100644 --- a/signature_xattr_copy_test.go +++ b/signature_xattr_copy_test.go @@ -4,42 +4,105 @@ package acp import ( + "context" "crypto/sha256" "os" "path/filepath" + "runtime" "testing" + + "golang.org/x/sys/unix" ) +// controlXattrName is an ordinary attribute, spelled in the platform's own namespace, that the +// metadata restore must carry onto a target. +var controlXattrName = func() string { + if runtime.GOOS == "darwin" { + return "acp.copy-control" + } + return "user.acp.copy-control" +}() + +// TestManagedSignatureIsNotOrdinaryCopiedXattr pins the metadata rule on the copy path itself: a +// transfer restores the source's ordinary xattrs onto the target, but the managed signature cache +// key is not one of them. The source carries a decoy entry no run writes for this content, so a +// target that ends up with it would prove the key travelled as an ordinary attribute, and the +// control attribute proves the restore ran at all instead of never happening. func TestManagedSignatureIsNotOrdinaryCopiedXattr(t *testing.T) { - // Seed ACP's managed xattr on a regular source file. - path := filepath.Join(t.TempDir(), "source.txt") - if err := os.WriteFile(path, []byte("fixture"), 0o644); err != nil { + root := t.TempDir() + content := []byte("fixture") + source := filepath.Join(root, "source.txt") + if err := os.WriteFile(source, content, 0o644); err != nil { t.Fatal(err) } - info, err := os.Stat(path) + target := filepath.Join(root, "target.txt") + + // A value no run would publish for this file: any managed entry on the target is a copy. + decoy := CachedSignature{Size: 1, MtimeNS: 2, SHA256: sha256.Sum256([]byte("decoy"))} + file, err := os.Open(source) if err != nil { t.Fatal(err) } - file, err := os.Open(path) - if err != nil { - t.Fatal(err) - } - err = writeSignatureXattr(file, encodeCachedSignature(CachedSignature{ - Size: info.Size(), MtimeNS: info.ModTime().UnixNano(), SHA256: sha256.Sum256([]byte("fixture")), - })) + err = writeSignatureXattr(file, encodeCachedSignature(decoy)) _ = file.Close() if err != nil { t.Skipf("temporary filesystem does not support signature xattrs: %v", err) } + if err := unix.Setxattr(source, controlXattrName, []byte("carried"), 0); err != nil { + t.Skipf("temporary filesystem does not support ordinary xattrs: %v", err) + } - // The ordinary metadata reader must exclude every platform spelling of the key. - xattrs, err := readXattrs(path) + // The default policy is the sharpest case: it manages no cache at all, so the only way the + // target could receive a managed attribute is the ordinary metadata copy. + item := newFixtureItem(source, target) + if err := runFixture(context.Background(), newStreamFixture(item), []Item{item}); err != nil { + t.Fatalf("run error = %v", err) + } + if _, err := item.terminal(t); err != nil { + t.Fatalf("item failed: %v", err) + } + + // The ordinary attribute reached the target, so the metadata restore ran. + restored, err := readXattrs(target) if err != nil { t.Fatal(err) } - for _, xattr := range xattrs { - if signatureCacheKey(xattr.key) { - t.Fatalf("managed signature key %q was returned as an ordinary xattr", xattr.key) + carried := false + for _, xattr := range restored { + if xattr.key == controlXattrName { + carried = true } } + if !carried { + t.Fatalf("the copy did not restore the ordinary attribute %q: %v", controlXattrName, restored) + } + + // The managed key did not travel: the target has no managed attribute at all. + targetFile, err := os.Open(target) + if err != nil { + t.Fatal(err) + } + encoded, readErr := readSignatureXattr(targetFile) + _ = targetFile.Close() + if readErr == nil { + t.Fatalf("the target carries a managed signature attribute: %x", encoded) + } + if !signatureXattrIgnorable(readErr) { + t.Fatalf("read the target's managed attribute: %v", readErr) + } + + // The source keeps its own entry: the transfer neither moved nor removed it. + sourceFile, err := os.Open(source) + if err != nil { + t.Fatal(err) + } + kept, readErr := readSignatureXattr(sourceFile) + _ = sourceFile.Close() + if readErr != nil { + t.Fatalf("read the source's managed attribute: %v", readErr) + } + stored, err := DecodeCachedSignature(kept) + if err != nil || stored != decoy { + t.Fatalf("source managed attribute = %#v / %v, want the seeded decoy %#v", stored, err, decoy) + } }