Merge branch 'next' of https://git.code.sf.net/p/flightgear/simgear into next

simgear/package/Catalog.cxx

@@ -108,12 +108,12 @@ public:
     }
 
 protected:
-    virtual void gotBodyData(const char* s, int n)
+    void gotBodyData(const char* s, int n) override
     {
         m_buffer += std::string(s, n);
     }
 
-    virtual void onDone()
+    void onDone() override
     {
         if (responseCode() != 200) {
             Delegate::StatusCode code = Delegate::FAIL_DOWNLOAD;
@@ -157,6 +157,13 @@ protected:
 
             return;
         } // of version check failed
+        
+        // validate what we downloaded, in case it's now corrupted
+        // (i.e someone uploaded bad XML data)
+        if (!m_owner->validatePackages()) {
+            m_owner->refreshComplete(Delegate::FAIL_VALIDATION);
+            return;
+        }
 
         // cache the catalog data, now we have a valid install root
         Dir d(m_owner->installRoot());
@@ -233,7 +240,9 @@ CatalogRef Catalog::createFromPath(Root* aRoot, const SGPath& aPath)
     c->parseProps(props);
     c->parseTimestamp();
 
-    if (versionCheckOk) {
+    if (!c->validatePackages()) {
+        c->changeStatus(Delegate::FAIL_VALIDATION);
+    } else if (versionCheckOk) {
         // parsed XML ok, mark status as valid
         c->changeStatus(Delegate::STATUS_SUCCESS);
     } else {
@@ -242,25 +251,44 @@ CatalogRef Catalog::createFromPath(Root* aRoot, const SGPath& aPath)
 
     return c;
 }
+    
+bool Catalog::validatePackages() const
+{
+    for (auto pack : packages()) {
+        if (!pack->validate()) {
+            SG_LOG(SG_GENERAL, SG_WARN, "Catalog " << id() << " failed validation due to invalid package:" << pack->id());
+            return false;
+        }
+    }
+    
+    return true;
+}
 
 bool Catalog::uninstall()
 {
     bool ok;
     bool atLeastOneFailure = false;
 
-    BOOST_FOREACH(PackageRef p, installedPackages()) {
+    try {
-        ok = p->existingInstall()->uninstall();
+        // clean uninstall of each airacft / package in turn. This is
-        if (!ok) {
+        // slightly overkill since we then nuke the entire catalog
-            SG_LOG(SG_GENERAL, SG_WARN, "uninstall of package " <<
+        // directory anyway
-                p->id() << " failed");
+        for (PackageRef p : installedPackages()) {
-            // continue trying other packages, bailing out here
+            ok = p->existingInstall()->uninstall();
-            // gains us nothing
+            if (!ok) {
-            atLeastOneFailure = true;
+                SG_LOG(SG_GENERAL, SG_WARN, "uninstall of package " <<
+                    p->id() << " failed");
+                // continue trying other packages, bailing out here
+                // gains us nothing
+                atLeastOneFailure = true;
+            }
         }
+    } catch (sg_exception& e) {
+        SG_LOG(SG_GENERAL, SG_WARN, "uninstall of catalog failed " << e.getMessage() << ", will clean-up directory");
+        atLeastOneFailure = true;
     }
 
-    Dir d(m_installRoot);
+    ok = removeDirectory();
-    ok = d.remove(true /* recursive */);
     if (!ok) {
         atLeastOneFailure = true;
     }
@@ -270,6 +298,15 @@ bool Catalog::uninstall()
     return ok;
 }
 
+bool Catalog::removeDirectory()
+{
+    Dir d(m_installRoot);
+    if (!m_installRoot.exists())
+        return true;
+    
+    return d.remove(true /* recursive */);
+}
+    
 PackageList const&
 Catalog::packages() const
 {

simgear/package/Catalog.hxx

@@ -155,7 +155,8 @@ private:
 
     class Downloader;
     friend class Downloader;
-
+    friend class Root;
+    
     void parseProps(const SGPropertyNode* aProps);
 
     void refreshComplete(Delegate::StatusCode aReason);
@@ -163,6 +164,17 @@ private:
     void parseTimestamp();
     void writeTimestamp();
 
+    /**
+     * @brief wipe the catalog directory from the disk
+     */
+    bool removeDirectory();
+    
+    /**
+     * @brief Helper to ensure all packages are at least somewhat valid, in terms
+     * of an ID, name and directory.
+     */
+    bool validatePackages() const;
+    
     std::string getLocalisedString(const SGPropertyNode* aRoot, const char* aName) const;
 
     void changeStatus(Delegate::StatusCode newStatus);

simgear/package/CatalogTest.cxx

@@ -84,6 +84,14 @@ public:
                 path = ss.str();
             }
         }
+        
+        if (path == "/catalogTestInvalid/catalog.xml") {
+            if (global_catalogVersion > 0) {
+                std::stringstream ss;
+                ss << "/catalogTestInvalid/catalog-v" << global_catalogVersion << ".xml";
+                path = ss.str();
+            }
+        }
 
         localPath.append(path);
 
@@ -124,6 +132,12 @@ void waitForUpdateComplete(HTTP::Client* cl, pkg::Root* root)
     std::cerr << "timed out" << std::endl;
 }
 
+template<class T>
+bool vectorContains(const std::vector<T>& vec, const T value)
+{
+    return std::find(vec.begin(), vec.end(), value) != vec.end();
+}
+
 int parseTest()
 {
     SGPath rootPath = simgear::Dir::current().path();
@@ -725,6 +739,127 @@ void testOfflineMode(HTTP::Client* cl)
     global_failRequests = false;
 }
 
+int parseInvalidTest()
+{
+    SGPath rootPath = simgear::Dir::current().path();
+    rootPath.append("testRoot");
+    pkg::Root* root = new pkg::Root(rootPath, "8.1.12");
+    pkg::CatalogRef cat = pkg::Catalog::createFromPath(root, SGPath(SRC_DIR "/catalogTestInvalid"));
+    SG_VERIFY(cat.valid());
+
+    SG_CHECK_EQUAL(cat->status(), pkg::Delegate::FAIL_VALIDATION);
+ 
+    return 0;
+}
+
+void removeInvalidCatalog(HTTP::Client* cl)
+{
+    global_catalogVersion = 0; // fetch the good version
+    
+    SGPath rootPath(simgear::Dir::current().path());
+    rootPath.append("cat_remove_invalid");
+    simgear::Dir pd(rootPath);
+    pd.removeChildren();
+    
+    pkg::RootRef root(new pkg::Root(rootPath, "8.1.2"));
+    root->setHTTPClient(cl);
+    
+    // another catalog so the dicts are non-empty
+    pkg::CatalogRef anotherCat = pkg::Catalog::createFromUrl(root.ptr(), "http://localhost:2000/catalogTest1/catalog.xml");
+
+    pkg::CatalogRef c = pkg::Catalog::createFromUrl(root.ptr(), "http://localhost:2000/catalogTestInvalid/catalog.xml");
+    waitForUpdateComplete(cl, root);
+    SG_VERIFY(!c->isEnabled());
+    SG_VERIFY(c->status() == pkg::Delegate::FAIL_VALIDATION);
+    SG_VERIFY(!vectorContains(root->catalogs(), c));
+    SG_VERIFY(vectorContains(root->allCatalogs(), c));
+ 
+    // now remove it
+    root->removeCatalog(c);
+    SG_VERIFY(!vectorContains(root->catalogs(), c));
+    SG_VERIFY(!vectorContains(root->allCatalogs(), c));
+    c.clear(); // drop the catalog
+
+    // re-add it again, and remove it again
+    {
+        pkg::CatalogRef c2 = pkg::Catalog::createFromUrl(root.ptr(), "http://localhost:2000/catalogTestInvalid/catalog.xml");
+        waitForUpdateComplete(cl, root);
+        SG_VERIFY(!c2->isEnabled());
+        SG_VERIFY(c2->status() == pkg::Delegate::FAIL_VALIDATION);
+        SG_VERIFY(!vectorContains(root->catalogs(), c2));
+        SG_VERIFY(vectorContains(root->allCatalogs(), c2));
+        
+        // now remove it
+        root->removeCatalog(c2);
+        SG_VERIFY(!vectorContains(root->catalogs(), c2));
+        SG_VERIFY(!vectorContains(root->allCatalogs(), c2));
+    }
+    
+    // only the other catalog (testCatalog should be left)
+    SG_VERIFY(root->allCatalogs().size() == 1);
+    SG_VERIFY(root->catalogs().size() == 1);
+    
+    SG_LOG(SG_GENERAL, SG_INFO, "Remove invalid catalog test passeed");
+}
+
+void updateInvalidToValid(HTTP::Client* cl)
+{
+    global_catalogVersion = 0;
+    SGPath rootPath(simgear::Dir::current().path());
+    rootPath.append("cat_update_invalid_to_valid");
+    simgear::Dir pd(rootPath);
+    pd.removeChildren();
+    
+// first, sync the invalid version
+    pkg::RootRef root(new pkg::Root(rootPath, "8.1.2"));
+    root->setHTTPClient(cl);
+    
+    pkg::CatalogRef c = pkg::Catalog::createFromUrl(root.ptr(), "http://localhost:2000/catalogTestInvalid/catalog.xml");
+    waitForUpdateComplete(cl, root);
+    SG_VERIFY(!c->isEnabled());
+    
+    SG_VERIFY(c->status() == pkg::Delegate::FAIL_VALIDATION);
+    SG_VERIFY(!vectorContains(root->catalogs(), c));
+    SG_VERIFY(vectorContains(root->allCatalogs(), c));
+
+// now refrsh the good one
+    global_catalogVersion = 2;
+    c->refresh();
+    waitForUpdateComplete(cl, root);
+    SG_VERIFY(c->isEnabled());
+    SG_VERIFY(c->status() == pkg::Delegate::STATUS_REFRESHED);
+    SG_VERIFY(vectorContains(root->catalogs(), c));
+
+}
+
+void updateValidToInvalid(HTTP::Client* cl)
+{
+    global_catalogVersion = 2; // fetch the good version
+    
+    SGPath rootPath(simgear::Dir::current().path());
+    rootPath.append("cat_update_valid_to_invalid");
+    simgear::Dir pd(rootPath);
+    pd.removeChildren();
+    
+    // first, sync the invalid version
+    pkg::RootRef root(new pkg::Root(rootPath, "8.1.2"));
+    root->setHTTPClient(cl);
+    
+    pkg::CatalogRef c = pkg::Catalog::createFromUrl(root.ptr(), "http://localhost:2000/catalogTestInvalid/catalog.xml");
+    waitForUpdateComplete(cl, root);
+    SG_VERIFY(c->isEnabled());
+    SG_VERIFY(c->status() == pkg::Delegate::STATUS_REFRESHED);
+    SG_VERIFY(vectorContains(root->catalogs(), c));
+    SG_VERIFY(vectorContains(root->allCatalogs(), c));
+    
+    // now refrsh the bad one
+    global_catalogVersion = 3;
+    c->refresh();
+    waitForUpdateComplete(cl, root);
+    SG_VERIFY(!c->isEnabled());
+    SG_VERIFY(c->status() == pkg::Delegate::FAIL_VALIDATION);
+    SG_VERIFY(!vectorContains(root->catalogs(), c));
+}
 
 int main(int argc, char* argv[])
 {
@@ -739,6 +874,8 @@ int main(int argc, char* argv[])
 
     parseTest();
 
+    parseInvalidTest();
+    
     testInstallPackage(&cl);
 
     testUninstall(&cl);
@@ -755,6 +892,11 @@ int main(int argc, char* argv[])
     
     testVersionMigrate(&cl);
     
-    std::cout << "Successfully passed all tests!" << std::endl;
+    updateInvalidToValid(&cl);
+    updateValidToInvalid(&cl);
+    
+    removeInvalidCatalog(&cl);
+    
+    SG_LOG(SG_GENERAL, SG_INFO, "Successfully passed all tests!");
     return EXIT_SUCCESS;
 }

simgear/package/Delegate.hxx

@@ -54,6 +54,7 @@ public:
         FAIL_VERSION,       ///< version check mismatch
         FAIL_NOT_FOUND,     ///< package URL returned a 404
         FAIL_HTTP_FORBIDDEN, ///< URL returned a 403. Marked specially to catch rate-limiting
+        FAIL_VALIDATION,    ///< catalog or package failed to validate
         STATUS_REFRESHED,
         USER_CANCELLED
     } StatusCode;

simgear/package/Package.cxx

@@ -497,6 +497,23 @@ Package::PreviewVec Package::previewsFromProps(const SGPropertyNode_ptr& ptr) co
     return result;
 }
 
