Add FGGlobals::get/set_download_dir(), unify TerraSync and download dirs setup

Similar to the existing FGGlobals::get/set_terrasync_dir(), add
FGGlobals::get_download_dir() and FGGlobals::set_download_dir() methods,
and of course the corresponding FGGlobals::download_dir public member
variable. FGGlobals::set_download_dir() stores the realpath() of the
given directory, including into the /sim/paths/download-dir property,
which is marked as read-only just as /sim/terrasync/scenery-dir already
is.

Handle the setup of the TerraSync and download dirs all in the same
place (Options::processOptions()), since most of the work is already
done there. This allows one to get rid of fgOptTerrasyncDir() and
fgOptDownloadDir(), and to make it easier to see that
globals->set_terrasync_dir() (resp. globals->set_download_dir()) is
called on the correct SGPath, regardless of whether --terrasync-dir
(resp. --download-dir) was passed.

Always create the TerraSync and download dirs when they don't already
exist, regardless of whether --terrasync-dir or --download-dir has been
given on the command line.

Add comments explaining how to avoid security pitfalls with download and
TerraSync dirs (cf. discussion around
<https://sourceforge.net/p/flightgear/mailman/message/35461636/>).

Adjust indentation where it was too broken, hampering readbility.

src/Main/globals.cxx

@@ -416,6 +416,31 @@ void FGGlobals::clear_fg_scenery()
 
 }
 
