Make utimes_test pass on VFS2.

PiperOrigin-RevId: 311657502

4 files changed, 101 insertions, 61 deletions

diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go
index 353e2cf5b..ebf063a58 100644
--- a/pkg/sentry/fsimpl/gofer/gofer.go
+++ b/pkg/sentry/fsimpl/gofer/gofer.go
@@ -869,8 +869,8 @@ func (d *dentry) setStat(ctx context.Context, creds *auth.Credentials, stat *lin
 				Size:               stat.Mask&linux.STATX_SIZE != 0,
 				ATime:              stat.Mask&linux.STATX_ATIME != 0,
 				MTime:              stat.Mask&linux.STATX_MTIME != 0,
-				ATimeNotSystemTime: stat.Atime.Nsec != linux.UTIME_NOW,
-				MTimeNotSystemTime: stat.Mtime.Nsec != linux.UTIME_NOW,
+				ATimeNotSystemTime: stat.Mask&linux.STATX_ATIME != 0 && stat.Atime.Nsec != linux.UTIME_NOW,
+				MTimeNotSystemTime: stat.Mask&linux.STATX_MTIME != 0 && stat.Mtime.Nsec != linux.UTIME_NOW,
 			}, p9.SetAttr{
 				Permissions:      p9.FileMode(stat.Mode),
 				UID:              p9.UID(stat.UID),
diff --git a/pkg/sentry/syscalls/linux/vfs2/setstat.go b/pkg/sentry/syscalls/linux/vfs2/setstat.go
index 4e61f1452..09ecfed26 100644
--- a/pkg/sentry/syscalls/linux/vfs2/setstat.go
+++ b/pkg/sentry/syscalls/linux/vfs2/setstat.go
@@ -246,73 +246,104 @@ func Utimes(t *kernel.Task, args arch.SyscallArguments) (uintptr, *kernel.Syscal
 		return 0, nil, err
 	}
 
-	opts := vfs.SetStatOptions{
-		Stat: linux.Statx{
-			Mask: linux.STATX_ATIME | linux.STATX_MTIME,
-		},
-	}
-	if timesAddr == 0 {
-		opts.Stat.Atime.Nsec = linux.UTIME_NOW
-		opts.Stat.Mtime.Nsec = linux.UTIME_NOW
-	} else {
-		var times [2]linux.Timeval
-		if _, err := t.CopyIn(timesAddr, &times); err != nil {
-			return 0, nil, err
-		}
-		opts.Stat.Atime = linux.StatxTimestamp{
-			Sec:  times[0].Sec,
-			Nsec: uint32(times[0].Usec * 1000),
-		}
-		opts.Stat.Mtime = linux.StatxTimestamp{
-			Sec:  times[1].Sec,
-			Nsec: uint32(times[1].Usec * 1000),
-		}
+	var opts vfs.SetStatOptions
+	if err := populateSetStatOptionsForUtimes(t, timesAddr, &opts); err != nil {
+		return 0, nil, err
 	}
 
 	return 0, nil, setstatat(t, linux.AT_FDCWD, path, disallowEmptyPath, followFinalSymlink, &opts)
 }
 
-// Utimensat implements Linux syscall utimensat(2).
-func Utimensat(t *kernel.Task, args arch.SyscallArguments) (uintptr, *kernel.SyscallControl, error) {
+// Futimesat implements Linux syscall futimesat(2).
+func Futimesat(t *kernel.Task, args arch.SyscallArguments) (uintptr, *kernel.SyscallControl, error) {
 	dirfd := args[0].Int()
 	pathAddr := args[1].Pointer()
 	timesAddr := args[2].Pointer()
-	flags := args[3].Int()
 
-	if flags&^linux.AT_SYMLINK_NOFOLLOW != 0 {
-		return 0, nil, syserror.EINVAL
-	}
-
-	path, err := copyInPath(t, pathAddr)
-	if err != nil {
-		return 0, nil, err
+	// "If filename is NULL and dfd refers to an open file, then operate on the
+	// file. Otherwise look up filename, possibly using dfd as a starting
+	// point." - fs/utimes.c
+	var path fspath.Path
+	shouldAllowEmptyPath := allowEmptyPath
+	if dirfd == linux.AT_FDCWD || pathAddr != 0 {
+		var err error
+		path, err = copyInPath(t, pathAddr)
+		if err != nil {
+			return 0, nil, err
+		}
+		shouldAllowEmptyPath = disallowEmptyPath
 	}
 
 	var opts vfs.SetStatOptions
-	if err := populateSetStatOptionsForUtimens(t, timesAddr, &opts); err != nil {
+	if err := populateSetStatOptionsForUtimes(t, timesAddr, &opts); err != nil {
 		return 0, nil, err
 	}
 
-	return 0, nil, setstatat(t, dirfd, path, disallowEmptyPath, followFinalSymlink, &opts)
+	return 0, nil, setstatat(t, dirfd, path, shouldAllowEmptyPath, followFinalSymlink, &opts)
 }
 
-// Futimens implements Linux syscall futimens(2).
-func Futimens(t *kernel.Task, args arch.SyscallArguments) (uintptr, *kernel.SyscallControl, error) {
-	fd := args[0].Int()
-	timesAddr := args[1].Pointer()
-
-	file := t.GetFileVFS2(fd)
-	if file == nil {
-		return 0, nil, syserror.EBADF
+func populateSetStatOptionsForUtimes(t *kernel.Task, timesAddr usermem.Addr, opts *vfs.SetStatOptions) error {
+	if timesAddr == 0 {
+		opts.Stat.Mask = linux.STATX_ATIME | linux.STATX_MTIME
+		opts.Stat.Atime.Nsec = linux.UTIME_NOW
+		opts.Stat.Mtime.Nsec = linux.UTIME_NOW
+		return nil
 	}
-	defer file.DecRef()
+	var times [2]linux.Timeval
+	if _, err := t.CopyIn(timesAddr, &times); err != nil {
+		return err
+	}
+	if times[0].Usec < 0 || times[0].Usec > 999999 || times[1].Usec < 0 || times[1].Usec > 999999 {
+		return syserror.EINVAL
+	}
+	opts.Stat.Mask = linux.STATX_ATIME | linux.STATX_MTIME
+	opts.Stat.Atime = linux.StatxTimestamp{
+		Sec:  times[0].Sec,
+		Nsec: uint32(times[0].Usec * 1000),
+	}
+	opts.Stat.Mtime = linux.StatxTimestamp{
+		Sec:  times[1].Sec,
+		Nsec: uint32(times[1].Usec * 1000),
+	}
+	return nil
+}
 
+// Utimensat implements Linux syscall utimensat(2).
+func Utimensat(t *kernel.Task, args arch.SyscallArguments) (uintptr, *kernel.SyscallControl, error) {
+	dirfd := args[0].Int()
+	pathAddr := args[1].Pointer()
+	timesAddr := args[2].Pointer()
+	flags := args[3].Int()
+
+	// Linux requires that the UTIME_OMIT check occur before checking path or
+	// flags.
 	var opts vfs.SetStatOptions
 	if err := populateSetStatOptionsForUtimens(t, timesAddr, &opts); err != nil {
 		return 0, nil, err
 	}
+	if opts.Stat.Mask == 0 {
+		return 0, nil, nil
+	}
 
-	return 0, nil, file.SetStat(t, opts)
+	if flags&^linux.AT_SYMLINK_NOFOLLOW != 0 {
+		return 0, nil, syserror.EINVAL
+	}
+
+	// "If filename is NULL and dfd refers to an open file, then operate on the
+	// file. Otherwise look up filename, possibly using dfd as a starting
+	// point." - fs/utimes.c
+	var path fspath.Path
+	shouldAllowEmptyPath := allowEmptyPath
+	if dirfd == linux.AT_FDCWD || pathAddr != 0 {
+		var err error
+		path, err = copyInPath(t, pathAddr)
+		if err != nil {
+			return 0, nil, err
+		}
+		shouldAllowEmptyPath = disallowEmptyPath
+	}
+
+	return 0, nil, setstatat(t, dirfd, path, shouldAllowEmptyPath, shouldFollowFinalSymlink(flags&linux.AT_SYMLINK_NOFOLLOW == 0), &opts)
 }
 
 func populateSetStatOptionsForUtimens(t *kernel.Task, timesAddr usermem.Addr, opts *vfs.SetStatOptions) error {
@@ -327,6 +358,9 @@ func populateSetStatOptionsForUtimens(t *kernel.Task, timesAddr usermem.Addr, op
 		return err
 	}
 	if times[0].Nsec != linux.UTIME_OMIT {
+		if times[0].Nsec != linux.UTIME_NOW && (times[0].Nsec < 0 || times[0].Nsec > 999999999) {
+			return syserror.EINVAL
+		}
 		opts.Stat.Mask |= linux.STATX_ATIME
 		opts.Stat.Atime = linux.StatxTimestamp{
 			Sec:  times[0].Sec,
@@ -334,6 +368,9 @@ func populateSetStatOptionsForUtimens(t *kernel.Task, timesAddr usermem.Addr, op
 		}
 	}
 	if times[1].Nsec != linux.UTIME_OMIT {
+		if times[1].Nsec != linux.UTIME_NOW && (times[1].Nsec < 0 || times[1].Nsec > 999999999) {
+			return syserror.EINVAL
+		}
 		opts.Stat.Mask |= linux.STATX_MTIME
 		opts.Stat.Mtime = linux.StatxTimestamp{
 			Sec:  times[1].Sec,
diff --git a/pkg/sentry/syscalls/linux/vfs2/vfs2.go b/pkg/sentry/syscalls/linux/vfs2/vfs2.go
index ec8da7f06..a332d01bd 100644
--- a/pkg/sentry/syscalls/linux/vfs2/vfs2.go
+++ b/pkg/sentry/syscalls/linux/vfs2/vfs2.go
@@ -123,7 +123,7 @@ func Override() {
 	s.Table[258] = syscalls.Supported("mkdirat", Mkdirat)
 	s.Table[259] = syscalls.Supported("mknodat", Mknodat)
 	s.Table[260] = syscalls.Supported("fchownat", Fchownat)
-	s.Table[261] = syscalls.Supported("futimens", Futimens)
+	s.Table[261] = syscalls.Supported("futimesat", Futimesat)
 	s.Table[262] = syscalls.Supported("newfstatat", Newfstatat)
 	s.Table[263] = syscalls.Supported("unlinkat", Unlinkat)
 	s.Table[264] = syscalls.Supported("renameat", Renameat)
diff --git a/test/syscalls/linux/utimes.cc b/test/syscalls/linux/utimes.cc
index 22e6d1a85..e647d2896 100644
--- a/test/syscalls/linux/utimes.cc
+++ b/test/syscalls/linux/utimes.cc
@@ -48,12 +48,15 @@ void TimeBoxed(absl::Time* before, absl::Time* after,
     // filesystems set it to 1, so we don't do any truncation.
     struct timespec ts;
     EXPECT_THAT(clock_gettime(CLOCK_REALTIME_COARSE, &ts), SyscallSucceeds());
-    *before = absl::TimeFromTimespec(ts);
+    // FIXME(b/132819225): gVisor filesystem timestamps inconsistently use the
+    // internal or host clock, which may diverge slightly. Allow some slack on
+    // times to account for the difference.
+    *before = absl::TimeFromTimespec(ts) - absl::Seconds(1);
 
     fn();
 
     EXPECT_THAT(clock_gettime(CLOCK_REALTIME_COARSE, &ts), SyscallSucceeds());
-    *after = absl::TimeFromTimespec(ts);
+    *after = absl::TimeFromTimespec(ts) + absl::Seconds(1);
 
     if (*after < *before) {
       // Clock jumped backwards; retry.
@@ -68,11 +71,11 @@ void TimeBoxed(absl::Time* before, absl::Time* after,
 void TestUtimesOnPath(std::string const& path) {
   struct stat statbuf;
 
-  struct timeval times[2] = {{1, 0}, {2, 0}};
+  struct timeval times[2] = {{10, 0}, {20, 0}};
   EXPECT_THAT(utimes(path.c_str(), times), SyscallSucceeds());
   EXPECT_THAT(stat(path.c_str(), &statbuf), SyscallSucceeds());
-  EXPECT_EQ(1, statbuf.st_atime);
-  EXPECT_EQ(2, statbuf.st_mtime);
+  EXPECT_EQ(10, statbuf.st_atime);
+  EXPECT_EQ(20, statbuf.st_mtime);
 
   absl::Time before;
   absl::Time after;
@@ -103,18 +106,18 @@ TEST(UtimesTest, OnDir) {
 
 TEST(UtimesTest, MissingPath) {
   auto path = NewTempAbsPath();
-  struct timeval times[2] = {{1, 0}, {2, 0}};
+  struct timeval times[2] = {{10, 0}, {20, 0}};
   EXPECT_THAT(utimes(path.c_str(), times), SyscallFailsWithErrno(ENOENT));
 }
 
 void TestFutimesat(int dirFd, std::string const& path) {
   struct stat statbuf;
 
-  struct timeval times[2] = {{1, 0}, {2, 0}};
+  struct timeval times[2] = {{10, 0}, {20, 0}};
   EXPECT_THAT(futimesat(dirFd, path.c_str(), times), SyscallSucceeds());
   EXPECT_THAT(fstatat(dirFd, path.c_str(), &statbuf, 0), SyscallSucceeds());
-  EXPECT_EQ(1, statbuf.st_atime);
-  EXPECT_EQ(2, statbuf.st_mtime);
+  EXPECT_EQ(10, statbuf.st_atime);
+  EXPECT_EQ(20, statbuf.st_mtime);
 
   absl::Time before;
   absl::Time after;
@@ -175,11 +178,11 @@ TEST(FutimesatTest, InvalidNsec) {
 
 void TestUtimensat(int dirFd, std::string const& path) {
   struct stat statbuf;
-  const struct timespec times[2] = {{1, 0}, {2, 0}};
+  const struct timespec times[2] = {{10, 0}, {20, 0}};
   EXPECT_THAT(utimensat(dirFd, path.c_str(), times, 0), SyscallSucceeds());
   EXPECT_THAT(fstatat(dirFd, path.c_str(), &statbuf, 0), SyscallSucceeds());
-  EXPECT_EQ(1, statbuf.st_atime);
-  EXPECT_EQ(2, statbuf.st_mtime);
+  EXPECT_EQ(10, statbuf.st_atime);
+  EXPECT_EQ(20, statbuf.st_mtime);
 
   // Test setting with UTIME_NOW and UTIME_OMIT.
   struct stat statbuf2;
@@ -301,13 +304,13 @@ TEST(Utimensat, NullPath) {
   auto f = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
   const FileDescriptor fd = ASSERT_NO_ERRNO_AND_VALUE(Open(f.path(), O_RDWR));
   struct stat statbuf;
-  const struct timespec times[2] = {{1, 0}, {2, 0}};
+  const struct timespec times[2] = {{10, 0}, {20, 0}};
   // Call syscall directly.
   EXPECT_THAT(syscall(SYS_utimensat, fd.get(), NULL, times, 0),
               SyscallSucceeds());
   EXPECT_THAT(fstatat(0, f.path().c_str(), &statbuf, 0), SyscallSucceeds());
-  EXPECT_EQ(1, statbuf.st_atime);
-  EXPECT_EQ(2, statbuf.st_mtime);
+  EXPECT_EQ(10, statbuf.st_atime);
+  EXPECT_EQ(20, statbuf.st_mtime);
 }
 
 }  // namespace