+bool Package::validate() const
+{
+    if (m_id.empty())
+        return false;
+    
+    std::string nm(m_props->getStringValue("name"));
+    if (nm.empty())
+        return false;
+    
+    std::string dir(m_props->getStringValue("dir"));
+    if (dir.empty())
+        return false;
+    
+    return true;
+}
+
+    
 } // of namespace pkg
 
 } // of namespace simgear

simgear/package/Package.hxx

@@ -225,6 +225,11 @@ private:
 
     void updateFromProps(const SGPropertyNode* aProps);
 
+    /**
+     * @brief check the Package passes some basic consistence checks
+     */
+    bool validate() const;
+    
     std::string getLocalisedString(const SGPropertyNode* aRoot, const char* aName) const;
 
     PreviewVec previewsFromProps(const SGPropertyNode_ptr& ptr) const;

simgear/package/Root.cxx

@@ -413,6 +413,8 @@ Root::Root(const SGPath& aPath, const std::string& aVersion) :
         auto cat = Catalog::createFromPath(this, c);
         if (cat && cat->isEnabled()) {
             d->catalogs.insert({cat->id(), cat});
+        } else if (cat) {
+            SG_LOG(SG_GENERAL, SG_WARN, "Package-Root init: catalog is disabled: " << cat->id());
         }
     } // of child directories iteration
 }