+// The 'path' argument to this method must come from trustworthy code, because
+// the method grants read permissions to Nasal code for many files beneath
+// 'path'. In particular, don't call this method with a 'path' value taken
+// from the property tree or any other Nasal-writable place.
+void FGGlobals::set_download_dir(const SGPath& path)
+{
+  SGPath abspath(path.realpath());
+  download_dir = abspath;
+
+  append_read_allowed_paths(abspath / "Aircraft");
+  append_read_allowed_paths(abspath / "AI");
+  append_read_allowed_paths(abspath / "Liveries");
+  // If in use, abspath / TerraSync will be added to 'extra_read_allowed_paths'
+  // by FGGlobals::append_fg_scenery(), as any scenery path.
+
+  SGPropertyNode *n = fgGetNode("/sim/paths/download-dir", true);
+  n->setAttribute(SGPropertyNode::WRITE, true);
+  n->setStringValue(abspath.utf8Str());
+  n->setAttribute(SGPropertyNode::WRITE, false);
+}
+
+// The 'path' argument to this method must come from trustworthy code, because
+// the method grants read permissions to Nasal code for all files beneath
+// 'path'. In particular, don't call this method with a 'path' value taken
+// from the property tree or any other Nasal-writable place.
 void FGGlobals::set_terrasync_dir(const SGPath &path)
 {
   if (terrasync_dir.realpath() != SGPath(fgGetString("/sim/terrasync/scenery-dir")).realpath()) {

src/Main/globals.hxx

@@ -104,7 +104,9 @@ private:
     
     // Users home directory for data
     SGPath fg_home;
-    //Terrasync directory
+    // Download directory
+    SGPath download_dir;
+    // Terrasync directory
     SGPath terrasync_dir;
 
     // Roots of FlightGear scenery tree
@@ -221,7 +223,20 @@ public:
     const SGPath &get_fg_home () const { return fg_home; }
     void set_fg_home (const SGPath &home);
 
+    const SGPath &get_download_dir () const { return download_dir; }
+    // The 'path' argument to set_download_dir() must come from trustworthy
+    // code, because the method grants read permissions to Nasal code for many
+    // files beneath 'path'. In particular, don't call this method with a
+    // 'path' value taken from the property tree or any other Nasal-writable
+    // place.
+    void set_download_dir (const SGPath &path);
+
     const SGPath &get_terrasync_dir () const { return terrasync_dir; }
+    // The 'path' argument to set_terrasync_dir() must come from trustworthy
+    // code, because the method grants read permissions to Nasal code for all
+    // files beneath 'path'. In particular, don't call this method with a
+    // 'path' value taken from the property tree or any other Nasal-writable
+    // place.
     void set_terrasync_dir (const SGPath &path);
 
     const PathList &get_fg_scenery () const { return fg_scenery; }

src/Main/options.cxx

@@ -856,26 +856,6 @@ fgOptFgScenery( const char *arg )
     return FG_OPTIONS_OK;
 }
 
-static int
-fgOptTerrasyncDir( const char *arg )
-{
-    SGPath p = SGPath::fromLocal8Bit(arg);
-    globals->set_terrasync_dir(p);
-    return FG_OPTIONS_OK;
-}
-
-static int
-fgOptDownloadDir( const char *arg )
-{
-    SGPath p = SGPath::fromLocal8Bit(arg);
-    globals->append_read_allowed_paths(p / "Aircraft");
-    globals->append_read_allowed_paths(p / "AI");
-    globals->append_read_allowed_paths(p / "Liveries");
-    // p / TerraSync is allowed later if in use
-    fgSetString("/sim/paths/download-dir", p.utf8Str());
-    return FG_OPTIONS_OK;
-}
-
 static int
 fgOptAllowNasalRead( const char *arg )
 {
@@ -1627,8 +1607,8 @@ struct OptionDesc {
     {"materials-file",               true,  OPTION_STRING, "/sim/rendering/materials-file", false, "", 0 },
     {"disable-terrasync",            false, OPTION_BOOL,   "/sim/terrasync/enabled", false, "", 0 },
     {"enable-terrasync",             false, OPTION_BOOL,   "/sim/terrasync/enabled", true, "", 0 },
-    {"terrasync-dir",                true,  OPTION_FUNC,   "", false, "", fgOptTerrasyncDir },
+    {"terrasync-dir",                true,  OPTION_IGNORE,   "", false, "", 0 },
-    {"download-dir",                 true,  OPTION_FUNC,   "", false, "", fgOptDownloadDir },
+    {"download-dir",                 true,  OPTION_IGNORE,   "", false, "", 0 },
     {"allow-nasal-read",             true,  OPTION_FUNC | OPTION_MULTI,   "", false, "", fgOptAllowNasalRead },
     {"geometry",                     true,  OPTION_FUNC,   "", false, "", fgOptGeometry },
     {"bpp",                          true,  OPTION_FUNC,   "", false, "", fgOptBpp },
@@ -2354,36 +2334,54 @@ OptionResult Options::processOptions()
       globals->append_fg_scenery(SGPath::pathsFromEnv("FG_SCENERY"));
   }
 
-// download dir fix-up
+    // Download dir fix-up
-    SGPath downloadDir = SGPath::fromLocal8Bit(valueForOption("download-dir").c_str());
+    SGPath downloadDir = SGPath::fromLocal8Bit(
+      valueForOption("download-dir").c_str());
     if (downloadDir.isNull()) {
         downloadDir = defaultDownloadDir();
-        SG_LOG(SG_GENERAL, SG_INFO, "Using default download dir: " << downloadDir);
+        SG_LOG(SG_GENERAL, SG_INFO,
+               "Using default download dir: " << downloadDir);
     } else {
-        simgear::Dir d(downloadDir);
+      SG_LOG(SG_GENERAL, SG_INFO,
-        if (!d.exists()) {
+             "Using explicit download dir: " << downloadDir);
-            SG_LOG(SG_GENERAL, SG_INFO, "Creating requested download dir: " << downloadDir);
-            d.create(0755);
-        }
     }
 
-// terrasync directory fixup
+    simgear::Dir d(downloadDir);
-    SGPath terrasyncDir = globals->get_terrasync_dir();
+    if (!d.exists()) {
-  if (terrasyncDir.isNull()) {
+      SG_LOG(SG_GENERAL, SG_INFO,
-      SGPath p(downloadDir);
+             "Creating download dir: " << downloadDir);
-      p.append("TerraSync");
+      d.create(0755);
-      terrasyncDir = p;
+    }
 
-      simgear::Dir d(terrasyncDir);
+    // This is safe because the value of 'downloadDir' is trustworthy. In
-      if (!d.exists()) {
+    // particular, it can't be influenced by Nasal code, not even indirectly
-          d.create(0755);
+    // via a Nasal-writable place such as the property tree.
-      }
+    globals->set_download_dir(downloadDir);
 
-	  SG_LOG(SG_GENERAL, SG_INFO, "Using default TerraSync: " << terrasyncDir);
+    // TerraSync directory fixup
-      globals->set_terrasync_dir(p);
+    SGPath terrasyncDir = SGPath::fromLocal8Bit(
-  } else {
+      valueForOption("terrasync-dir").c_str());
-      SG_LOG(SG_GENERAL, SG_INFO, "Using explicit TerraSync dir: " << terrasyncDir);
+    if (terrasyncDir.isNull()) {
-  }
+      terrasyncDir = downloadDir / "TerraSync";
+      // No “default” qualifier here, because 'downloadDir' may be non-default
+      SG_LOG(SG_GENERAL, SG_INFO,
+             "Using TerraSync dir: " << terrasyncDir);
+    } else {
+      SG_LOG(SG_GENERAL, SG_INFO,
+             "Using explicit TerraSync dir: " << terrasyncDir);
+    }
+
+    d = simgear::Dir(terrasyncDir);
+    if (!d.exists()) {
+      SG_LOG(SG_GENERAL, SG_INFO,
+             "Creating TerraSync dir: " << terrasyncDir);
+      d.create(0755);
+    }
+
+    // This is safe because the value of 'terrasyncDir' is trustworthy. In
+    // particular, it can't be influenced by Nasal code, not even indirectly
+    // via a Nasal-writable place such as the property tree.
+    globals->set_terrasync_dir(terrasyncDir);
 
     // check if we setup a scenery path so far
     bool addFGDataScenery = globals->get_fg_scenery().empty();