From 3fd9b0e44bc62209ed9a602835354a85b9b6e736 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Sun, 6 Sep 2026 14:04:37 +0200 Subject: [PATCH] fix: every file is written under a name nothing else holds Files go to a temporary name and are renamed into place. Three of those temporary names were created in a way that follows a link, so a link left at one of them sent the bytes outside the output directory while the run reported success. Reproduced against a build of the previous tree, in a scratch directory: a link at .tfg-writing the manifest landed on a file outside the output directory, exit 0, and then verify said "matches" and cleanup said "2 files removed", both exit 0 a link at .tfg-writing recipe fmt -w wrote the recipe onto another file and left the recipe as a link, exit 0 a link at an empty file appeared outside the output directory, exit 0 The first needs no race and no guessing: the name is fixed and nothing looked at it. On Windows none of it needs a privilege, because a hard link is enough - which is also why os.Lstat alone does not close it, since a hard link is an ordinary file to every question but the create itself. core.CreateNew is now the one door. It creates exclusively and believes a refusal only when os.Lstat finds an entry, so O_EXCL closes the hard link and Lstat closes the link that points at nothing. The fallback that exists because O_EXCL misreports a path running through a reparse point on Windows is unchanged, and an output directory reached through a link still works. Measured: 23 formats byte for byte identical, 48 refusals character for character, and no measurable cost on the write path - txt 4 kB x2000 median 2170 to 2143 ms and png 200 kB x240 866 to 849 ms, ranges overlapping with a canary of 27% and 19%. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 26 +++ internal/core/createnew.go | 90 ++++++++ internal/core/replace.go | 33 ++- internal/engine/parallel.go | 38 ++-- internal/guard/writeescape_test.go | 324 +++++++++++++++++++++++++++++ internal/manifest/manifest.go | 49 ++--- 6 files changed, 508 insertions(+), 52 deletions(-) create mode 100644 internal/core/createnew.go create mode 100644 internal/guard/writeescape_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ad52c9..832c462 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,32 @@ because it turns other people's test suites red. same new code and were checked against their recorded hashes and across every format at five sizes and two seeds. +### Security + +- **A file is never written under a name something else already holds.** Every + file this tool writes goes to a temporary name first and is renamed into + place. Three of those temporary names were created in a way that follows a + link, so a link left at one of them by somebody else sent the bytes wherever + it pointed - outside the directory you gave - and the run still reported + success. + + Reproduced against the previous build. A link at the manifest's temporary name + put the manifest onto a file outside the output directory and exited 0, after + which `verify` called that run a match and `cleanup` reported it removed. The + same shape made `recipe fmt -w` write your recipe onto somebody else's file + and leave your recipe itself as a link. On Windows none of this needs a + privilege, because a hard link is enough. + + Every one of those names is now claimed rather than created, and a name + something else holds is a refusal that says which name and what to do. + Pointing `--out` at a directory reached through a link keeps working, which is + the setup this was measured against. + + **What changes for an ordinary run: nothing.** The one case you can meet + without somebody working against you is a leftover `.tfg-writing` file from a + run that was killed part way through. That used to be written over in silence. + It is now a refusal naming the file, so remove it and run again. + ### Changed - **Files are written over several threads, so a run of many files is several diff --git a/internal/core/createnew.go b/internal/core/createnew.go new file mode 100644 index 0000000..bd4b1e5 --- /dev/null +++ b/internal/core/createnew.go @@ -0,0 +1,90 @@ +package core + +import ( + "errors" + "io/fs" + "os" +) + +// CreateNew creates a file under a name nobody is holding, and refuses when +// something already is. +// +// Every file this tool writes goes through here, and there is one reason for +// that rather than a preference for tidiness. A create that is not exclusive +// follows whatever the name points at, so a name somebody else put there first +// decides where the bytes land. Measured on 2026-09-06 against the shipped +// binary, in a scratch directory outside the repository: +// +// a link at .tfg-writing the manifest landed on the file the +// link pointed at, outside the output +// directory, and the run exited 0 +// a link at .tfg-writing "recipe fmt -w" wrote the recipe onto +// somebody else's file and left the +// recipe itself as a link, exit 0 +// +// Neither name was checked anywhere, because the checks that exist are about +// the file a run produces and the manifest it records - and these two are the +// names those files are written under before they are renamed into place. +// +// THE CHECK CANNOT BE A LOOK BEFORE THE WRITE, and that is what makes this a +// create rather than a question. os.Stat follows a link, so a link pointing at +// nothing answers "there is nothing here" - and a hard link is not a link at +// all as far as any question goes: os.Lstat reports it as an ordinary file, +// because that is what it is. On Windows an ordinary user creates one without +// any privilege, which is measured rather than read. So the only answer that +// holds is the one the operating system settles while it creates the file. +// +// O_EXCL IS NOT RELIABLE EVERYWHERE, and that was measured too, on 2026-08-03 +// and again on 2026-08-25. On Windows, Go asks for the reparse point rather +// than for what it points at when O_EXCL is set, and the create then reports +// "the file exists" about a file that is not there whenever any part of the +// path is a symbolic link or a junction. A directory reached through a link is +// an ordinary setup - a redirected workspace, a mounted scratch disk - and this +// tool supports it on purpose. +// +// So a refusal is believed only when something really is there, and the +// question that settles it is os.Lstat rather than os.Stat: a link pointing at +// nothing is a name being taken, whatever it points at. Where O_EXCL works this +// is exactly O_EXCL. Where it lies, this is what the tool did before it, and +// what is left is the window between the two calls - narrow, on that one +// platform, and smaller than the whole of the door it replaces. +func CreateNew(path string, perm os.FileMode) (*os.File, error) { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, perm) + if err == nil { + return f, nil + } + + _, lookErr := os.Lstat(path) + if lookErr == nil { + // Something is genuinely there. This is the refusal that matters, and + // it is the one the escapes above went round. + return nil, &NameTakenError{Path: path, Err: err} + } + if !errors.Is(lookErr, fs.ErrNotExist) { + // A name we cannot ask about is not a name we may write over. Reported + // as the create failed rather than as the look did, because the create + // is what the caller asked for. + return nil, err + } + + return os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm) +} + +// NameTakenError is refusing to write under a name something else is holding. +// +// It carries the create's own error so that a caller can still ask +// errors.Is(err, fs.ErrExist) and give the refusal its own words - which the +// engine does, because "this run will not write over it" is a better sentence +// about a generated file than anything a general purpose helper could write. +type NameTakenError struct { + Path string + Err error +} + +func (e *NameTakenError) Error() string { + return "the name " + e.Path + " is already in use, so nothing was written. " + + "This tool writes under a temporary name and renames it into place, and it never writes over a name somebody else holds. " + + "Remove what is at that name, or work in a directory nothing else is writing to" +} + +func (e *NameTakenError) Unwrap() error { return e.Err } diff --git a/internal/core/replace.go b/internal/core/replace.go index 281c5b6..afacace 100644 --- a/internal/core/replace.go +++ b/internal/core/replace.go @@ -1,6 +1,9 @@ package core -import "os" +import ( + "errors" + "os" +) // writingSuffix marks the half written copy while it is being filled. // @@ -47,7 +50,14 @@ func ReplaceFile(path string, content []byte) error { tmp := path + writingSuffix if err := writeWhole(tmp, content, mode); err != nil { - _ = os.Remove(tmp) + // Only what this call created is taken away. A refusal from CreateNew + // means the name was already somebody's - a leftover from an + // interrupted run, or something planted there - and untouchable rule 7 + // is that this tool does not remove what it did not write. + var taken *NameTakenError + if !errors.As(err, &taken) { + _ = os.Remove(tmp) + } return err } if err := os.Rename(tmp, path); err != nil { @@ -83,13 +93,20 @@ func modeToKeep(path string) (os.FileMode, error) { // writeWhole fills the copy and makes sure it carries the mode it was given. // -// The mode is set explicitly rather than left to the create call, for two -// reasons that both bite quietly: a create only applies its mode when the file -// is new, so a leftover copy from an interrupted run would keep whatever it -// had, and the process umask takes bits away from a create and not from a -// chmod. +// The mode is set explicitly rather than left to the create call, because the +// process umask takes bits away from a create and not from a chmod. +// +// A second reason stood here until 2026-09-06 and it went with the create it +// described: a create only applies its mode when the file is new, so a leftover +// copy from an interrupted run used to keep whatever mode it had. CreateNew +// refuses a name something is already holding, so there is no leftover to +// inherit a mode from - there is a refusal naming the file instead. That +// changed because this name is beside a file in somebody's repository, and a +// create that is not exclusive wrote through a link planted at it. Measured on +// 2026-09-06: "recipe fmt -w" put the recipe on a file outside the directory +// and left the recipe itself as a link, exit 0. func writeWhole(path string, content []byte, mode os.FileMode) error { - f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode) + f, err := CreateNew(path, mode) if err != nil { return err } diff --git a/internal/engine/parallel.go b/internal/engine/parallel.go index 2d2116c..edbb867 100644 --- a/internal/engine/parallel.go +++ b/internal/engine/parallel.go @@ -6,13 +6,17 @@ import ( "context" "crypto/sha256" "encoding/hex" + "errors" "fmt" "io" + "io/fs" "os" "path/filepath" "runtime" "sync" "sync/atomic" + + "github.com/donislawdev/TestingFilesGenerator/internal/core" ) // This file is the only place in internal/engine that runs anything beside @@ -289,12 +293,11 @@ func writeOne(ctx context.Context, f PlannedFile, outDir string, p *fileProgress // files that want the same name and this name is built from that one. tmp := tempPathFor(outDir, f.Name) - // os.Create, and O_EXCL was tried here and taken back out on 2026-08-25. + // Claimed rather than created, and core.CreateNew carries the measurement + // that settles how. // - // The idea was sound: the check in preflight answers "this name is free" - // a few hundred lines before the write, and O_EXCL would have the - // filesystem answer it at the moment of writing instead. What it costs on - // Windows is not sound. Measured with a probe, a file created in a + // O_EXCL on its own was tried here and taken back out on 2026-08-25, for a + // reason that has not changed. Measured with a probe, a file created in a // directory reached through a symbolic link: // // os.Create works @@ -302,17 +305,28 @@ func writeOne(ctx context.Context, f PlannedFile, outDir string, p *fileProgress // // about a file that does not exist. Go asks for the reparse point rather // than what it points at when O_EXCL is set, so every file of a run whose - // output directory is a link fails - and this tool supports exactly that + // output directory is a link failed - and this tool supports exactly that // on purpose, because people keep fixtures on a mounted workspace or a // scratch disk. Two guards said so within a minute of the change. // - // The window O_EXCL would have closed is a real one and it is small: - // preflight refuses every name that is taken before the run starts, so - // what is left is somebody else creating our temporary name, with our - // process id in it, during the run. Trading a supported way of pointing - // the tool at a directory for that is the wrong way round. - fh, err := os.Create(tmp) + // What came back on 2026-09-06 is not that flag on its own. It is the + // pair: create exclusively, and believe the refusal only when os.Lstat + // says something is really there. The supported setup keeps working, and + // the window this file used to leave open closes with it. + // + // That window is small and it was the last one of its kind: preflight + // refuses every name that is taken before the run starts, so what was left + // is somebody creating our temporary name - with our process id in it - + // during the run, and a create that follows links putting the bytes + // wherever it pointed. Owner's call on 2026-09-06, after the same class + // was found unguarded in two other places. See core.CreateNew. + fh, err := core.CreateNew(tmp, 0o666) if err != nil { + if errors.Is(err, fs.ErrExist) { + // In our own words. The fault is the one preflight names, arriving + // later than preflight can look. + return "", &CollisionError{Path: tmp} + } return "", err } diff --git a/internal/guard/writeescape_test.go b/internal/guard/writeescape_test.go new file mode 100644 index 0000000..983dc45 --- /dev/null +++ b/internal/guard/writeescape_test.go @@ -0,0 +1,324 @@ +package guard + +import ( + "errors" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/core" + "github.com/donislawdev/TestingFilesGenerator/internal/manifest" +) + +// Every file this tool writes is created under a name nobody else holds. +// +// This is one rule that was spelled in four places and complete in none of +// them, and what that cost was measured on 2026-09-06 against the shipped +// binary, in a scratch directory outside the repository: +// +// a link at .tfg-writing the manifest landed on a file outside +// the output directory, exit 0, and then +// verify said "matches" and cleanup said +// "2 files removed" - both exit 0 +// a link at .tfg-writing "recipe fmt -w" wrote the recipe onto +// somebody else's file and left the +// recipe itself as a link, exit 0 +// a link at an empty file appeared outside the +// output directory, exit 0 +// +// The first of those needs no race and no guessing: the name is fixed, and +// nothing anywhere looked at it. On Windows it needs no privilege either, +// because a HARD link is enough and an ordinary user creates one - measured on +// this machine, the victim file came back holding the manifest. +// +// SECURITY.md puts "a way to make the tool write outside the directory it was +// given" first in scope, and says in its own words that a path leaving the +// directory through a symbolic link is refused. It was true of the two reading +// commands and of the files a run produces. It was not true of the names those +// files are written under first. +// +// So the rule became one function, and this asks that it stays one. A fifth +// writer added next year inherits the answer instead of having to know the +// question. +func TestEveryFileThisToolWritesIsCreatedThroughOneClaim(t *testing.T) { + // Every place allowed to create a file for itself, and why. An entry that + // stops naming real code is a failure below, not a comment nobody reads. + allowed := map[string]string{ + "internal/core/createnew.go": "the one claim - this is where the rule lives", + "internal/legal/cmd/sbom/main.go": "a development command that neither shipped binary links, " + + "measured with go list -deps: zero", + } + + // The calls that bring a file into being. os.Open and os.ReadFile are not + // here on purpose: reading through somebody's link is a different question, + // and the two reading commands already answer it with core.Boundary. + creators := map[string]bool{"Create": true, "WriteFile": true, "OpenFile": true} + + seen := map[string]bool{} + root := repoRoot(t) + + for _, start := range []string{filepath.Join(root, "internal"), filepath.Join(root, "cmd")} { + err := filepath.WalkDir(start, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + rel := filepath.ToSlash(strings.TrimPrefix(path, root+string(filepath.Separator))) + if d.IsDir() { + // The guards themselves plant files to test with, and two + // packages exist only for guards to reach - neither is in a + // binary anybody downloads. + switch rel { + case "internal/guard", "internal/oracle", "internal/site": + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + + fset := token.NewFileSet() + file, perr := parser.ParseFile(fset, path, nil, 0) + if perr != nil { + return perr + } + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + pkg, ok := sel.X.(*ast.Ident) + if !ok || pkg.Name != "os" || !creators[sel.Sel.Name] { + return true + } + if _, granted := allowed[rel]; granted { + seen[rel] = true + return true + } + t.Errorf("%s:%d creates a file with os.%s.\n"+ + "Files are created through core.CreateNew, which refuses a name something else "+ + "already holds - a leftover, a symbolic link, or a hard link somebody planted. "+ + "A create that is not exclusive follows whatever is at the name, and on "+ + "2026-09-06 three of them did exactly that and wrote outside the output "+ + "directory with exit 0. Use core.CreateNew, or add this file to the allowed "+ + "map above with the reason.", + rel, fset.Position(call.Pos()).Line, sel.Sel.Name) + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walking %s: %v", start, err) + } + } + + // An exception that outlived its code is an exception nobody granted. + for rel, why := range allowed { + if !seen[rel] { + t.Errorf("%s is listed as allowed to create a file (%s), but it does not create one.\n"+ + "Delete the entry. A standing exception for code that has gone will quietly cover "+ + "the next thing that lands in that file.", rel, why) + } + } +} + +// core.CreateNew creates only when the name is free, and what it refuses covers +// every way a name can be held. +// +// The three shapes matter for different reasons and no one of them proves the +// others: +// +// a plain file O_EXCL alone answers this +// a hard link O_EXCL answers it, and NOTHING ELSE CAN - os.Lstat +// reports a hard link as an ordinary file, because that +// is what it is. This is the shape that needs no +// privilege on Windows +// a link to nothing O_EXCL says "it exists" and the fallback has to agree. +// os.Stat follows the link and says the name is free, +// which is exactly how the manifest escaped +// +// The last one is why the fallback asks os.Lstat. The fallback exists because +// O_EXCL lies on Windows when the path runs through a reparse point - measured +// 2026-08-03 - so it cannot simply be taken away. +func TestCreateNewCreatesOnlyWhenTheNameIsFree(t *testing.T) { + t.Run("a free name is created", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "fresh.txt") + f, err := core.CreateNew(path, 0o644) + if err != nil { + t.Fatalf("a free name was refused: %v", err) + } + if _, err := f.WriteString("ours"); err != nil { + t.Fatalf("writing: %v", err) + } + if err := f.Close(); err != nil { + t.Fatalf("closing: %v", err) + } + got, err := os.ReadFile(path) + if err != nil || string(got) != "ours" { + t.Fatalf("the file did not come back as it was written: %q, %v", got, err) + } + }) + + held := []struct { + what string + plant func(t *testing.T, dir, name, victim string) + }{ + {"a plain file", func(t *testing.T, dir, name, victim string) { + if err := os.WriteFile(name, []byte("SOMEBODY ELSE"), 0o644); err != nil { + t.Fatalf("planting a file: %v", err) + } + }}, + {"a hard link to a file outside the directory", func(t *testing.T, dir, name, victim string) { + if err := os.Link(victim, name); err != nil { + t.Fatalf("planting a hard link: %v", err) + } + }}, + {"a symbolic link to a file outside the directory", func(t *testing.T, dir, name, victim string) { + if err := os.Symlink(victim, name); err != nil { + skipIfLinksAreNotAllowed(t, err) + } + }}, + {"a symbolic link to nothing at all", func(t *testing.T, dir, name, victim string) { + if err := os.Symlink(filepath.Join(dir, "nothing-is-here"), name); err != nil { + skipIfLinksAreNotAllowed(t, err) + } + }}, + } + + for _, c := range held { + t.Run(c.what, func(t *testing.T) { + dir := t.TempDir() + victim := filepath.Join(t.TempDir(), "victim.txt") + if err := os.WriteFile(victim, []byte("ORIGINAL"), 0o644); err != nil { + t.Fatalf("writing the victim: %v", err) + } + name := filepath.Join(dir, "taken.txt") + c.plant(t, dir, name, victim) + + f, err := core.CreateNew(name, 0o644) + if err == nil { + _ = f.Close() + t.Fatalf("%s was written through rather than refused", c.what) + } + if !errors.Is(err, fs.ErrExist) { + t.Errorf("the refusal for %s does not read as \"already there\": %v.\n"+ + "Callers tell this apart from a disk failure with errors.Is(err, fs.ErrExist), "+ + "and the engine turns it into its own wording that way.", c.what, err) + } + if got, rerr := os.ReadFile(victim); rerr != nil || string(got) != "ORIGINAL" { + t.Errorf("the file outside the directory changed: %q, %v", got, rerr) + } + // What somebody else put there stays there. Untouchable rule 7 is + // that this tool removes only what a manifest lists, and a refusal + // is not a licence to tidy. + if _, lerr := os.Lstat(name); lerr != nil { + t.Errorf("%s was removed by the refusal: %v", c.what, lerr) + } + }) + } +} + +// The two writers that put a file beside somebody else's refuse a held +// temporary name rather than writing through it. +// +// Asked through the packages rather than of core.CreateNew again, because what +// broke was not the primitive - it did not exist. What broke is that these two +// call sites did their own create. A guard on the primitive alone would stay +// green if either of them stopped calling it. +func TestAHeldTemporaryNameStopsTheWriteRatherThanGoingThroughIt(t *testing.T) { + t.Run("the manifest", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "manifest.json") + victim := filepath.Join(t.TempDir(), "notes.txt") + if err := os.WriteFile(victim, []byte("ORIGINAL"), 0o644); err != nil { + t.Fatalf("writing the victim: %v", err) + } + if err := os.Link(victim, path+".tfg-writing"); err != nil { + t.Fatalf("planting a hard link: %v", err) + } + + m := manifest.New("testing-files-generator", "0.0.0-test", "run_x", "tfg generate", 1, "windows", "amd64") + m.Add(manifest.File{ID: "files", Path: "files_0001.txt", Name: "files_0001.txt", Bytes: 1024}) + if err := m.Save(path); err == nil { + t.Fatal("the manifest was saved through a name somebody else held") + } + if got, err := os.ReadFile(victim); err != nil || string(got) != "ORIGINAL" { + t.Errorf("the manifest landed on a file outside the directory: %q, %v", got, err) + } + }) + + t.Run("the manifest name itself, held by a link to nothing", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "manifest.json") + target := filepath.Join(t.TempDir(), "created-by-escape.json") + if err := os.Symlink(target, path); err != nil { + skipIfLinksAreNotAllowed(t, err) + } + + if err := manifest.Claim(path); err == nil { + t.Fatal("the name was claimed through a link pointing at nothing") + } + if _, err := os.Stat(target); err == nil { + t.Error("a file was created outside the directory. os.Stat follows a link, so a link " + + "pointing at nothing answers \"the name is free\" - the claim has to ask os.Lstat") + } + }) + + t.Run("recipe fmt -w", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "r.yaml") + if err := os.WriteFile(path, []byte("version: 1\n"), 0o644); err != nil { + t.Fatalf("writing the recipe: %v", err) + } + victim := filepath.Join(t.TempDir(), "passwd-ish.txt") + if err := os.WriteFile(victim, []byte("ORIGINAL"), 0o644); err != nil { + t.Fatalf("writing the victim: %v", err) + } + if err := os.Link(victim, path+".tfg-writing"); err != nil { + t.Fatalf("planting a hard link: %v", err) + } + + if err := core.ReplaceFile(path, []byte("version: 1\nreplaced: true\n")); err == nil { + t.Fatal("the recipe was replaced through a name somebody else held") + } + if got, err := os.ReadFile(victim); err != nil || string(got) != "ORIGINAL" { + t.Errorf("the recipe landed on a file outside the directory: %q, %v", got, err) + } + // The recipe is still the file it was, rather than a link to the + // victim. That is what the rename did with the planted link on + // 2026-09-06, and it is the half that loses the user's own work. + info, err := os.Lstat(path) + if err != nil || info.Mode()&os.ModeSymlink != 0 { + t.Errorf("the recipe is no longer an ordinary file of its own: %v, %v", info, err) + } + if got, err := os.ReadFile(path); err != nil || string(got) != "version: 1\n" { + t.Errorf("the recipe changed even though the write was refused: %q, %v", got, err) + } + }) +} + +// skipIfLinksAreNotAllowed says out loud when a case did not run. +// +// Creating a symbolic link needs a privilege on Windows that an ordinary +// account does not have, and a case that quietly passes because it never ran is +// the failure this project has recorded more than any other. The hard link +// cases above need no privilege anywhere, so the shape that matters most is +// never the one being skipped. +func skipIfLinksAreNotAllowed(t *testing.T, err error) { + t.Helper() + if errors.Is(err, fs.ErrPermission) || strings.Contains(err.Error(), "privilege") { + t.Skipf("this host does not allow creating a symbolic link (%v), so this case did not run. "+ + "The hard link cases beside it did.", err) + } + t.Fatalf("planting a symbolic link: %v", err) +} diff --git a/internal/manifest/manifest.go b/internal/manifest/manifest.go index be81faf..004aff2 100644 --- a/internal/manifest/manifest.go +++ b/internal/manifest/manifest.go @@ -599,7 +599,12 @@ func (m *Manifest) Save(path string) error { // half applied. func (m *Manifest) writeOver(path string) error { tmp := path + ".tfg-writing" - f, err := os.Create(tmp) + // Claimed rather than created, and core.CreateNew says why: this name sits + // in a directory the run does not own, nothing else in the tool checks it, + // and a create that is not exclusive follows whatever is at the name. + // Measured on 2026-09-06 - a link here put the manifest on a file outside + // the output directory and the run still exited 0. + f, err := core.CreateNew(tmp, 0o666) if err != nil { return err } @@ -638,40 +643,20 @@ func (m *Manifest) writeOver(path string) error { // claimName creates the file only if nobody else holds the name. // -// O_EXCL is the way to ask that question, because creating the file and finding -// out whether it existed are then one operation nobody can get between. +// How that question is settled, and why creating the file is the only way to +// ask it, is in core.CreateNew - together with the measurement of what Windows +// answers when the path runs through a reparse point. // -// It is not reliable everywhere, and that was measured rather than read. -// On Windows, Go opens with the flag that stops a create from following a link, -// and CREATE_NEW then reports ERROR_ALREADY_EXISTS for a file that is not there -// whenever any part of the path is a reparse point - a symbolic link or a -// junction. Measured on 2026-08-03 against a directory made three ways: +// It moved there on 2026-09-06. This function had the better half of the answer +// and asked os.Stat, which follows a link: a link pointing at nothing answered +// "there is nothing here" and the create went through it. The temporary name +// beside this one had no half of the answer at all. One rule spelled in two +// places is one rule with a hole in it, so now there is one place. // -// plain directory O_EXCL succeeds -// symbolic link O_EXCL says "The file exists" and nothing is there -// junction the same -// -// It is Go rather than the system: the same create through the same link -// succeeds from .NET. A directory reached through a link is an ordinary setup - -// a redirected workspace, a mounted scratch disk - so taking the answer at face -// value made "tfg generate --out" fail with exit code 5 for those users. That -// was introduced on 2026-08-03 and found the same day by the guard that -// generates into a linked directory. -// -// So a refusal is believed only when something really is there. Where O_EXCL -// works this is exactly O_EXCL. Where it lies, this falls back to what the tool -// did before, which leaves the same narrow window two runs starting together -// could meet - see O43. +// What was measured here stays true of the fallback: it leaves the narrow +// window two runs starting together could meet - see O43. func claimName(path string) error { - f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) - if err == nil { - return f.Close() - } - if _, statErr := os.Stat(path); statErr == nil { - // Something is genuinely there. This is the refusal that matters. - return err - } - f, err = os.Create(path) + f, err := core.CreateNew(path, 0o644) if err != nil { return err }