Added startup error for bad temporary directory setting. (#9551)

* Added startup error for bad temporary directory setting.

  If the temporary directory (--temp.path) setting is identical to the database
  directory (`--database.directory`) this can eventually lead to data loss, as
  temporary files may be created inside the temporary directory, causing overwrites of
  existing database files/directories with the same names.
  Additionally the temporary directory may be cleaned at some point, and this would lead
  to an unintended cleanup of the database files/directories as well.
  Now, if the database directory and temporary directory are set to the same path, there
  will be a startup warning about potential data loss (though in ArangoDB 3.4 allowing to
  continue the startup - in 3.5 and higher we will abort the startup).

* fix failing tests

CHANGELOG

@@ -1,6 +1,18 @@
 v3.5.0-rc.6 (2019-XX-XX)
 ------------------------
 
+* Added startup error for bad temporary directory setting.
+
+  If the temporary directory (--temp.path) setting is identical to the database 
+  directory (`--database.directory`) this can eventually lead to data loss, as 
+  temporary files may be created inside the temporary directory, causing overwrites of 
+  existing database files/directories with the same names.
+  Additionally the temporary directory may be cleaned at some point, and this would lead 
+  to an unintended cleanup of the database files/directories as well.
+  Now, if the database directory and temporary directory are set to the same path, there 
+  will be a startup warning about potential data loss (though in ArangoDB 3.4 allowing to 
+  continue the startup - in 3.5 and higher we will abort the startup).
+
 * Make TTL indexes behave like other indexes on creation
     
   If a TTL index is already present on a collection, the previous behavior

arangod/RestServer/DatabasePathFeature.cpp

@@ -23,6 +23,7 @@
 #include "DatabasePathFeature.h"
 
 #include "ApplicationFeatures/ApplicationServer.h"
+#include "ApplicationFeatures/TempFeature.h"
 #include "Basics/ArangoGlobalContext.h"
 #include "Basics/FileUtils.h"
 #include "Basics/StringUtils.h"
@@ -94,7 +95,7 @@ void DatabasePathFeature::validateOptions(std::shared_ptr<ProgramOptions> option
   auto ctx = ArangoGlobalContext::CONTEXT;
 
   if (ctx == nullptr) {
-    LOG_TOPIC("19066", ERR, arangodb::Logger::FIXME) << "failed to get global context.";
+    LOG_TOPIC("19066", FATAL, arangodb::Logger::FIXME) << "failed to get global context.";
     FATAL_ERROR_EXIT();
   }
 
@@ -102,6 +103,28 @@ void DatabasePathFeature::validateOptions(std::shared_ptr<ProgramOptions> option
 }
 
 void DatabasePathFeature::prepare() {
+  // check if temporary directory and database directory are identical
+  {
+    std::string directoryCopy = _directory;
+    basics::FileUtils::makePathAbsolute(directoryCopy);
+
+    auto* tf = application_features::ApplicationServer::lookupFeature<TempFeature>("Temp");
+    if (tf) {
+      // the feature is not present in unit tests, so make the execution depend
+      // on whether the feature is available
+      std::string tempPathCopy = tf->path();
+      basics::FileUtils::makePathAbsolute(tempPathCopy);
+      tempPathCopy = basics::StringUtils::rTrim(tempPathCopy, TRI_DIR_SEPARATOR_STR);
+
+      if (directoryCopy == tempPathCopy) {
+        LOG_TOPIC("fd70b", FATAL, arangodb::Logger::FIXME) 
+          << "database directory '" << directoryCopy << "' is identical to the temporary directory. "
+          << "This can cause follow-up problems, including data loss. Please review your setup!";
+        FATAL_ERROR_EXIT();
+      }
+    }
+  }
+
   if (_requiredDirectoryState == "any") {
     // database directory can have any state. this is the default
     return;

js/common/modules/@arangodb/foxx/store.js

@@ -91,85 +91,75 @@ var updateFishbowlFromZip = function (filename) {
   var tempPath = fs.getTempPath();
   var toSave = [];
 
-  try {
-    fs.makeDirectoryRecursive(tempPath);
-    var root = fs.join(tempPath, 'foxx-apps-master/applications');
+  fs.makeDirectoryRecursive(tempPath);
+  var root = fs.join(tempPath, 'foxx-apps-master/applications');
 
-    // remove any previous files in the directory
-    fs.listTree(root).forEach(function (file) {
-      if (file.match(/\.json$/)) {
-        try {
-          fs.remove(fs.join(root, file));
-        } catch (ignore) {}
+  // remove any previous files in the directory
+  fs.listTree(root).forEach(function (file) {
+    if (file.match(/\.json$/)) {
+      try {
+        fs.remove(fs.join(root, file));
+      } catch (ignore) {}
+    }
+  });
+
+  fs.unzipFile(filename, tempPath, false, true);
+
+  if (!fs.exists(root)) {
+    throw new Error("'applications' directory is missing in foxx-apps-master, giving up");
+  }
+
+  var m = fs.listTree(root);
+  var reSub = /(.*)\.json$/;
+  var f, match, service, desc;
+
+  for (i = 0; i < m.length; ++i) {
+    f = m[i];
+    match = reSub.exec(f);
+    if (match === null) {
+      continue;
+    }
+
+    service = fs.join(root, f);
+
+    try {
+      desc = JSON.parse(fs.read(service));
+    } catch (err1) {
+      arangodb.printf("Cannot parse description for service '" + f + "': %s\n", String(err1));
+      continue;
+    }
+
+    desc._key = match[1];
+
+    if (!desc.hasOwnProperty('name')) {
+      desc.name = match[1];
+    }
+
+    toSave.push(desc);
+  }
+
+  if (toSave.length > 0) {
+    var fishbowl = getFishbowlStorage();
+
+    db._executeTransaction({
+      collections: {
+        exclusive: fishbowl.name()
+      },
+      action: function (params) {
+        var c = require('internal').db._collection(params.collection);
+        c.truncate();
+
+        params.services.forEach(function (service) {
+          c.save(service);
+        });
+      },
+      params: {
+        services: toSave,
+        collection: fishbowl.name()
       }
     });
 
-    fs.unzipFile(filename, tempPath, false, true);
-
-    if (!fs.exists(root)) {
-      throw new Error("'applications' directory is missing in foxx-apps-master, giving up");
-    }
-
-    var m = fs.listTree(root);
-    var reSub = /(.*)\.json$/;
-    var f, match, service, desc;
-
-    for (i = 0; i < m.length; ++i) {
-      f = m[i];
-      match = reSub.exec(f);
-      if (match === null) {
-        continue;
-      }
-
-      service = fs.join(root, f);
-
-      try {
-        desc = JSON.parse(fs.read(service));
-      } catch (err1) {
-        arangodb.printf("Cannot parse description for service '" + f + "': %s\n", String(err1));
-        continue;
-      }
-
-      desc._key = match[1];
-
-      if (!desc.hasOwnProperty('name')) {
-        desc.name = match[1];
-      }
-
-      toSave.push(desc);
-    }
-
-    if (toSave.length > 0) {
-      var fishbowl = getFishbowlStorage();
-
-      db._executeTransaction({
-        collections: {
-          exclusive: fishbowl.name()
-        },
-        action: function (params) {
-          var c = require('internal').db._collection(params.collection);
-          c.truncate();
-
-          params.services.forEach(function (service) {
-            c.save(service);
-          });
-        },
-        params: {
-          services: toSave,
-          collection: fishbowl.name()
-        }
-      });
-
-      require('console').debug('Updated local Foxx repository with ' + toSave.length + ' service(s)');
-    }
-  } catch (err) {
-    if (tempPath !== undefined && tempPath !== '') {
-      try {
-        fs.removeDirectoryRecursive(tempPath);
-      } catch (ignore) {}
-    }
-
-    throw err;
+    require('console').debug('Updated local Foxx repository with ' + toSave.length + ' service(s)');
   }
 };
 

lib/ApplicationFeatures/TempFeature.h

@@ -36,6 +36,9 @@ class TempFeature final : public application_features::ApplicationFeature {
   void prepare() override final;
   void start() override final;
 
+  std::string path() const { return _path; }
+
+ private:
   std::string _path;
   std::string _appname;
 };