Merge pull request #9103 from bboozzoo/bboozzoo/uc20-mountedfs-atomic…

…-file

gadget, osutil: use atomic file copy, adjust tests

gadget/mountedfilesystem.go

@@ -1,7 +1,7 @@
 // -*- Mode: Go; indent-tabs-mode: t -*-
 
 /*
- * Copyright (C) 2019 Canonical Ltd
+ * Copyright (C) 2019-2020 Canonical Ltd
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
@@ -217,12 +217,11 @@ func writeFileOrSymlink(src, dst string, preserveInDst []string) error {
 			return fmt.Errorf("cannot write a symlink: %v", err)
 		}
 	} else {
-		// overwrite & sync by default
-		copyFlags := osutil.CopyFlagOverwrite | osutil.CopyFlagSync
-
-		// TODO use osutil.AtomicFile
 		// TODO try to preserve ownership and permission bits
-		if err := osutil.CopyFile(src, dst, copyFlags); err != nil {
+
+		// do not follow sylimks, dst is a reflection of the src which
+		// is a file
+		if err := osutil.AtomicWriteFileCopy(dst, src, 0); err != nil {
 			return fmt.Errorf("cannot copy %s: %v", src, err)
 		}
 	}

gadget/mountedfilesystem_test.go

@@ -473,7 +473,7 @@ func (s *mountedfilesystemTestSuite) TestMountedWriterConflictingDestinationDire
 
 	// can't overwrite a directory with a file
 	err = rw.Write(outDir, nil)
-	c.Assert(err, ErrorMatches, fmt.Sprintf("cannot write filesystem content of source:foo-dir: cannot copy .*: unable to create %s/foo-dir: .* is a directory", outDir))
+	c.Assert(err, ErrorMatches, fmt.Sprintf(`cannot write filesystem content of source:foo-dir: cannot copy .*: cannot commit atomic file copy: rename %[1]s/foo-dir\.[a-zA-Z0-9]+~ %[1]s/foo-dir: file exists`, outDir))
 
 }
 
@@ -2021,7 +2021,8 @@ func (s *mountedfilesystemTestSuite) TestMountedUpdaterRollbackRestoreFails(c *C
 		{target: "foo", content: "written"},
 		{target: "some-dir/foo", content: "written"},
 	})
-	// make rollback fail when restoring
+	// the file exists, and cannot be modified directly, rollback will still
+	// restore the backup as we atomically swap copies with rename()
 	err := os.Chmod(filepath.Join(outDir, "foo"), 0000)
 	c.Assert(err, IsNil)
 
@@ -2055,12 +2056,19 @@ func (s *mountedfilesystemTestSuite) TestMountedUpdaterRollbackRestoreFails(c *C
 	makeSizedFile(c, filepath.Join(s.backup, "struct-0/foo.backup"), 0, []byte("backup"))
 
 	err = rw.Rollback()
-	c.Assert(err, ErrorMatches, "cannot rollback content: cannot copy .*: unable to create .*/out-dir/foo: permission denied")
+	c.Assert(err, IsNil)
+	// the file was restored
+	c.Check(filepath.Join(outDir, "foo"), testutil.FileEquals, "backup")
+	// directory was removed
+	c.Check(osutil.IsDirectory(filepath.Join(outDir, "some-dir")), Equals, false)
 
-	// remove offending file
-	c.Assert(os.Remove(filepath.Join(outDir, "foo")), IsNil)
+	// mock the data again
+	makeExistingData(c, outDir, []gadgetData{
+		{target: "foo", content: "written"},
+		{target: "some-dir/foo", content: "written"},
+	})
 
-	// make destination dir non-writable
+	// make the directory non-writable
 	err = os.Chmod(filepath.Join(outDir, "some-dir"), 0555)
 	c.Assert(err, IsNil)
 	// restore permissions later, otherwise test suite cleanup complains

osutil/cp.go

@@ -117,6 +117,47 @@ func CopyFile(src, dst string, flags CopyFlag) (err error) {
 	return nil
 }
 
