From 279797083742f227f16f5a9c09952a334ac5f381 Mon Sep 17 00:00:00 2001 From: James Turner Date: Sun, 8 Apr 2018 23:57:24 +0100 Subject: [PATCH] Catalogs: better invalid data handling --- simgear/package/Catalog.cxx | 63 ++++++-- simgear/package/Catalog.hxx | 14 +- simgear/package/CatalogTest.cxx | 144 +++++++++++++++++- simgear/package/Delegate.hxx | 1 + simgear/package/Package.cxx | 17 +++ simgear/package/Package.hxx | 5 + simgear/package/Root.cxx | 32 +++- simgear/package/Root.hxx | 7 + .../package/catalogTestInvalid/catalog-v2.xml | 22 +++ .../package/catalogTestInvalid/catalog-v3.xml | 21 +++ .../package/catalogTestInvalid/catalog.xml | 21 +++ 11 files changed, 330 insertions(+), 17 deletions(-) create mode 100644 simgear/package/catalogTestInvalid/catalog-v2.xml create mode 100644 simgear/package/catalogTestInvalid/catalog-v3.xml create mode 100644 simgear/package/catalogTestInvalid/catalog.xml diff --git a/simgear/package/Catalog.cxx b/simgear/package/Catalog.cxx index 21d25bb5..de74d19d 100644 --- a/simgear/package/Catalog.cxx +++ b/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()) { - ok = p->existingInstall()->uninstall(); - if (!ok) { - SG_LOG(SG_GENERAL, SG_WARN, "uninstall of package " << - p->id() << " failed"); - // continue trying other packages, bailing out here - // gains us nothing - atLeastOneFailure = true; + try { + // clean uninstall of each airacft / package in turn. This is + // slightly overkill since we then nuke the entire catalog + // directory anyway + for (PackageRef p : installedPackages()) { + ok = p->existingInstall()->uninstall(); + if (!ok) { + 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 = d.remove(true /* recursive */); + ok = removeDirectory(); 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 { diff --git a/simgear/package/Catalog.hxx b/simgear/package/Catalog.hxx index 513d5b43..c0a6edd8 100644 --- a/simgear/package/Catalog.hxx +++ b/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); diff --git a/simgear/package/CatalogTest.cxx b/simgear/package/CatalogTest.cxx index 4e3e8e1b..3f603fa4 100644 --- a/simgear/package/CatalogTest.cxx +++ b/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 +bool vectorContains(const std::vector& 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; } diff --git a/simgear/package/Delegate.hxx b/simgear/package/Delegate.hxx index 9e75b4de..4f92abca 100644 --- a/simgear/package/Delegate.hxx +++ b/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; diff --git a/simgear/package/Package.cxx b/simgear/package/Package.cxx index f802f6b3..17613093 100644 --- a/simgear/package/Package.cxx +++ b/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 diff --git a/simgear/package/Package.hxx b/simgear/package/Package.hxx index 0c5daa6c..5f696dcc 100644 --- a/simgear/package/Package.hxx +++ b/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; diff --git a/simgear/package/Root.cxx b/simgear/package/Root.cxx index e4c895a8..24ed0d55 100644 --- a/simgear/package/Root.cxx +++ b/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 diff --git a/simgear/package/Root.hxx b/simgear/package/Root.hxx index 3f461740..70461915 100644 --- a/simgear/package/Root.hxx +++ b/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 */ diff --git a/simgear/package/catalogTestInvalid/catalog-v2.xml b/simgear/package/catalogTestInvalid/catalog-v2.xml new file mode 100644 index 00000000..3b1daf18 --- /dev/null +++ b/simgear/package/catalogTestInvalid/catalog-v2.xml @@ -0,0 +1,22 @@ + + + + org.flightgear.test.catalog-invalid + Invalid test catalog + http://localhost:2000/catalogTestInvalid/catalog.xml + 4 + + 8.1.* + 8.0.0 + 8.2.0 + + + alpha + + Alpha aircraft + alpha + + + + + diff --git a/simgear/package/catalogTestInvalid/catalog-v3.xml b/simgear/package/catalogTestInvalid/catalog-v3.xml new file mode 100644 index 00000000..7197eecb --- /dev/null +++ b/simgear/package/catalogTestInvalid/catalog-v3.xml @@ -0,0 +1,21 @@ + + + + org.flightgear.test.catalog-invalid + Invalid test catalog + http://localhost:2000/catalogTestInvalid/catalog.xml + 4 + + 8.1.* + 8.0.0 + 8.2.0 + + + alpha + + alpha + + + + + diff --git a/simgear/package/catalogTestInvalid/catalog.xml b/simgear/package/catalogTestInvalid/catalog.xml new file mode 100644 index 00000000..7197eecb --- /dev/null +++ b/simgear/package/catalogTestInvalid/catalog.xml @@ -0,0 +1,21 @@ + + + + org.flightgear.test.catalog-invalid + Invalid test catalog + http://localhost:2000/catalogTestInvalid/catalog.xml + 4 + + 8.1.* + 8.0.0 + 8.2.0 + + + alpha + + alpha + + + + +