From 8c2bdc3c0896d3acec21b4744bff65c8cfa4b418 Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Mon, 5 Nov 2018 06:21:50 -0500 Subject: [PATCH] Return EOPNOTSUPP 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 EOPNOTSUPP 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..f1c064f 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.EOPNOTSUPP { + t.Fatalf("Expected EOPNOTSUPP changing the times of a symlink; got %v", err) } }) } diff --git a/internal/sandbox/node.go b/internal/sandbox/node.go index ed80fdb..0b28617 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.Errno(syscall.EOPNOTSUPP) + } + if err := os.Chtimes(underlyingPath, atime, mtime); err != nil { return err } diff --git a/src/nodes/mod.rs b/src/nodes/mod.rs index 07cd4ce..becfdb4 100644 --- a/src/nodes/mod.rs +++ b/src/nodes/mod.rs @@ -143,13 +143,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::EOPNOTSUPP)); } + + 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);