MGM: Make "move directory into subdirectory of itself" protection robust

Fixes EOS-2850.

Author: gbitzes

.gitignore

@@ -41,3 +41,4 @@ kineticio-dist.tgz
 /.settings/
 .cproject
 .project
+.vscode

.ignore

@@ -3,3 +3,4 @@ common/fmt/
 namespace/ns_quarkdb/qclient/src/fmt/
 common/sqlite/
 man/man1/
+.vscode

mgm/XrdMgmOfs.cc

@@ -36,6 +36,7 @@
 #include "namespace/Constants.hh"
 #include "namespace/interface/ContainerIterators.hh"
 #include "namespace/utils/Checksum.hh"
+#include "namespace/utils/RenameSafetyCheck.hh"
 #include "authz/XrdCapability.hh"
 #include "mgm/Stat.hh"
 #include "mgm/Access.hh"

mgm/XrdMgmOfs/Rename.cc

@@ -424,6 +424,11 @@ XrdMgmOfs::_rename(const char* old_name,
       if (renameDir) {
         rdir = dir->findContainer(oPath.GetName());
 
+        if(!eos::isSafeToRename(gOFS->eosView, rdir.get(), newdir.get())) {
+          errno = EINVAL;
+          return Emsg(epname, error, EINVAL, "rename - old path is subpath of new path");
+        }
+
         if (rdir) {
           // Remove all the quota from the source node and add to the target node
           std::map<std::string, std::set<std::string> >::const_reverse_iterator rfoundit;
@@ -574,6 +579,13 @@ XrdMgmOfs::_rename(const char* old_name,
             gOFS->FuseXCastContainer(rdir->getIdentifier());
             gOFS->FuseXCastContainer(rdir->getParentIdentifier());
           } else {
+            // Do the check once again, because we're paranoid
+            if(!eos::isSafeToRename(gOFS->eosView, rdir.get(), newdir.get())) {
+              eos_static_crit("%s", SSTR("Unsafe rename of container " << rdir->getId() << " -> " << newdir->getId() << " was prevented at the last resort check"));
+              errno = EINVAL;
+              return Emsg(epname, error, EINVAL, "rename - old path is subpath of new path - caught by last resort check, quotanodes may have become inconsistent");
+            }
+
             // Remove from one container to another one
             unsigned long long tree_size = rdir->getTreeSize();
             {

namespace/ns_quarkdb/tests/HierarchicalViewTest.cc

@@ -29,6 +29,7 @@
 #include "namespace/utils/TestHelpers.hh"
 #include "namespace/utils/RmrfHelper.hh"
 #include "namespace/Resolver.hh"
+#include "namespace/utils/RenameSafetyCheck.hh"
 #include <algorithm>
 #include <cstdint>
 #include <memory>
@@ -515,3 +516,16 @@ TEST_F(HierarchicalViewF, LostContainerTest)
   // TODO(gbitzes): Something wrong is here, this should succeed, investigate.
   // eos::RmrfHelper::nukeDirectory(view(), "/test/");
 }
+
+TEST_F(HierarchicalViewF, RenameDirectoryAsSubdirOfItself)
+{
+  std::shared_ptr<eos::IContainerMD> cont1 = view()->createContainer("/eos/dev/my-dir", true);
+  std::shared_ptr<eos::IContainerMD> cont2 = view()->createContainer("/eos/dev/my-dir/subdir1", true);
+  std::shared_ptr<eos::IContainerMD> cont3 = view()->createContainer("/eos/dev/my-dir/subdir1/subdir2", true);
+
+  ASSERT_TRUE(eos::isSafeToRename(view(), cont3.get(), cont1.get()));
+  ASSERT_FALSE(eos::isSafeToRename(view(), cont1.get(), cont3.get()));
+
+  ASSERT_TRUE(eos::isSafeToRename(view(), cont2.get(), cont1.get())); // non-sensical to do, but safe (no-op)
+  ASSERT_FALSE(eos::isSafeToRename(view(), cont1.get(), cont2.get()));
+}

namespace/utils/RenameSafetyCheck.hh

@@ -0,0 +1,81 @@
+/************************************************************************
+ * EOS - the CERN Disk Storage System                                   *
+ * Copyright (C) 2018 CERN/Switzerland                                  *
+ *                                                                      *
+ * This program is free software: you can redistribute it and/or modify *
+ * it under the terms of the GNU General Public License as published by *
+ * the Free Software Foundation, either version 3 of the License, or    *
+ * (at your option) any later version.                                  *
+ *                                                                      *
+ * This program is distributed in the hope that it will be useful,      *
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of       *
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the        *
+ * GNU General Public License for more details.                         *
+ *                                                                      *
+ * You should have received a copy of the GNU General Public License    *
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.*
+ ************************************************************************/
+
+//------------------------------------------------------------------------------
+//! @author Georgios Bitzes <[email protected]>
+//! @brief Helper function to check if it's safe to rename a directory into
+//!        another
+//------------------------------------------------------------------------------
+
+#ifndef EOS_NS_RENAME_SAFETY_CHECK_HH
+#define EOS_NS_RENAME_SAFETY_CHECK_HH
+
+#include <iostream>
+#include "namespace/interface/IContainerMD.hh"
+#include "namespace/interface/IView.hh"
+#include "namespace/Namespace.hh"
+#include "common/Logging.hh"
+
+EOSNSNAMESPACE_BEGIN
+
+//------------------------------------------------------------------------------
+// Is it safe to make "source" directory a subdirectory of "target"?
+// Assumes eosViewRWMutex is at-least read-locked when calling this function.
+//------------------------------------------------------------------------------
+bool isSafeToRename(IView *view, IContainerMD *source, IContainerMD *target) {
+  if(source == target) return false;
+
+  IContainerMDSvc *svc = view->getContainerMDSvc();
+  IContainerMDPtr current = svc->getContainerMD(target->getParentId());
+
+  size_t iterations = 0;
+  while(true) {
+    iterations++;
+
+    if(iterations > 1024) {
+      std::string msg = SSTR("potential loop when scanning parents of container "
+      << target->getId() << " - serious namespace corruption");
+
+      eos_static_crit("%s", msg.c_str());
+      throw_mdexception(EFAULT, msg);
+    }
+
+    if(current.get() == source) {
+      return false; // Nope, sound alarm, this rename is not safe
+    }
+
+    if(current->getId() == source->getId()) {
+      // Should not happen.
+      eos_static_crit("%s", SSTR("Two containers with the same ID ended up with different objects in memory - " <<
+      current->getId() << " == " << source->getId() << " - " << current << " vs " << source));
+      return false;
+    }
+
+    if(current->getId() == 1) {
+      // We've reached root, this rename looks safe.
+      return true;
+    }
+
+    // Move up one step.
+    current = svc->getContainerMD(current->getParentId());
+  }
+}
+
+EOSNSNAMESPACE_END
+
+#endif