diff --git a/pkg/fsutil/fsutil.go b/pkg/fsutil/fsutil.go new file mode 100644 index 00000000..436895fa --- /dev/null +++ b/pkg/fsutil/fsutil.go @@ -0,0 +1,29 @@ +// SPDX-FileCopyrightText: Copyright The Lima Authors +// SPDX-License-Identifier: Apache-2.0 + +package fsutil + +import ( + "os" + "path/filepath" +) + +// WriteFileAtomic writes data to a temp file in path's directory and renames it onto path, so a reader +// sees the old file or the whole new one, never a partial write; the rename is atomic on Unix only. +// os.CreateTemp requests 0600, so the result is 0600 masked by the umask (0600 under a typical umask). +func WriteFileAtomic(path string, data []byte) error { + tmp, err := os.CreateTemp(filepath.Dir(path), ".lima-atomic-*.tmp") + if err != nil { + return err + } + name := tmp.Name() + defer func() { _ = os.Remove(name) }() + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(name, path) +} diff --git a/pkg/fsutil/fsutil_atomic_test.go b/pkg/fsutil/fsutil_atomic_test.go new file mode 100644 index 00000000..72b1f9ad --- /dev/null +++ b/pkg/fsutil/fsutil_atomic_test.go @@ -0,0 +1,65 @@ +// SPDX-FileCopyrightText: Copyright The Lima Authors +// SPDX-License-Identifier: Apache-2.0 + +package fsutil + +import ( + "os" + "path/filepath" + "testing" + + "gotest.tools/v3/assert" +) + +func TestWriteFileAtomic(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "marker") + + // A new file is created with the given content. + assert.NilError(t, WriteFileAtomic(path, []byte("first\n"))) + got, err := os.ReadFile(path) + assert.NilError(t, err) + assert.Equal(t, string(got), "first\n") + + // Rewriting replaces the content in place and leaves no temporary file behind. + assert.NilError(t, WriteFileAtomic(path, []byte("second\n"))) + got, err = os.ReadFile(path) + assert.NilError(t, err) + assert.Equal(t, string(got), "second\n") + + entries, err := os.ReadDir(dir) + assert.NilError(t, err) + assert.Equal(t, len(entries), 1, "only the destination file should remain, with no leftover temp") + assert.Equal(t, entries[0].Name(), "marker") +} + +func TestWriteFileAtomicErrorLeavesNoTemp(t *testing.T) { + dir := t.TempDir() + // Make the parent of the destination a regular file, so creating the temp fails immediately; + // WriteFileAtomic must surface the error and not leave a temp file lying around. + notDir := filepath.Join(dir, "file") + assert.NilError(t, os.WriteFile(notDir, nil, 0o644)) + + err := WriteFileAtomic(filepath.Join(notDir, "child"), []byte("x")) + assert.Assert(t, err != nil, "writing under a non-directory should fail") + + entries, err := os.ReadDir(dir) + assert.NilError(t, err) + assert.Equal(t, len(entries), 1, "no temp file should be created under the destination's parent") +} + +func TestWriteFileAtomicCleansUpAfterRenameFailure(t *testing.T) { + dir := t.TempDir() + // The destination is an existing directory, so the temp file is created and written and only the + // rename fails, exercising the deferred cleanup that the immediate-failure test above never reaches. + dst := filepath.Join(dir, "dst") + assert.NilError(t, os.Mkdir(dst, 0o700)) + + err := WriteFileAtomic(dst, []byte("x")) + assert.Assert(t, err != nil, "renaming onto an existing directory should fail") + + entries, err := os.ReadDir(dir) + assert.NilError(t, err) + assert.Equal(t, len(entries), 1, "only the destination directory should remain, with no leftover temp") + assert.Equal(t, entries[0].Name(), "dst") +} diff --git a/pkg/fsutil/fsutil_atomic_unix_test.go b/pkg/fsutil/fsutil_atomic_unix_test.go new file mode 100644 index 00000000..f9896486 --- /dev/null +++ b/pkg/fsutil/fsutil_atomic_unix_test.go @@ -0,0 +1,84 @@ +//go:build unix + +// SPDX-FileCopyrightText: Copyright The Lima Authors +// SPDX-License-Identifier: Apache-2.0 + +package fsutil + +import ( + "os" + "path/filepath" + "strings" + "syscall" + "testing" + + "gotest.tools/v3/assert" +) + +// TestWriteFileAtomicModeUnderUmask records that the 0600 os.CreateTemp requests is masked by the +// process umask. umask is process-global, so the test restores it and must not run in parallel. +func TestWriteFileAtomicModeUnderUmask(t *testing.T) { + saved := syscall.Umask(0) + t.Cleanup(func() { syscall.Umask(saved) }) + for _, tc := range []struct { + umask int + want os.FileMode + }{ + {0o022, 0o600}, + {0o077, 0o600}, + {0o400, 0o200}, + {0o200, 0o400}, + } { + // Create the dir under umask 0 (accessible for Stat and cleanup); apply the test umask only + // across the write itself. + dir := t.TempDir() + path := filepath.Join(dir, "marker") + syscall.Umask(tc.umask) + err := WriteFileAtomic(path, []byte("x")) + syscall.Umask(0) + assert.NilError(t, err) + fi, err := os.Stat(path) + assert.NilError(t, err) + assert.Equal(t, fi.Mode().Perm(), tc.want, "umask %#o", tc.umask) + } +} + +// TestWriteFileAtomicLongBasename checks that the temp file uses a short fixed prefix: a long but +// valid destination basename must not overflow the name limit through a basename-derived temp name. +func TestWriteFileAtomicLongBasename(t *testing.T) { + path := filepath.Join(t.TempDir(), strings.Repeat("a", 250)) + assert.NilError(t, WriteFileAtomic(path, []byte("x"))) + got, err := os.ReadFile(path) + assert.NilError(t, err) + assert.Equal(t, string(got), "x") +} + +// TestWriteFileAtomicOverwriteResetsMode records that overwriting an existing file gives it the mode +// os.CreateTemp requests masked by the umask, rather than preserving the old file's permission bits. +// It mirrors the new-file table against a pre-existing 0666 destination. umask is process-global, so +// the test restores it and must not run in parallel. +func TestWriteFileAtomicOverwriteResetsMode(t *testing.T) { + saved := syscall.Umask(0) + t.Cleanup(func() { syscall.Umask(saved) }) + for _, tc := range []struct { + umask int + want os.FileMode + }{ + {0o022, 0o600}, + {0o077, 0o600}, + {0o400, 0o200}, + {0o200, 0o400}, + } { + // Pre-create a 0666 destination under umask 0 so the broad bits stick, then overwrite it + // under the test umask. A rewrite that kept the old mode would leave 0666, not tc.want. + path := filepath.Join(t.TempDir(), "marker") + assert.NilError(t, os.WriteFile(path, []byte("old"), 0o666)) + syscall.Umask(tc.umask) + err := WriteFileAtomic(path, []byte("new")) + syscall.Umask(0) + assert.NilError(t, err) + fi, err := os.Stat(path) + assert.NilError(t, err) + assert.Equal(t, fi.Mode().Perm(), tc.want, "umask %#o", tc.umask) + } +} diff --git a/pkg/store/bootsession_marker_unix_test.go b/pkg/store/bootsession_marker_unix_test.go new file mode 100644 index 00000000..c031905c --- /dev/null +++ b/pkg/store/bootsession_marker_unix_test.go @@ -0,0 +1,65 @@ +//go:build !windows + +// SPDX-FileCopyrightText: Copyright The Lima Authors +// SPDX-License-Identifier: Apache-2.0 + +package store + +import ( + "sync" + "sync/atomic" + "testing" + + "gotest.tools/v3/assert" +) + +// TestWriteBootSessionMarkerAtomic has readers race a fixed number of concurrent rewrites of one +// marker with a constant boot ID. An in-place truncating write lets a reader catch the empty file +// and report a previous boot; the atomic rename never does. +func TestWriteBootSessionMarkerAtomic(t *testing.T) { + dir := t.TempDir() + const id = "01234567-89ab-cdef-0123-456789abcdef" + assert.NilError(t, writeBootSessionMarker(dir, id)) + + const writesPerWriter = 2000 + var writers, readers sync.WaitGroup + stop := make(chan struct{}) + var reads, falsePos atomic.Int64 + + for range 4 { + writers.Go(func() { + for range writesPerWriter { + if err := writeBootSessionMarker(dir, id); err != nil { + t.Errorf("writer: %v", err) + return + } + } + }) + } + for range 4 { + readers.Go(func() { + for { + select { + case <-stop: + return + default: + } + stale, err := staleBootSession(dir, id) + if err != nil { + t.Errorf("reader: %v", err) + return + } + reads.Add(1) + if stale { + falsePos.Add(1) + } + } + }) + } + writers.Wait() + close(stop) + readers.Wait() + + assert.Assert(t, reads.Load() > 0, "no reads happened") + assert.Equal(t, falsePos.Load(), int64(0), "staleBootSession reported a previous boot while the boot ID was constant") +} diff --git a/pkg/store/pidfile.go b/pkg/store/pidfile.go index 781b8d4e..f254cdb2 100644 --- a/pkg/store/pidfile.go +++ b/pkg/store/pidfile.go @@ -12,6 +12,7 @@ import ( "github.com/sirupsen/logrus" + "github.com/lima-vm/lima/v2/pkg/fsutil" "github.com/lima-vm/lima/v2/pkg/limatype/filenames" "github.com/lima-vm/lima/v2/pkg/osutil" ) @@ -70,15 +71,7 @@ func WritePIDFile(path string, pid int) error { // writePIDFileAtomically renames the PID file into place, so that a reader never sees a partially // written one. func writePIDFileAtomically(path string, pid int) error { - tmpPath := path + ".tmp" - if err := os.WriteFile(tmpPath, fmt.Appendf(nil, "%d\n", pid), 0o644); err != nil { - return err - } - if err := os.Rename(tmpPath, path); err != nil { - _ = os.Remove(tmpPath) - return err - } - return nil + return fsutil.WriteFileAtomic(path, fmt.Appendf(nil, "%d\n", pid)) } // pidFileFromPreviousBoot returns true when the PID file at path was written during a @@ -105,8 +98,9 @@ func staleBootSession(dir, current string) (bool, error) { return strings.TrimSpace(string(b)) != current, nil } +// writeBootSessionMarker writes the boot session marker atomically. func writeBootSessionMarker(dir, id string) error { - return os.WriteFile(filepath.Join(dir, filenames.BootSession), []byte(id+"\n"), 0o644) + return fsutil.WriteFileAtomic(filepath.Join(dir, filenames.BootSession), []byte(id+"\n")) } func removePIDFiles(dir string) error {