@@ -713,6 +715,32 @@ void Root::catalogRefreshStatus(CatalogRef aCat, Delegate::StatusCode aReason)
         d->firePackagesChanged();
     }
 }
+    
+bool Root::removeCatalog(CatalogRef cat)
+{
+    if (!cat)
+        return false;
+    
+    // normal remove path
+    if (!cat->id().empty()) {
+        return removeCatalogById(cat->id());
+    }
+    
+    if (!cat->removeDirectory()) {
+        SG_LOG(SG_GENERAL, SG_WARN, "removeCatalog: failed to remove directory " << cat->installRoot());
+    }
+    auto it = std::find(d->disabledCatalogs.begin(),
+                       d->disabledCatalogs.end(),
+                       cat);
+    if (it != d->disabledCatalogs.end()) {
+        d->disabledCatalogs.erase(it);
+    }
+    
+    // notify that a catalog is being removed
+    d->firePackagesChanged();
+    
+    return true;
+}
 
 bool Root::removeCatalogById(const std::string& aId)
 {
@@ -736,10 +764,10 @@ bool Root::removeCatalogById(const std::string& aId)
         d->catalogs.erase(catIt);
     }
 
-    bool ok = cat->uninstall();
+    bool ok = cat->removeDirectory();
     if (!ok) {
         SG_LOG(SG_GENERAL, SG_WARN, "removeCatalogById: catalog :" << aId
-            << "failed to uninstall");
+            << "failed to remove directory");
     }
 
     // notify that a catalog is being removed

