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.
This commit is contained in:
Samuel Cui committed 2026-09-19 18:31:29 +08:00
1 parent e5d9150b3d
commit 5be0952efa
4 files changed
+98 -27

No files matched your search

+9 -5
View File
@@ -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
+7 -5
View File
@@ -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
+3 -1
View File
@@ -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
}
+79 -16
View File
@@ -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)
}
}