Fix setting mtimes on directories

Sargun Dhillon · Sargun Dhillon · commit 77a2bc3e5bbc · 2017-12-01T09:12:43.000-08:00
Previously, the code would set the mtime on the directories before
creating files in the directory itself. This was problematic
because it resulted in the mtimes on the directories being
incorrectly set. This change makes it so that the mtime is
set only _after_ all of the files have been created.

Signed-off-by: Sargun Dhillon &lt;sargun@sargun.me&gt;
diff --git a/daemon/graphdriver/copy/copy.go b/daemon/graphdriver/copy/copy.go
@@ -11,6 +11,7 @@ package copy
 */
 import "C"
 import (
+	"container/list"
 	"fmt"
 	"io"
 	"os"
@@ -111,16 +112,23 @@ type fileID struct {
 	ino uint64
 }
 
+type dirMtimeInfo struct {
+	dstPath *string
+	stat    *syscall.Stat_t
+}
+
 // DirCopy copies or hardlinks the contents of one directory to another,
 // properly handling xattrs, and soft links
 //
 // Copying xattrs can be opted out of by passing false for copyXattrs.
 func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error {
 	copyWithFileRange := true
 	copyWithFileClone := true
+
 	// This is a map of source file inodes to dst file paths
 	copiedFiles := make(map[fileID]string)
 
+	dirsToSetMtimes := list.New()
 	err := filepath.Walk(srcDir, func(srcPath string, f os.FileInfo, err error) error {
 		if err != nil {
 			return err
@@ -226,7 +234,9 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error {
 
 		// system.Chtimes doesn't support a NOFOLLOW flag atm
 		// nolint: unconvert
-		if !isSymlink {
+		if f.IsDir() {
+			dirsToSetMtimes.PushFront(&dirMtimeInfo{dstPath: &dstPath, stat: stat})
+		} else if !isSymlink {
 			aTime := time.Unix(int64(stat.Atim.Sec), int64(stat.Atim.Nsec))
 			mTime := time.Unix(int64(stat.Mtim.Sec), int64(stat.Mtim.Nsec))
 			if err := system.Chtimes(dstPath, aTime, mTime); err != nil {
@@ -240,7 +250,18 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error {
 		}
 		return nil
 	})
-	return err
+	if err != nil {
+		return err
+	}
+	for e := dirsToSetMtimes.Front(); e != nil; e = e.Next() {
+		mtimeInfo := e.Value.(*dirMtimeInfo)
+		ts := []syscall.Timespec{mtimeInfo.stat.Atim, mtimeInfo.stat.Mtim}
+		if err := system.LUtimesNano(*mtimeInfo.dstPath, ts); err != nil {
+			return err
+		}
+	}
+
+	return nil
 }
 
 func doCopyXattrs(srcPath, dstPath string) error {
diff --git a/daemon/graphdriver/copy/copy_test.go b/daemon/graphdriver/copy/copy_test.go
@@ -3,17 +3,20 @@
 package copy
 
 import (
+	"fmt"
 	"io/ioutil"
 	"math/rand"
 	"os"
 	"path/filepath"
+	"syscall"
 	"testing"
-
-	"golang.org/x/sys/unix"
+	"time"
 
 	"github.com/docker/docker/pkg/parsers/kernel"
+	"github.com/docker/docker/pkg/system"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/unix"
 )
 
 func TestIsCopyFileRangeSyscallAvailable(t *testing.T) {
@@ -47,6 +50,84 @@ func TestCopyWithoutRange(t *testing.T) {
 	doCopyTest(t, &copyWithFileRange, &copyWithFileClone)
 }
 
+func TestCopyDir(t *testing.T) {
+	srcDir, err := ioutil.TempDir("", "srcDir")
+	require.NoError(t, err)
+	populateSrcDir(t, srcDir, 3)
+
+	dstDir, err := ioutil.TempDir("", "testdst")
+	require.NoError(t, err)
+	defer os.RemoveAll(dstDir)
+
+	assert.NoError(t, DirCopy(srcDir, dstDir, Content, false))
+	require.NoError(t, filepath.Walk(srcDir, func(srcPath string, f os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+
+		// Rebase path
+		relPath, err := filepath.Rel(srcDir, srcPath)
+		require.NoError(t, err)
+		if relPath == "." {
+			return nil
+		}
+
+		dstPath := filepath.Join(dstDir, relPath)
+		require.NoError(t, err)
+
+		// If we add non-regular dirs and files to the test
+		// then we need to add more checks here.
+		dstFileInfo, err := os.Lstat(dstPath)
+		require.NoError(t, err)
+
+		srcFileSys := f.Sys().(*syscall.Stat_t)
+		dstFileSys := dstFileInfo.Sys().(*syscall.Stat_t)
+
+		t.Log(relPath)
+		if srcFileSys.Dev == dstFileSys.Dev {
+			assert.NotEqual(t, srcFileSys.Ino, dstFileSys.Ino)
+		}
+		// Todo: check size, and ctim is not equal
+		/// on filesystems that have granular ctimes
+		assert.Equal(t, srcFileSys.Mode, dstFileSys.Mode)
+		assert.Equal(t, srcFileSys.Uid, dstFileSys.Uid)
+		assert.Equal(t, srcFileSys.Gid, dstFileSys.Gid)
+		assert.Equal(t, srcFileSys.Mtim, dstFileSys.Mtim)
+
+		return nil
+	}))
+}
+
+func randomMode(baseMode int) os.FileMode {
+	for i := 0; i < 7; i++ {
+		baseMode = baseMode | (1&rand.Intn(2))<<uint(i)
+	}
+	return os.FileMode(baseMode)
+}
+
+func populateSrcDir(t *testing.T, srcDir string, remainingDepth int) {
+	if remainingDepth == 0 {
+		return
+	}
+	aTime := time.Unix(rand.Int63(), 0)
+	mTime := time.Unix(rand.Int63(), 0)
+
+	for i := 0; i < 10; i++ {
+		dirName := filepath.Join(srcDir, fmt.Sprintf("srcdir-%d", i))
+		// Owner all bits set
+		require.NoError(t, os.Mkdir(dirName, randomMode(0700)))
+		populateSrcDir(t, dirName, remainingDepth-1)
+		require.NoError(t, system.Chtimes(dirName, aTime, mTime))
+	}
+
+	for i := 0; i < 10; i++ {
+		fileName := filepath.Join(srcDir, fmt.Sprintf("srcfile-%d", i))
+		// Owner read bit set
+		require.NoError(t, ioutil.WriteFile(fileName, []byte{}, randomMode(0400)))
+		require.NoError(t, system.Chtimes(fileName, aTime, mTime))
+	}
+}
+
 func doCopyTest(t *testing.T, copyWithFileRange, copyWithFileClone *bool) {
 	dir, err := ioutil.TempDir("", "docker-copy-check")
 	require.NoError(t, err)
