[5/5] mesos git commit: Stop using os::system to chown a directory hierarchy.

[ya...@apache.org]

Stop using os::system to chown a directory hierarchy.

Reimplement os::chown() to use fts(3) rather than sometimes spawning
chown(1). This removes the use of the shell and the corresponding
need to sanitize path arguments.  It also enables us to provide
consistent handling of symbolic links for the recursive and
non-recursive cases. We ensure that symlinks are never followed
and that we always change the ownership of the link itself, not
its referent.

Review: https://reviews.apache.org/r/55242/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/de300908
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/de300908
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/de300908

Branch: refs/heads/master
Commit: de300908c1b2ff43b3847c4f76986960d98ee5f9
Parents: 5034292
Author: James Peach <jp...@apache.org>
Authored: Tue Jan 17 17:55:23 2017 -0800
Committer: Jiang Yan Xu <xu...@apple.com>
Committed: Tue Jan 17 17:59:13 2017 -0800

----------------------------------------------------------------------
 3rdparty/stout/include/stout/os/posix/chown.hpp | 65 ++++++++++---
 3rdparty/stout/include/stout/os/posix/stat.hpp  | 25 +++++
 3rdparty/stout/tests/os_tests.cpp               | 96 ++++++++++++++++++++
 3 files changed, 172 insertions(+), 14 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/de300908/3rdparty/stout/include/stout/os/posix/chown.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/posix/chown.hpp b/3rdparty/stout/include/stout/os/posix/chown.hpp
index c82e2e5..e2214ac 100644
--- a/3rdparty/stout/include/stout/os/posix/chown.hpp
+++ b/3rdparty/stout/include/stout/os/posix/chown.hpp
@@ -13,6 +13,7 @@
 #ifndef __STOUT_OS_POSIX_CHOWN_HPP__
 #define __STOUT_OS_POSIX_CHOWN_HPP__
 
+#include <fts.h>
 #include <sys/types.h>
 #include <pwd.h>
 
@@ -21,33 +22,69 @@
 #include <stout/try.hpp>
 
 #include <stout/os/shell.hpp>
+#include <stout/os/stat.hpp>
 
 namespace os {
 
+// Set the ownership for a path. This function never follows any symlinks.
 inline Try<Nothing> chown(
     uid_t uid,
     gid_t gid,
     const std::string& path,
     bool recursive)
 {
-  if (recursive) {
-    // TODO(bmahler): Consider walking the file tree instead. We would need
-    // to be careful to not miss dotfiles.
-    std::string command =
-      "chown -R " + stringify(uid) + ':' + stringify(gid) + " '" + path + "'";
-
-    int status = os::system(command);
-    if (status != 0) {
-      return ErrnoError(
-          "Failed to execute '" + command +
-          "' (exit status: " + stringify(status) + ")");
+  char* path_[] = {const_cast<char*>(path.c_str()), nullptr};
+
+  FTS* tree = ::fts_open(
+      path_, FTS_NOCHDIR | FTS_PHYSICAL, nullptr);
+
+  if (tree == nullptr) {
+    return ErrnoError();
+  }
+
+  FTSENT *node;
+  while ((node = ::fts_read(tree)) != nullptr) {
+    switch (node->fts_info) {
+      // Preorder directory.
+      case FTS_D:
+      // Regular file.
+      case FTS_F:
+      // Symbolic link.
+      case FTS_SL:
+      // Symbolic link without target.
+      case FTS_SLNONE: {
+        if (::lchown(node->fts_path, uid, gid) < 0) {
+          Error error = ErrnoError();
+          ::fts_close(tree);
+          return error;
+        }
+
+        break;
+      }
+
+      // Unreadable directory.
+      case FTS_DNR:
+      // Error; errno is set.
+      case FTS_ERR:
+      // Directory that causes cycles.
+      case FTS_DC:
+      // `stat(2)` failed.
+      case FTS_NS: {
+        Error error = ErrnoError();
+        ::fts_close(tree);
+        return error;
+      }
+
+      default:
+        break;
     }
-  } else {
-    if (::chown(path.c_str(), uid, gid) < 0) {
-      return ErrnoError();
+
+    if (node->fts_level == FTS_ROOTLEVEL && !recursive) {
+      break;
     }
   }
 
+  ::fts_close(tree);
   return Nothing();
 }
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/de300908/3rdparty/stout/include/stout/os/posix/stat.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/posix/stat.hpp b/3rdparty/stout/include/stout/os/posix/stat.hpp
index 1ab20e7..e7440a4 100644
--- a/3rdparty/stout/include/stout/os/posix/stat.hpp
+++ b/3rdparty/stout/include/stout/os/posix/stat.hpp
@@ -168,6 +168,31 @@ inline Try<ino_t> inode(const std::string& path)
   return s.st_ino;
 }
 
+
+inline Try<uid_t> uid(
+    const std::string& path,
+    const FollowSymlink follow = FOLLOW_SYMLINK)
+{
+  struct stat s;
+
+  switch (follow) {
+    case DO_NOT_FOLLOW_SYMLINK:
+      if (::lstat(path.c_str(), &s) == 0) {
+        return s.st_uid;
+      }
+      break;
+    case FOLLOW_SYMLINK:
+      if (::stat(path.c_str(), &s) == 0) {
+        return s.st_uid;
+      }
+      break;
+    default:
+      UNREACHABLE();
+  }
+
+  return ErrnoError("Error invoking stat for '" + path + "'");
+}
+
 } // namespace stat {
 
 } // namespace os {

http://git-wip-us.apache.org/repos/asf/mesos/blob/de300908/3rdparty/stout/tests/os_tests.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/tests/os_tests.cpp b/3rdparty/stout/tests/os_tests.cpp
index 30735e2..31a4c8f 100644
--- a/3rdparty/stout/tests/os_tests.cpp
+++ b/3rdparty/stout/tests/os_tests.cpp
@@ -723,6 +723,102 @@ TEST_F(OsTest, User)
   EXPECT_SOME(os::setgroups(gids.get(), uid.get()));
   EXPECT_SOME(os::setuid(uid.get()));
 }
