From d8955b2c052590603f0df28a64316f63967b1f84 Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Mon, 5 Nov 2018 06:21:50 -0500 Subject: [PATCH] Return ENOTSUP when trying to setattr the times of a symlink The setattr hook should be using futimensat to update the timestamps of the node so that we could support the case of modifying those of a symlink (without following it). Unfortunately, we currently cannot do so because our testing environment does not have this system call available in all platforms... so just return ENOTSUP for now (instead of doing the wrong thing and following the symlink). --- WORKSPACE | 2 +- integration/read_write_test.go | 58 +++++++++++----------------------- internal/sandbox/node.go | 7 ++++ src/nodes/mod.rs | 11 ++++--- 4 files changed, 32 insertions(+), 46 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 77f1c9e..8187155 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -57,7 +57,7 @@ go_repository( go_repository( name = "org_golang_x_sys", importpath = "golang.org/x/sys", - commit = "4b45465282a4624cf39876842a017334f13b8aff", + commit = "9b800f95dbbc54abff0acf7ee32d88ba4e328c89", ) gazelle_dependencies() diff --git a/integration/read_write_test.go b/integration/read_write_test.go index ca7d743..9b34fa2 100644 --- a/integration/read_write_test.go +++ b/integration/read_write_test.go @@ -24,6 +24,8 @@ import ( "testing" "time" + "golang.org/x/sys/unix" + "github.com/bazelbuild/sandboxfs/integration/utils" "github.com/bazelbuild/sandboxfs/internal/sandbox" ) @@ -565,20 +567,11 @@ func TestReadWrite_Chmod(t *testing.T) { } }) - t.Run("DanglingSymlink", func(t *testing.T) { - utils.MustSymlink(t, "missing", state.RootPath("dangling-symlink")) - - path := state.MountPath("dangling-symlink") - if err := os.Chmod(path, 0555); err == nil { - t.Errorf("Want chmod to fail on dangling link, got success") - } - }) - - t.Run("GoodSymlink", func(t *testing.T) { + t.Run("Symlink", func(t *testing.T) { utils.MustWriteFile(t, state.RootPath("target"), 0644, "") - utils.MustSymlink(t, "target", state.RootPath("good-symlink")) + utils.MustSymlink(t, "target", state.RootPath("symlink")) - path := state.MountPath("good-symlink") + path := state.MountPath("symlink") linkFileInfo, err := os.Lstat(path) if err != nil { t.Fatalf("Failed to stat %s: %v", path, err) @@ -588,11 +581,11 @@ func TestReadWrite_Chmod(t *testing.T) { t.Fatalf("Failed to chmod %s: %v", path, err) } - if err := checkPerm("good-symlink", linkFileInfo.Mode()&os.ModePerm); err != nil { + if err := checkPerm("symlink", linkFileInfo.Mode()&os.ModePerm); err != nil { t.Error(err) } if err := checkPerm("target", 0200); err != nil { - t.Error(err) + t.Errorf("Mode of symlink target was modified but shouldn't have been: %v", err) } }) } @@ -664,9 +657,8 @@ func TestReadWrite_Chown(t *testing.T) { utils.MustMkdirAll(t, state.RootPath("dir"), 0755) utils.MustWriteFile(t, state.RootPath("file"), 0644, "new content") - utils.MustSymlink(t, "missing", state.RootPath("dangling-symlink")) utils.MustWriteFile(t, state.RootPath("target"), 0644, "") - utils.MustSymlink(t, "target", state.RootPath("good-symlink")) + utils.MustSymlink(t, "target", state.RootPath("symlink")) targetFileInfo, err := os.Lstat(state.RootPath("target")) if err != nil { @@ -683,8 +675,7 @@ func TestReadWrite_Chown(t *testing.T) { }{ {"Dir", "dir", 1, 2}, {"File", "file", 3, 4}, - {"DanglingSymlink", "dangling-symlink", 5, 6}, - {"GoodSymlink", "good-symlink", 7, 8}, + {"Symlink", "symlink", 7, 8}, } for _, d := range testData { t.Run(d.name, func(t *testing.T) { @@ -821,35 +812,22 @@ func TestReadWrite_Chtimes(t *testing.T) { } }) - t.Run("DanglingSymlink", func(t *testing.T) { - utils.MustSymlink(t, "missing", state.RootPath("dangling-symlink")) - - if _, err := chtimes("dangling-symlink", time.Unix(0, 0), time.Unix(0, 0)); err == nil { - t.Errorf("Want chtimes to fail on dangling link, got success") - } - }) - - t.Run("GoodSymlink", func(t *testing.T) { + t.Run("Symlink", func(t *testing.T) { utils.MustWriteFile(t, state.RootPath("target"), 0644, "") - utils.MustSymlink(t, "target", state.RootPath("good-symlink")) - path := state.MountPath("good-symlink") + utils.MustSymlink(t, "target", state.RootPath("symlink")) + path := state.MountPath("symlink") - linkFileInfo, err := os.Lstat(path) + atimeTimespec, err := unix.TimeToTimespec(someAtime) if err != nil { - t.Fatalf("Failed to stat %s: %v", path, err) + t.Fatalf("Failed to convert %v to a timespec: %v", someAtime, err) } - linkStat := linkFileInfo.Sys().(*syscall.Stat_t) - - wantMinCtime, err := chtimes(path, someAtime, someMtime) + mtimeTimespec, err := unix.TimeToTimespec(someMtime) if err != nil { - t.Fatal(err) + t.Fatalf("Failed to convert %v to a timespec: %v", someMtime, err) } - if err := checkTimes("good-symlink", time.Unix(0, 0), linkFileInfo.ModTime(), sandbox.Ctime(linkStat)); err != nil { - t.Error(err) - } - if err := checkTimes("target", someAtime, someMtime, wantMinCtime); err != nil { - t.Error(err) + if err = unix.UtimesNanoAt(unix.AT_FDCWD, path, []unix.Timespec{atimeTimespec, mtimeTimespec}, unix.AT_SYMLINK_NOFOLLOW); err == nil || err != syscall.ENOTSUP { + t.Fatalf("Expected ENOTSUP changing the times of a symlink; got %v", err) } }) } diff --git a/internal/sandbox/node.go b/internal/sandbox/node.go index ed80fdb..a5e9ae6 100644 --- a/internal/sandbox/node.go +++ b/internal/sandbox/node.go @@ -342,6 +342,13 @@ func (n *BaseNode) setattrTimes(req *fuse.SetattrRequest) error { } if underlyingPath, isMapped := n.UnderlyingPath(); isMapped && !n.deleted { + if n.attr.Mode&os.ModeType == os.ModeSymlink { + // TODO(https://github.com/bazelbuild/sandboxfs/issues/46): Should use + // futimensat to support changing the times of a symlink if requested to + // do so. + return fuse.ENOTSUP + } + if err := os.Chtimes(underlyingPath, atime, mtime); err != nil { return err } diff --git a/src/nodes/mod.rs b/src/nodes/mod.rs index 9b9ae0f..75e8c6f 100644 --- a/src/nodes/mod.rs +++ b/src/nodes/mod.rs @@ -136,13 +136,14 @@ fn setattr_times(attr: &mut fuse::FileAttr, path: Option<&PathBuf>, return Ok(()); } - let atime = atime.unwrap_or_else(|| conv::timespec_to_timeval(attr.atime)); - let mtime = mtime.unwrap_or_else(|| conv::timespec_to_timeval(attr.mtime)); if attr.kind == fuse::FileType::Symlink { - // TODO(jmmv): Should use futimensat to avoid following symlinks but this function is - // not available on macOS El Capitan, which we currently require for CI builds. - warn!("Asked to update atime/mtime for symlink {:?} but following symlink instead", path); + // TODO(https://github.com/bazelbuild/sandboxfs/issues/46): Should use futimensat to support + // changing the times of a symlink if requested to do so. + return Err(nix::Error::from_errno(Errno::ENOTSUP)); } + + let atime = atime.unwrap_or_else(|| conv::timespec_to_timeval(attr.atime)); + let mtime = mtime.unwrap_or_else(|| conv::timespec_to_timeval(attr.mtime)); let result = try_path(path, |p| sys::stat::utimes(p, &atime, &mtime)); if result.is_ok() { attr.atime = conv::timeval_to_timespec(atime);