storage: fsync staging directory before atomic rename

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)
 }
 

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
+}

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)
+	}
+}