From d3e161df24e3d3ade4c740d5aa685d3f247679d3 Mon Sep 17 00:00:00 2001 From: Shubham Kumar Date: Mon, 2 Nov 2026 23:58:06 +0530 Subject: [PATCH] fix(initrd): make file mount merges atomic and reject conflicts MergeFileMountsIntoInitrd truncated the initrd at its trailer before writing any new record, so a failed merge left the archive without a trailer and restarting the same container failed too. Write the merged archive to a temporary file next to the initrd and rename it over the initrd on success, keeping the owner and mode of the initrd. Destinations were also not checked against the existing entries. A file below a regular file, or a file over a directory, merged successfully and the guest only failed at boot. Record the type of each entry while inspecting the archive and reject these destinations with a clear error. Symlinks are still accepted as parents, since they may point at a directory. Fixes: #1107 Signed-off-by: Shubham Kumar --- pkg/unikontainers/initrd/initrd.go | 118 ++++++++++++++----- pkg/unikontainers/initrd/initrd_test.go | 145 +++++++++++++++++++++++- 2 files changed, 233 insertions(+), 30 deletions(-) diff --git a/pkg/unikontainers/initrd/initrd.go b/pkg/unikontainers/initrd/initrd.go index c6ea5dd8b..48aafa80e 100644 --- a/pkg/unikontainers/initrd/initrd.go +++ b/pkg/unikontainers/initrd/initrd.go @@ -20,6 +20,7 @@ import ( "io" "os" "path" + "path/filepath" "strconv" "strings" "syscall" @@ -141,8 +142,9 @@ func CopyFileMountsToInitrd(oldInitrd string, mounts []specs.Mount) error { } // MergeFileMountsIntoInitrd adds regular-file bind mounts before the trailer -// of an uncompressed newc archive. The archive is fully parsed before the -// disposable initrd is updated in place. The caller should pass only bind mounts. +// of an uncompressed newc archive. The merged archive is written to a temporary +// file next to the initrd and replaces it only on success, so a failed merge +// leaves the initrd untouched. The caller should pass only bind mounts. func MergeFileMountsIntoInitrd(oldInitrd string, mounts []specs.Mount) (retErr error) { fileMounts, err := regularFileBindMounts(mounts) if err != nil { @@ -152,40 +154,77 @@ func MergeFileMountsIntoInitrd(oldInitrd string, mounts []specs.Mount) (retErr e return nil } - initrdFile, err := os.OpenFile(oldInitrd, os.O_RDWR, 0) + initrdFile, err := os.Open(oldInitrd) if err != nil { return fmt.Errorf("could not open %s: %w", oldInitrd, err) } + defer initrdFile.Close() + + trailerOffset, entries, inodes, err := inspectInitrd(initrdFile) + if err != nil { + return fmt.Errorf("could not parse %s: %w", oldInitrd, err) + } + + tmpFile, err := os.CreateTemp(filepath.Dir(oldInitrd), filepath.Base(oldInitrd)+".tmp-*") + if err != nil { + return fmt.Errorf("could not create temporary file for %s: %w", oldInitrd, err) + } defer func() { - if err := initrdFile.Close(); err != nil { - closeErr := fmt.Errorf("could not close %s: %w", oldInitrd, err) - if retErr == nil { - retErr = closeErr - } else { - retErr = errors.Join(retErr, closeErr) - } + if retErr != nil { + tmpFile.Close() + os.Remove(tmpFile.Name()) } }() - trailerOffset, existingNames, inodes, err := inspectInitrd(initrdFile) + if err := copyOwnerAndMode(initrdFile, tmpFile); err != nil { + return fmt.Errorf("could not copy owner and mode of %s: %w", oldInitrd, err) + } + if err := writeMergedInitrd(tmpFile, initrdFile, trailerOffset, fileMounts, entries, inodes); err != nil { + return err + } + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("could not close temporary file for %s: %w", oldInitrd, err) + } + if err := os.Rename(tmpFile.Name(), oldInitrd); err != nil { + return fmt.Errorf("could not replace %s: %w", oldInitrd, err) + } + + return nil +} + +func copyOwnerAndMode(src, dst *os.File) error { + info, err := src.Stat() if err != nil { - return fmt.Errorf("could not parse %s: %w", oldInitrd, err) + return err + } + fileInfo := info.Sys().(*syscall.Stat_t) + if err := dst.Chown(int(fileInfo.Uid), int(fileInfo.Gid)); err != nil { + return err } - if err := initrdFile.Truncate(trailerOffset); err != nil { - return fmt.Errorf("could not truncate %s: %w", oldInitrd, err) + return dst.Chmod(info.Mode().Perm()) +} + +// writeMergedInitrd copies the records of src up to its trailer into dst, then +// adds the file mounts and a new trailer. +func writeMergedInitrd(dst, src *os.File, trailerOffset int64, fileMounts []specs.Mount, entries map[string]cpio.FileMode, inodes *inodeAllocator) error { + if _, err := src.Seek(0, io.SeekStart); err != nil { + return fmt.Errorf("could not seek in %s: %w", src.Name(), err) } - if _, err := initrdFile.Seek(trailerOffset, io.SeekStart); err != nil { - return fmt.Errorf("could not seek in %s: %w", oldInitrd, err) + if _, err := io.CopyN(dst, src, trailerOffset); err != nil { + return fmt.Errorf("could not copy %s: %w", src.Name(), err) } - w := cpio.NewWriter(initrdFile) + w := cpio.NewWriter(dst) for _, mount := range fileMounts { archiveName, err := archivePath(mount.Destination) if err != nil { return err } lookupName := archiveLookupPath(archiveName) - if err := addMissingParents(w, lookupName, existingNames, inodes); err != nil { + if err := checkDestinationConflict(lookupName, entries); err != nil { + return fmt.Errorf("could not add mount destination %s to initrd: %w", mount.Destination, err) + } + if err := addMissingParents(w, lookupName, entries, inodes); err != nil { return err } inode, err := inodes.allocate() @@ -195,7 +234,7 @@ func MergeFileMountsIntoInitrd(oldInitrd string, mounts []specs.Mount) (retErr e if err := copyFileToInitrdWithInode(w, mount.Source, archiveName, inode); err != nil { return fmt.Errorf("could not add file %s to initrd: %w", mount.Source, err) } - existingNames[lookupName] = struct{}{} + entries[lookupName] = cpio.TypeReg } if err := w.Close(); err != nil { return fmt.Errorf("could not close initrd writer: %w", err) @@ -204,6 +243,33 @@ func MergeFileMountsIntoInitrd(oldInitrd string, mounts []specs.Mount) (retErr e return nil } +// checkDestinationConflict rejects a destination the guest could not unpack: a +// file replacing a directory, or a file below an entry that cannot hold it. The +// archive itself would stay valid, so the guest would only fail later at boot. +// A symlink may point at a directory, so it is accepted as a parent. +func checkDestinationConflict(lookupName string, entries map[string]cpio.FileMode) error { + if mode, exists := entries[lookupName]; exists && mode&cpio.ModeType == cpio.TypeDir { + return fmt.Errorf("%s is a directory in the initrd", lookupName) + } + + parentName := "" + for _, component := range strings.Split(path.Dir(lookupName), "/") { + if component == "" || component == "." { + continue + } + parentName = path.Join(parentName, component) + mode, exists := entries[parentName] + if !exists { + continue + } + if fileType := mode & cpio.ModeType; fileType != cpio.TypeDir && fileType != cpio.TypeSymlink { + return fmt.Errorf("%s is not a directory in the initrd", parentName) + } + } + + return nil +} + func regularFileBindMounts(mounts []specs.Mount) ([]specs.Mount, error) { var fileMounts []specs.Mount for _, mount := range mounts { @@ -218,13 +284,13 @@ func regularFileBindMounts(mounts []specs.Mount) ([]specs.Mount, error) { return fileMounts, nil } -func inspectInitrd(f *os.File) (int64, map[string]struct{}, *inodeAllocator, error) { +func inspectInitrd(f *os.File) (int64, map[string]cpio.FileMode, *inodeAllocator, error) { if _, err := f.Seek(0, io.SeekStart); err != nil { return 0, nil, nil, err } r := cpio.NewReader(f) - names := make(map[string]struct{}) + entries := make(map[string]cpio.FileMode) inodes := newInodeAllocator() var trailerOffset int64 for { @@ -233,12 +299,12 @@ func inspectInitrd(f *os.File) (int64, map[string]struct{}, *inodeAllocator, err if err := validateTrailerAt(f, trailerOffset); err != nil { return 0, nil, nil, err } - return trailerOffset, names, inodes, nil + return trailerOffset, entries, inodes, nil } if err != nil { return 0, nil, nil, fmt.Errorf("could not read newc record at offset %d: %w", trailerOffset, err) } - names[archiveLookupPath(hdr.Name)] = struct{}{} + entries[archiveLookupPath(hdr.Name)] = hdr.Mode inodes.reserve(hdr.Inode) if _, err := io.Copy(io.Discard, r); err != nil { return 0, nil, nil, err @@ -307,14 +373,14 @@ func archiveLookupPath(name string) string { return strings.TrimPrefix(path.Clean(name), "/") } -func addMissingParents(w *cpio.Writer, lookupName string, existingNames map[string]struct{}, inodes *inodeAllocator) error { +func addMissingParents(w *cpio.Writer, lookupName string, entries map[string]cpio.FileMode, inodes *inodeAllocator) error { parentName := "" for _, component := range strings.Split(path.Dir(lookupName), "/") { if component == "" || component == "." { continue } parentName = path.Join(parentName, component) - if _, exists := existingNames[parentName]; exists { + if _, exists := entries[parentName]; exists { continue } @@ -329,7 +395,7 @@ func addMissingParents(w *cpio.Writer, lookupName string, existingNames map[stri if err := w.WriteHeader(hdr); err != nil { return fmt.Errorf("could not add directory %s to initrd: %w", archiveName, err) } - existingNames[parentName] = struct{}{} + entries[parentName] = hdr.Mode } return nil } diff --git a/pkg/unikontainers/initrd/initrd_test.go b/pkg/unikontainers/initrd/initrd_test.go index bfac37748..cd458e7a9 100644 --- a/pkg/unikontainers/initrd/initrd_test.go +++ b/pkg/unikontainers/initrd/initrd_test.go @@ -19,6 +19,7 @@ import ( "io" "os" "path/filepath" + "syscall" "testing" "github.com/cavaliergopher/cpio" @@ -35,8 +36,6 @@ func TestMergeFileMountsIntoInitrd(t *testing.T) { }) require.NoError(t, os.Chmod(initrdPath, 0o640)) original := readFile(t, initrdPath) - originalInfo, err := os.Stat(initrdPath) - require.NoError(t, err) originalTrailer := bytes.Index(original, []byte("TRAILER!!!\x00")) - 110 require.GreaterOrEqual(t, originalTrailer, 0) @@ -62,8 +61,8 @@ func TestMergeFileMountsIntoInitrd(t *testing.T) { info, err := os.Stat(initrdPath) require.NoError(t, err) - require.True(t, os.SameFile(originalInfo, info), "initrd was replaced") require.Equal(t, os.FileMode(0o640), info.Mode().Perm()) + requireNoTemporaryFiles(t, initrdPath) } func TestMergeFileMountsIntoInitrdCanonicalizesExistingNames(t *testing.T) { @@ -107,7 +106,7 @@ func TestAddMissingParentsWalksTopDownAndTerminatesAtRoot(t *testing.T) { require.NoError(t, err) w := cpio.NewWriter(archive) - require.NoError(t, addMissingParents(w, lookupName, make(map[string]struct{}), newInodeAllocator())) + require.NoError(t, addMissingParents(w, lookupName, make(map[string]cpio.FileMode), newInodeAllocator())) require.NoError(t, w.Close()) require.NoError(t, archive.Close()) @@ -261,6 +260,137 @@ func TestMergeFileMountsIntoInitrdInvalidTrailerDoesNotMutateOriginal(t *testing } } +func TestMergeFileMountsIntoInitrdFailedMergeKeepsOriginal(t *testing.T) { + dir := t.TempDir() + initrdPath := filepath.Join(dir, "initrd.cpio") + writeArchive(t, initrdPath, []archiveEntry{ + {name: "./original", mode: cpio.TypeReg | 0o644, content: "old"}, + }) + original := readFile(t, initrdPath) + originalInfo, err := os.Stat(initrdPath) + require.NoError(t, err) + source := writeSource(t, dir, "mounted", "new") + + // The first mount is written before the second one fails. + err = MergeFileMountsIntoInitrd(initrdPath, []specs.Mount{ + {Type: "bind", Source: source, Destination: "/mounted"}, + {Type: "bind", Source: source, Destination: "relative"}, + }) + + require.ErrorContains(t, err, "is not absolute") + require.Equal(t, original, readFile(t, initrdPath)) + updatedInfo, err := os.Stat(initrdPath) + require.NoError(t, err) + require.True(t, os.SameFile(originalInfo, updatedInfo), "initrd was replaced") + requireNoTemporaryFiles(t, initrdPath) + + // Restarting the same container merges again from the intact initrd. + require.NoError(t, MergeFileMountsIntoInitrd(initrdPath, []specs.Mount{ + {Type: "bind", Source: source, Destination: "/mounted"}, + })) + require.Equal(t, "new", lastContent(readArchive(t, initrdPath), "./mounted")) +} + +func TestMergeFileMountsIntoInitrdRejectsConflictingDestinations(t *testing.T) { + directory := func(name string) archiveEntry { + return archiveEntry{name: name, mode: cpio.TypeDir | 0o755} + } + file := func(name string) archiveEntry { + return archiveEntry{name: name, mode: cpio.TypeReg | 0o644, content: "old"} + } + + tests := map[string]struct { + archive []archiveEntry + destinations []string + wantErr string + }{ + "parent is a regular file": { + archive: []archiveEntry{directory("./a"), file("./a/b")}, + destinations: []string{"/a/b/c"}, + wantErr: "a/b is not a directory", + }, + "parent spelled without the dot prefix is a regular file": { + archive: []archiveEntry{directory("a"), file("a/b")}, + destinations: []string{"/a/b/c"}, + wantErr: "a/b is not a directory", + }, + "destination is a directory": { + archive: []archiveEntry{directory("./a"), directory("./a/b"), file("./a/b/c")}, + destinations: []string{"/a/b"}, + wantErr: "a/b is a directory", + }, + "earlier mount is a parent of a later mount": { + archive: []archiveEntry{file("./original")}, + destinations: []string{"/a/b", "/a/b/c"}, + wantErr: "a/b is not a directory", + }, + "later mount replaces a parent added for an earlier mount": { + archive: []archiveEntry{file("./original")}, + destinations: []string{"/a/b/c", "/a/b"}, + wantErr: "a/b is a directory", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + initrdPath := filepath.Join(dir, "initrd.cpio") + writeArchive(t, initrdPath, tt.archive) + original := readFile(t, initrdPath) + source := writeSource(t, dir, "mounted", "new") + + var mounts []specs.Mount + for _, destination := range tt.destinations { + mounts = append(mounts, specs.Mount{Type: "bind", Source: source, Destination: destination}) + } + err := MergeFileMountsIntoInitrd(initrdPath, mounts) + + require.ErrorContains(t, err, tt.wantErr) + require.Equal(t, original, readFile(t, initrdPath)) + requireNoTemporaryFiles(t, initrdPath) + }) + } +} + +func TestMergeFileMountsIntoInitrdAcceptsSymlinkParent(t *testing.T) { + dir := t.TempDir() + initrdPath := filepath.Join(dir, "initrd.cpio") + writeArchive(t, initrdPath, []archiveEntry{ + {name: "./real", mode: cpio.TypeDir | 0o755}, + {name: "./link", mode: cpio.TypeSymlink | 0o777, content: "real"}, + }) + source := writeSource(t, dir, "mounted", "new") + + require.NoError(t, MergeFileMountsIntoInitrd(initrdPath, []specs.Mount{ + {Type: "bind", Source: source, Destination: "/link/mounted"}, + })) + + require.Equal(t, "new", lastContent(readArchive(t, initrdPath), "./link/mounted")) +} + +func TestMergeFileMountsIntoInitrdKeepsOwner(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("changing the owner of a file requires root") + } + dir := t.TempDir() + initrdPath := filepath.Join(dir, "initrd.cpio") + writeArchive(t, initrdPath, []archiveEntry{ + {name: "./original", mode: cpio.TypeReg | 0o644, content: "old"}, + }) + require.NoError(t, os.Chown(initrdPath, 1000, 1001)) + source := writeSource(t, dir, "mounted", "new") + + require.NoError(t, MergeFileMountsIntoInitrd(initrdPath, []specs.Mount{ + {Type: "bind", Source: source, Destination: "/mounted"}, + })) + + info, err := os.Stat(initrdPath) + require.NoError(t, err) + fileInfo := info.Sys().(*syscall.Stat_t) + require.Equal(t, uint32(1000), fileInfo.Uid) + require.Equal(t, uint32(1001), fileInfo.Gid) +} + type archiveEntry struct { name string inode int64 @@ -324,6 +454,13 @@ func readFile(t *testing.T, name string) []byte { return content } +func requireNoTemporaryFiles(t *testing.T, initrdPath string) { + t.Helper() + leftovers, err := filepath.Glob(initrdPath + ".tmp-*") + require.NoError(t, err) + require.Empty(t, leftovers) +} + func countName(entries []archiveEntry, name string) int { count := 0 for _, entry := range entries {