+// AtomicWriteFileCopy writes to dst a copy of src using AtomicFile
+// internally to create the destination.
+// The destination path is always overwritten. The destination and
+// the owning directory are synced after copy completes. Pass additional flags
+// for AtomicFile wrapping the destination.
+func AtomicWriteFileCopy(dst, src string, flags AtomicWriteFlags) (err error) {
+	fin, err := openfile(src, os.O_RDONLY, 0)
+	if err != nil {
+		return fmt.Errorf("unable to open source file %s: %v", src, err)
+	}
+	defer func() {
+		if cerr := fin.Close(); cerr != nil && err == nil {
+			err = fmt.Errorf("when closing %s: %v", src, cerr)
+		}
+	}()
+
+	fi, err := fin.Stat()
+	if err != nil {
+		return fmt.Errorf("unable to stat %s: %v", src, err)
+	}
+
+	fout, err := NewAtomicFile(dst, fi.Mode(), flags, NoChown, NoChown)
+	if err != nil {
+		return fmt.Errorf("cannot create atomic file: %v", err)
+	}
+	defer func() {
+		if cerr := fout.Cancel(); cerr != ErrCannotCancel && err == nil {
+			err = fmt.Errorf("cannot cancel temporary file copy %s: %v", fout.Name(), cerr)
+		}
+	}()
+
+	if err := copyfile(fin, fout, fi); err != nil {
+		return fmt.Errorf("unable to copy %s to %s: %v", src, fout.Name(), err)
+	}
+
+	if err := fout.Commit(); err != nil {
+		return fmt.Errorf("cannot commit atomic file copy: %v", err)
+	}
+	return nil
+}
+
 func runCmd(cmd *exec.Cmd, errdesc string) error {
 	if output, err := cmd.CombinedOutput(); err != nil {
 		output = bytes.TrimSpace(output)

osutil/cp_test.go

@@ -1,7 +1,7 @@
 // -*- Mode: Go; indent-tabs-mode: t -*-
 
 /*
- * Copyright (C) 2014-2015 Canonical Ltd
+ * Copyright (C) 2014-2020 Canonical Ltd
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
@@ -300,3 +300,74 @@ func (s *cpSuite) TestCopyPreserveAllSyncSyncFailure(c *C) {
 		{"sync"},
 	})
 }
+
+func (s *cpSuite) TestAtomicWriteFileCopySimple(c *C) {
+	err := AtomicWriteFileCopy(s.f2, s.f1, 0)
+	c.Assert(err, IsNil)
+	c.Assert(s.f2, testutil.FileEquals, s.data)
+
+}
+
+func (s *cpSuite) TestAtomicWriteFileCopyOverwrites(c *C) {
+	err := ioutil.WriteFile(s.f2, []byte("this is f2 content"), 0644)
+	c.Assert(err, IsNil)
+
+	err = AtomicWriteFileCopy(s.f2, s.f1, 0)
+	c.Assert(err, IsNil)
+	c.Assert(s.f2, testutil.FileEquals, s.data)
+}
+
+func (s *cpSuite) TestAtomicWriteFileCopySymlinks(c *C) {
+	f2Symlink := filepath.Join(s.dir, "f2-symlink")
+	err := os.Symlink(s.f2, f2Symlink)
+	c.Assert(err, IsNil)
+
+	f2SymlinkNoFollow := filepath.Join(s.dir, "f2-symlink-no-follow")
+	err = os.Symlink(s.f2, f2SymlinkNoFollow)
+	c.Assert(err, IsNil)
+
+	// follows symlink, dst is f2
+	err = AtomicWriteFileCopy(f2Symlink, s.f1, AtomicWriteFollow)
+	c.Assert(err, IsNil)
+	c.Check(IsSymlink(f2Symlink), Equals, true, Commentf("%q is not a symlink", f2Symlink))
+	c.Check(s.f2, testutil.FileEquals, s.data)
+	c.Check(f2SymlinkNoFollow, testutil.FileEquals, s.data)
+
+	// when not following, copy overwrites the symlink
+	err = AtomicWriteFileCopy(f2SymlinkNoFollow, s.f1, 0)
+	c.Assert(err, IsNil)
+	c.Check(IsSymlink(f2SymlinkNoFollow), Equals, false, Commentf("%q is not a file", f2SymlinkNoFollow))
+	c.Check(f2SymlinkNoFollow, testutil.FileEquals, s.data)
+}
+
+func (s *cpSuite) TestAtomicWriteFileCopyErrReal(c *C) {
+	err := AtomicWriteFileCopy(s.f2, filepath.Join(s.dir, "random-file"), 0)
+	c.Assert(err, ErrorMatches, "unable to open source file .*/random-file: open .* no such file or directory")
+
+	dir := c.MkDir()
+
+	err = AtomicWriteFileCopy(filepath.Join(dir, "random-dir", "f3"), s.f1, 0)
+	c.Assert(err, ErrorMatches, `cannot create atomic file: open .*/random-dir/f3\.[a-zA-Z0-9]+~: no such file or directory`)
+
+	err = os.MkdirAll(filepath.Join(dir, "read-only"), 0000)
+	c.Assert(err, IsNil)
+	err = AtomicWriteFileCopy(filepath.Join(dir, "read-only", "f3"), s.f1, 0)
+	c.Assert(err, ErrorMatches, `cannot create atomic file: open .*/read-only/f3\.[a-zA-Z0-9]+~: permission denied`)
+}
+
+func (s *cpSuite) TestAtomicWriteFileCopyErrMockedCopy(c *C) {
+	s.mock()
+	s.errs = []error{
+		nil, // openFile
+		nil, // src.Stat()
+		errors.New("copy fail"),
+	}
+
+	err := AtomicWriteFileCopy(s.f2, s.f1, 0)
+	c.Assert(err, ErrorMatches, `unable to copy .*/f1 to .*/f2\.[a-zA-Z0-9]+~: copy fail`)
+	entries, err := filepath.Glob(filepath.Join(s.dir, "*"))
+	c.Assert(err, IsNil)
+	c.Assert(entries, DeepEquals, []string{
+		filepath.Join(s.dir, "f1"),
+	})
+}

osutil/io.go

@@ -1,7 +1,7 @@
 // -*- Mode: Go; indent-tabs-mode: t -*-
 
 /*
- * Copyright (C) 2014-2015 Canonical Ltd
+ * Copyright (C) 2014-2020 Canonical Ltd
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as