mirror of
https://github.com/lima-vm/lima.git
synced 2026-10-28 11:44:35 +08:00
fix(store): write the boot session marker atomically
os.WriteFile truncates the boot session marker in place before rewriting it. A concurrent staleBootSession can read it empty in that window, decide it does not match the current boot ID, and have WritePIDFile delete PID files that belong to the running boot. Write the marker through a new fsutil.WriteFileAtomic helper that writes a temporary file in the same directory and renames it into place, so a reader always sees a complete marker. writePIDFileAtomically now uses the same helper instead of its own temp-and-rename. The temp name is a short fixed prefix, not the destination basename, so a long custom PID path cannot overflow the filesystem name limit. os.CreateTemp creates it 0600, so the marker and PID files are now 0600 masked by the umask rather than the previous 0644, and a rewrite no longer preserves an existing file's mode. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
This commit is contained in:
1 parent
f93c1b316c
commit
fcde1028b9
5 files changed
+247
-10
No files matched your search
@@ -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)
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
+4
-10
@@ -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 {
|
||||
|
||||
Reference in new issue
Block a user