simgear/package/Root.hxx

@@ -140,6 +140,13 @@ public:
      */
     bool removeCatalogById(const std::string& aId);
     
+    /**
+     * remove a catalog by reference (used when abandoning installs, since
+     * there may not be a valid catalog Id)
+     */
+    bool removeCatalog(CatalogRef cat);
+
+    
     /**
      * request thumbnail data from the cache / network
      */

simgear/package/catalogTestInvalid/catalog-v2.xml

@@ -0,0 +1,22 @@
+<?xml version="1.0"?>
+
+<PropertyList>
+    <id>org.flightgear.test.catalog-invalid</id>
+    <description>Invalid test catalog</description>
+    <url>http://localhost:2000/catalogTestInvalid/catalog.xml</url>
+    <catalog-version>4</catalog-version>
+
+    <version>8.1.*</version>
+    <version>8.0.0</version>
+    <version>8.2.0</version>
+
+    <package>
+        <id>alpha</id>
+        <!-- fixed now -->
+        <name>Alpha aircraft</name>
+        <dir>alpha</dir>
+
+    </package>
+
+   
+</PropertyList>

simgear/package/catalogTestInvalid/catalog-v3.xml

@@ -0,0 +1,21 @@
+<?xml version="1.0"?>
+
+<PropertyList>
+    <id>org.flightgear.test.catalog-invalid</id>
+    <description>Invalid test catalog</description>
+    <url>http://localhost:2000/catalogTestInvalid/catalog.xml</url>
+    <catalog-version>4</catalog-version>
+
+    <version>8.1.*</version>
+    <version>8.0.0</version>
+    <version>8.2.0</version>
+
+    <package>
+        <id>alpha</id>
+        <!-- missing name -->
+        <dir>alpha</dir>
+
+    </package>
+
+   
+</PropertyList>

simgear/package/catalogTestInvalid/catalog.xml

@@ -0,0 +1,21 @@
+<?xml version="1.0"?>
+
+<PropertyList>
+    <id>org.flightgear.test.catalog-invalid</id>
+    <description>Invalid test catalog</description>
+    <url>http://localhost:2000/catalogTestInvalid/catalog.xml</url>
+    <catalog-version>4</catalog-version>
+
+    <version>8.1.*</version>
+    <version>8.0.0</version>
+    <version>8.2.0</version>
+
+    <package>
+        <id>alpha</id>
+        <!-- missing name -->
+        <dir>alpha</dir>
+
+    </package>
+
+   
+</PropertyList>