+
+
+TEST_F(OsTest, Chown)
+{
+  using os::stat::DO_NOT_FOLLOW_SYMLINK;
+
+  Result<uid_t> uid = os::getuid();
+  ASSERT_SOME(uid);
+
+  // 'chown' requires root permission.
+  if (uid.get() != 0) {
+    return;
+  }
+
+  // In the following tests, we chown to an artitrary UID. There is
+  // no special significance to the value 9.
+
+  // Set up a simple directory hierarchy.
+  EXPECT_SOME(os::mkdir("chown/one/two/three"));
+  EXPECT_SOME(os::touch("chown/one/file"));
+  EXPECT_SOME(os::touch("chown/one/two/file"));
+  EXPECT_SOME(os::touch("chown/one/two/three/file"));
+
+  // Make a symlink back to the top of the tree so we can verify
+  // that it isn't followed.
+  EXPECT_SOME(fs::symlink("../../../../chown", "chown/one/two/three/link"));
+
+  // Make a symlink to the middle of the tree and verify that chowning the
+  // symlink does not chown that subtree.
+  EXPECT_SOME(fs::symlink("chown/one/two", "two.link"));
+  EXPECT_SOME(os::chown(9, 9, "two.link", true));
+  EXPECT_SOME_EQ(9u, os::stat::uid("two.link", DO_NOT_FOLLOW_SYMLINK));
+  EXPECT_SOME_EQ(0u, os::stat::uid("chown", DO_NOT_FOLLOW_SYMLINK));
+  EXPECT_SOME_EQ(0u,
+      os::stat::uid("chown/one/two/three/file", DO_NOT_FOLLOW_SYMLINK));
+
+  // Recursively chown the whole tree.
+  EXPECT_SOME(os::chown(9, 9, "chown", true));
+  EXPECT_SOME_EQ(9u, os::stat::uid("chown", DO_NOT_FOLLOW_SYMLINK));
+  EXPECT_SOME_EQ(9u,
+      os::stat::uid("chown/one/two/three/file", DO_NOT_FOLLOW_SYMLINK));
+  EXPECT_SOME_EQ(9u,
+      os::stat::uid("chown/one/two/three/link", DO_NOT_FOLLOW_SYMLINK));
+
+  // Chown the subtree with the embedded link back and verify that it
+  // doesn't follow back to the top of the tree.
+  EXPECT_SOME(os::chown(0, 0, "chown/one/two/three", true));
+  EXPECT_SOME_EQ(9u, os::stat::uid("chown", DO_NOT_FOLLOW_SYMLINK));
+  EXPECT_SOME_EQ(0u,
+      os::stat::uid("chown/one/two/three", DO_NOT_FOLLOW_SYMLINK));
+  EXPECT_SOME_EQ(0u,
+      os::stat::uid("chown/one/two/three/link", DO_NOT_FOLLOW_SYMLINK));
+
+  // Verify that non-recursive chown changes the directory and not
+  // its contents.
+  EXPECT_SOME(os::chown(0, 0, "chown/one", false));
+  EXPECT_SOME_EQ(0u, os::stat::uid("chown/one", DO_NOT_FOLLOW_SYMLINK));
+  EXPECT_SOME_EQ(9u, os::stat::uid("chown/one/file", DO_NOT_FOLLOW_SYMLINK));
+}
+
+
+TEST_F(OsTest, ChownNoAccess)
+{
+  Result<uid_t> uid = os::getuid();
+  Result<gid_t> gid = os::getgid();
+
+  ASSERT_SOME(uid);
+  ASSERT_SOME(gid);
+
+  // This test requires that we not be root, since root will
+  // bypass access checks.
+  if (uid.get() == 0) {
+    return;
+  }
+
+  ASSERT_SOME(os::mkdir("one/two"));
+
+  // Chown to ourself should be a noop.
+  EXPECT_SOME(os::chown(uid.get(), gid.get(), "one/two", true));
+
+  ASSERT_SOME(os::chmod("one/two", 0));
+
+  // Recursive chown should now fail to fully recurse due to
+  // the lack of permission on "one/two".
+  EXPECT_ERROR(os::chown(uid.get(), gid.get(), "one", true));
+  EXPECT_ERROR(os::chown(uid.get(), gid.get(), "one/two", true));
+
+  // A non-recursive should succeed when the child is not traversable.
+  EXPECT_SOME(os::chown(uid.get(), gid.get(), "one", false));
+
+  // A non-recursive should succeed on the non-traversable child too.
+  EXPECT_SOME(os::chown(uid.get(), gid.get(), "one/two", false));
+
+  // Restore directory access so the rmdir can work.
+  ASSERT_SOME(os::chmod("one/two", 0755));
+}
 #endif // __WINDOWS__