From 24f18983923cc63706e3c5c8a5db6bb073e7afb7 Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Mon, 9 Jul 2018 22:05:53 +0300 Subject: Fix memory leak due to complement repository cycles --- bpkg/package.cxx | 4 ++-- bpkg/package.hxx | 26 ++++++++++++++------------ bpkg/rep-fetch.cxx | 50 ++++++++++++++++++++++++++++---------------------- bpkg/rep-list.cxx | 4 ++-- bpkg/rep-remove.cxx | 24 +++++++++++------------- 5 files changed, 57 insertions(+), 51 deletions(-) diff --git a/bpkg/package.cxx b/bpkg/package.cxx index dea16f2..66e9ba1 100644 --- a/bpkg/package.cxx +++ b/bpkg/package.cxx @@ -88,7 +88,7 @@ namespace bpkg return i.fragment == lrf; }; - for (const lazy_shared_ptr& r: cs) + for (const lazy_weak_ptr& r: cs) { const auto& frs (r.load ()->fragments); @@ -110,7 +110,7 @@ namespace bpkg // Finally, load the complements and prerequisites and check them // recursively. // - for (const lazy_shared_ptr& cr: cs) + for (const lazy_weak_ptr& cr: cs) { for (const auto& fr: cr.load ()->fragments) { diff --git a/bpkg/package.hxx b/bpkg/package.hxx index ea48045..8c8c661 100644 --- a/bpkg/package.hxx +++ b/bpkg/package.hxx @@ -266,16 +266,6 @@ namespace bpkg class repository_fragment { public: - // We use a weak pointer for prerequisite repositories because we could - // have cycles. No cycles in complements, thought. - // - // Note that these point to repositories, not repository fragments. - // - using complements_type = - std::set, compare_lazy_ptr>; - using prerequisites_type = - std::set, compare_lazy_ptr>; - // Repository fragment id is a repository canonical name that identifies // just this fragment (for example, for git it is a canonical name of // the repository URL with the full, non-abbreviated commit id). @@ -291,8 +281,20 @@ namespace bpkg // repository_location location; - complements_type complements; - prerequisites_type prerequisites; + // We use a weak pointer for prerequisite repositories because we could + // have cycles. + // + // Note that we could have cycles for complements via the root repository + // that is the default complement for dir and git repositories (see + // rep-fetch for details), and so we use a weak pointer for complements + // either. + // + // Also note that these point to repositories, not repository fragments. + // + using dependencies = std::set, compare_lazy_ptr>; + + dependencies complements; + dependencies prerequisites; public: explicit diff --git a/bpkg/rep-fetch.cxx b/bpkg/rep-fetch.cxx index b905a7b..abc43fc 100644 --- a/bpkg/rep-fetch.cxx +++ b/bpkg/rep-fetch.cxx @@ -642,7 +642,7 @@ namespace bpkg shared_ptr root ( db.load ("")); - repository_fragment::complements_type& ua (root->complements); + repository_fragment::dependencies& ua (root->complements); if (ua.find (r) == ua.end ()) { @@ -907,12 +907,12 @@ namespace bpkg // shallow repository fetch is possible and to register them for removal // if that's not the case. // - repository_fragment::complements_type old_complements; - repository_fragment::prerequisites_type old_prerequisites; + repository_fragment::dependencies old_complements; + repository_fragment::dependencies old_prerequisites; auto collect_deps = [] (const shared_ptr& rf, - repository_fragment::complements_type& cs, - repository_fragment::prerequisites_type& ps) + repository_fragment::dependencies& cs, + repository_fragment::dependencies& ps) { for (const auto& cr: rf->complements) cs.insert (cr); @@ -947,8 +947,8 @@ namespace bpkg // r->certificate = move (rfd.certificate_pem); - repository_fragment::complements_type new_complements; - repository_fragment::prerequisites_type new_prerequisites; + repository_fragment::dependencies new_complements; + repository_fragment::dependencies new_prerequisites; repository_trust repo_trust; for (rep_fetch_data::fragment& fr: rfd.fragments) @@ -990,14 +990,20 @@ namespace bpkg // directly. // if (shallow) - shallow = new_complements == old_complements && - equal (new_prerequisites.begin (), new_prerequisites.end (), - old_prerequisites.begin (), old_prerequisites.end (), - [] (const lazy_weak_ptr& x, - const lazy_weak_ptr& y) - { - return x.object_id () == y.object_id (); - }); + { + auto eq = [] (const lazy_weak_ptr& x, + const lazy_weak_ptr& y) + { + return x.object_id () == y.object_id (); + }; + + shallow = equal (new_complements.begin (), new_complements.end (), + old_complements.begin (), old_complements.end (), + eq) && + equal (new_prerequisites.begin (), new_prerequisites.end (), + old_prerequisites.begin (), old_prerequisites.end (), + eq); + } // Fetch prerequisites and complements, unless this is a shallow fetch. // @@ -1007,14 +1013,14 @@ namespace bpkg // unless they are fetched. // auto rm = [&fetched_repositories, &removed_repositories] ( - const lazy_shared_ptr& rp) + const lazy_weak_ptr& rp) { shared_ptr r (rp.load ()); if (fetched_repositories.find (r) == fetched_repositories.end ()) removed_repositories.insert (move (r)); }; - for (const lazy_shared_ptr& cr: old_complements) + for (const lazy_weak_ptr& cr: old_complements) { // Remove the complement unless it is the root repository (see // rep_fetch() for details). @@ -1024,7 +1030,7 @@ namespace bpkg } for (const lazy_weak_ptr& pr: old_prerequisites) - rm (lazy_shared_ptr (pr)); + rm (pr); auto fetch = [&co, &conf, @@ -1239,7 +1245,7 @@ namespace bpkg // User-added repos. // - repository_fragment::complements_type& ua (root->complements); + repository_fragment::dependencies& ua (root->complements); for (const repository_location& rl: rls) { @@ -1279,7 +1285,7 @@ namespace bpkg // User-added repos. // - repository_fragment::complements_type& ua (root->complements); + repository_fragment::dependencies& ua (root->complements); optional reason; bool full_fetch (!args.more ()); @@ -1290,8 +1296,8 @@ namespace bpkg fail << "configuration " << c << " has no repositories" << info << "use 'bpkg rep-add' to add a repository"; - for (const lazy_shared_ptr& r: ua) - repos.push_back (r); + for (const lazy_weak_ptr& r: ua) + repos.push_back (lazy_shared_ptr (r)); // Always print "fetching ..." for complements of the root, even if // there is only one. diff --git a/bpkg/rep-list.cxx b/bpkg/rep-list.cxx index e186665..71a66e4 100644 --- a/bpkg/rep-list.cxx +++ b/bpkg/rep-list.cxx @@ -65,7 +65,7 @@ namespace bpkg if (o.complements ()) { - for (const lazy_shared_ptr& rp: fr->complements) + for (const lazy_weak_ptr& rp: fr->complements) { // Skip the root complement (see rep_fetch() for details). // @@ -114,7 +114,7 @@ namespace bpkg shared_ptr root (db.load ("")); - for (const lazy_shared_ptr& rp: root->complements) + for (const lazy_weak_ptr& rp: root->complements) { shared_ptr r (rp.load ()); cout << r->location.canonical_name () << " " << r->location << endl; diff --git a/bpkg/rep-remove.cxx b/bpkg/rep-remove.cxx index c070418..5a7f8da 100644 --- a/bpkg/rep-remove.cxx +++ b/bpkg/rep-remove.cxx @@ -247,15 +247,13 @@ namespace bpkg // Prior to removing a prerequisite/complement we need to make sure it // still exists, which may not be the case due to the dependency cycle. // - auto remove = [&c, &db, &t] (const lazy_shared_ptr& rp) + auto remove = [&c, &db, &t] (const lazy_weak_ptr& rp) { - shared_ptr r (db.find (rp.object_id ())); - - if (r) + if (shared_ptr r = db.find (rp.object_id ())) rep_remove (c, t, r); }; - for (const lazy_shared_ptr& cr: rf->complements) + for (const lazy_weak_ptr& cr: rf->complements) { // Remove the complement unless it is the root repository (see // rep_fetch() for details). @@ -265,7 +263,7 @@ namespace bpkg } for (const lazy_weak_ptr& pr: rf->prerequisites) - remove (lazy_shared_ptr (pr)); + remove (pr); // If there are no repositories stayed in the database then no repository // fragments nor packages should stay either. @@ -301,13 +299,13 @@ namespace bpkg query::name != ""); shared_ptr root (db.load ("")); - repository_fragment::complements_type& ua (root->complements); + repository_fragment::dependencies& ua (root->complements); for (shared_ptr r: pointer_result (db.query ())) { if (r->name == "") l5 ([&]{trace << "skipping root repository";}); - else if (ua.find (lazy_shared_ptr (db, r)) != ua.end ()) + else if (ua.find (lazy_weak_ptr (db, r)) != ua.end ()) { r->fragments.clear (); db.update (r); @@ -395,12 +393,12 @@ namespace bpkg session s; // Repository dependencies can have cycles. shared_ptr root (db.load ("")); - repository_fragment::complements_type& ua (root->complements); + repository_fragment::dependencies& ua (root->complements); if (o.all ()) { - for (const lazy_shared_ptr& r: ua) - repos.push_back (r); + for (const lazy_weak_ptr& r: ua) + repos.push_back (lazy_shared_ptr (r)); } else { @@ -435,11 +433,11 @@ namespace bpkg repository_url u (a); assert (!u.empty ()); - for (const lazy_shared_ptr& rp: ua) + for (const lazy_weak_ptr& rp: ua) { if (rp.load ()->location.url () == u) { - r = rp; + r = lazy_shared_ptr (rp); break; } } -- cgit v1.1