From 1e6233026577c059154b30f87af2742673ad6708 Mon Sep 17 00:00:00 2001 From: Aditi Sahay Date: Wed, 17 Jun 2026 17:51:00 +0530 Subject: [PATCH] storage: fsync staging directory before atomic rename Signed-off-by: Aditi Sahay Co-authored-by: Cursor --- storage/drivers/overlay/overlay.go | 5 ++ storage/pkg/ioutils/sync_directory_linux.go | 78 +++++++++++++++++++ .../pkg/ioutils/sync_directory_linux_test.go | 66 ++++++++++++++++ 3 files changed, 149 insertions(+) create mode 100644 storage/pkg/ioutils/sync_directory_linux.go create mode 100644 storage/pkg/ioutils/sync_directory_linux_test.go diff --git a/storage/drivers/overlay/overlay.go b/storage/drivers/overlay/overlay.go index cd438e00c2..9eb220be7c 100644 --- a/storage/drivers/overlay/overlay.go +++ b/storage/drivers/overlay/overlay.go @@ -36,6 +36,7 @@ import ( "go.podman.io/storage/pkg/directory" "go.podman.io/storage/pkg/fileutils" "go.podman.io/storage/pkg/fsutils" + "go.podman.io/storage/pkg/ioutils" "go.podman.io/storage/pkg/idmap" "go.podman.io/storage/pkg/idtools" "go.podman.io/storage/pkg/mount" @@ -2310,6 +2311,10 @@ func (d *Driver) ApplyDiffFromStagingDirectory(id, parent string, diffOutput *gr return err } + if err := ioutils.SyncDirectoryContents(stagingDirectory); err != nil { + return fmt.Errorf("sync staging directory before rename: %w", err) + } + return os.Rename(stagingDirectory, diffPath) } diff --git a/storage/pkg/ioutils/sync_directory_linux.go b/storage/pkg/ioutils/sync_directory_linux.go new file mode 100644 index 0000000000..3ac14520b4 --- /dev/null +++ b/storage/pkg/ioutils/sync_directory_linux.go @@ -0,0 +1,78 @@ +//go:build linux + +package ioutils + +import ( + "fmt" + "io/fs" + "os" + "path/filepath" + + "golang.org/x/sys/unix" +) + +// SyncDirectoryContents flushes file data and directory metadata under dir to +// physical storage. Call this before atomically renaming a fully populated +// staging directory to its final location. +func SyncDirectoryContents(dir string) error { + var dirs []string + + err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if d.IsDir() { + dirs = append(dirs, path) + return nil + } + + // Only regular files have data worth fdatasync-ing. Symlinks, + // device nodes, FIFOs, and sockets carry no separate file + // content: their state lives entirely in directory-entry / + // inode metadata, which is already covered once the parent + // directory is fsync'd below. Treating them like regular + // files is actively wrong: os.Open on a symlink dereferences + // it, so a symlink whose target does not happen to resolve + // from inside the staging tree (e.g. Debian's + // /etc/alternatives/*, or any link into a lower/not-yet- + // materialized layer) fails the whole sync with ENOENT, and + // opening a FIFO can block indefinitely waiting for a reader. + if !d.Type().IsRegular() { + return nil + } + + f, err := os.Open(path) + if err != nil { + return err + } + + syncErr := unix.Fdatasync(int(f.Fd())) + closeErr := f.Close() + if syncErr != nil { + return syncErr + } + + return closeErr + }) + if err != nil { + return fmt.Errorf("sync directory contents in %q: %w", dir, err) + } + + for i := len(dirs) - 1; i >= 0; i-- { + dfd, err := os.Open(dirs[i]) + if err != nil { + return fmt.Errorf("open directory %q for sync: %w", dirs[i], err) + } + + syncErr := unix.Fsync(int(dfd.Fd())) + closeErr := dfd.Close() + if syncErr != nil { + return fmt.Errorf("sync directory %q: %w", dirs[i], syncErr) + } + if closeErr != nil { + return fmt.Errorf("close directory %q after sync: %w", dirs[i], closeErr) + } + } + + return nil +} diff --git a/storage/pkg/ioutils/sync_directory_linux_test.go b/storage/pkg/ioutils/sync_directory_linux_test.go new file mode 100644 index 0000000000..cdf354205c --- /dev/null +++ b/storage/pkg/ioutils/sync_directory_linux_test.go @@ -0,0 +1,66 @@ +//go:build linux + +package ioutils + +import ( + "os" + "path/filepath" + "testing" +) + +func TestSyncDirectoryContents(t *testing.T) { + dir := t.TempDir() + + nested := filepath.Join(dir, "nested") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatalf("mkdir nested: %v", err) + } + + files := []string{ + filepath.Join(dir, "file1"), + filepath.Join(nested, "file2"), + } + for _, file := range files { + if err := os.WriteFile(file, []byte("storage-resilience"), 0o644); err != nil { + t.Fatalf("write file %q: %v", file, err) + } + } + + if err := SyncDirectoryContents(dir); err != nil { + t.Fatalf("SyncDirectoryContents: %v", err) + } +} + +// TestSyncDirectoryContentsDanglingSymlink reproduces the failure this test +// was written after observing in practice: pulling quay.io/crio/nginx +// through the partial-pull/staging path failed with +// +// sync directory contents in ".../overlay/staging/.../dir": open +// .../dir/etc/alternatives/awk.1.gz: no such file or directory +// +// Debian-based images populate /etc/alternatives with symlinks, some of +// which point outside the layer's own diff (e.g. into a lower layer, or a +// path never materialized in this staging directory at all). os.Open +// dereferences symlinks, so walking the tree and opening every non-dir +// entry -- including symlinks -- fails with ENOENT on any such link. A +// symlink has no file content of its own to fdatasync; its target string +// is directory-entry metadata covered by fsync-ing the parent directory. +func TestSyncDirectoryContentsDanglingSymlink(t *testing.T) { + dir := t.TempDir() + + if err := os.WriteFile(filepath.Join(dir, "real-file"), []byte("data"), 0o644); err != nil { + t.Fatalf("write real-file: %v", err) + } + + // Points at a target that does not exist anywhere on disk, exactly + // like a symlink whose target lives in a different layer than the + // one currently staged. + dangling := filepath.Join(dir, "awk.1.gz") + if err := os.Symlink("/nonexistent/mawk.1.gz", dangling); err != nil { + t.Fatalf("create dangling symlink: %v", err) + } + + if err := SyncDirectoryContents(dir); err != nil { + t.Fatalf("SyncDirectoryContents should skip symlinks instead of dereferencing them: %v", err) + } +}