diff --git a/internal/xray/process.go b/internal/xray/process.go index 560b6ade1..bb056f647 100644 --- a/internal/xray/process.go +++ b/internal/xray/process.go @@ -526,7 +526,7 @@ func (p *process) Start() (err error) { if p.configPath != "" { configPath = p.configPath } - err = os.WriteFile(configPath, data, 0644) + err = writeFileAtomic(configPath, data, 0o600) if err != nil { return common.NewErrorf("Failed to write configuration file: %v", err) } @@ -546,6 +546,54 @@ func (p *process) Start() (err error) { return nil } +// writeFileAtomic writes data to path via a same-directory temp file that is +// permissioned, synced, and renamed into place, so a crash can never leave a +// partial config; the config holds credentials, hence the 0600 perm. After the +// rename the parent directory is fsynced to persist the directory entry. That +// final step is skipped on Windows, where directory fsync is unsupported and +// os.Rename already uses replace-existing semantics. +func writeFileAtomic(path string, data []byte, perm os.FileMode) (err error) { + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, ".config-*.tmp") + if err != nil { + return err + } + tmpPath := tmp.Name() + defer func() { + _ = tmp.Close() + if err != nil { + _ = os.Remove(tmpPath) + } + }() + if err = tmp.Chmod(perm); err != nil { + return err + } + if _, err = tmp.Write(data); err != nil { + return err + } + if err = tmp.Sync(); err != nil { + return err + } + if err = tmp.Close(); err != nil { + return err + } + if err = renameFile(tmpPath, path); err != nil { + return err + } + if runtime.GOOS == "windows" { + return nil + } + dirHandle, err := os.Open(dir) + if err != nil { + return err + } + err = dirHandle.Sync() + _ = dirHandle.Close() + return err +} + +var renameFile = os.Rename + func (p *process) startCommand(cmd *exec.Cmd) error { p.mu.Lock() p.cmd = cmd diff --git a/internal/xray/process_test.go b/internal/xray/process_test.go index 188030904..24314bf49 100644 --- a/internal/xray/process_test.go +++ b/internal/xray/process_test.go @@ -3,6 +3,7 @@ package xray import ( + "errors" "os" "os/exec" "os/signal" @@ -15,6 +16,52 @@ import ( "github.com/op/go-logging" ) +func TestWriteFileAtomicModeAndRenameFailure(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + if err := os.WriteFile(path, []byte("old"), 0o644); err != nil { + t.Fatalf("seed: %v", err) + } + if err := writeFileAtomic(path, []byte("new"), 0o600); err != nil { + t.Fatalf("writeFileAtomic: %v", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read: %v", err) + } + if string(data) != "new" { + t.Fatalf("content = %q, want new", data) + } + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if info.Mode().Perm() != 0o600 { + t.Fatalf("mode = %o, want 600", info.Mode().Perm()) + } + + originalRename := renameFile + renameFile = func(_, _ string) error { return errors.New("injected rename failure") } + t.Cleanup(func() { renameFile = originalRename }) + if err := writeFileAtomic(path, []byte("partial"), 0o600); err == nil { + t.Fatal("rename failure = nil") + } + data, err = os.ReadFile(path) + if err != nil { + t.Fatalf("read preserved file: %v", err) + } + if string(data) != "new" { + t.Fatalf("content after failed rename = %q, want committed content", data) + } + matches, err := filepath.Glob(filepath.Join(dir, ".config-*.tmp")) + if err != nil { + t.Fatalf("glob: %v", err) + } + if len(matches) != 0 { + t.Fatalf("temporary files leaked: %v", matches) + } +} + func TestStopWaitsForGracefulExit(t *testing.T) { initProcessTestLogger(t)