Fix memory leak due to complement repository cycles

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<repository>& r: cs)
+      for (const lazy_weak_ptr<repository>& 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<repository>& cr: cs)
+      for (const lazy_weak_ptr<repository>& 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<lazy_shared_ptr<repository>, compare_lazy_ptr>;
-    using prerequisites_type =
-      std::set<lazy_weak_ptr<repository>, 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<lazy_weak_ptr<repository>, 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<repository_fragment> root (
           db.load<repository_fragment> (""));
 
-        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<repository_fragment>& 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<repository>& x,
-                   const lazy_weak_ptr<repository>& y)
-               {
-                 return x.object_id () == y.object_id ();
-               });
+    {
+      auto eq = [] (const lazy_weak_ptr<repository>& x,
+                    const lazy_weak_ptr<repository>& 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<repository>& rp)
+        const lazy_weak_ptr<repository>& rp)
       {
         shared_ptr<repository> r (rp.load ());
         if (fetched_repositories.find (r) == fetched_repositories.end ())
           removed_repositories.insert (move (r));
       };
 
-      for (const lazy_shared_ptr<repository>& cr: old_complements)
+      for (const lazy_weak_ptr<repository>& 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<repository>& pr: old_prerequisites)
-        rm (lazy_shared_ptr<repository> (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<string> 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<repository>& r: ua)
-        repos.push_back (r);
+      for (const lazy_weak_ptr<repository>& r: ua)
+        repos.push_back (lazy_shared_ptr<repository> (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<repository>& rp: fr->complements)
+        for (const lazy_weak_ptr<repository>& rp: fr->complements)
         {
           // Skip the root complement (see rep_fetch() for details).
           //
@@ -114,7 +114,7 @@ namespace bpkg
 
     shared_ptr<repository_fragment> root (db.load<repository_fragment> (""));
 
-    for (const lazy_shared_ptr<repository>& rp: root->complements)
+    for (const lazy_weak_ptr<repository>& rp: root->complements)
     {
       shared_ptr<repository> 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<repository>& rp)
+    auto remove = [&c, &db, &t] (const lazy_weak_ptr<repository>& rp)
     {
-      shared_ptr<repository> r (db.find<repository> (rp.object_id ()));
-
-      if (r)
+      if (shared_ptr<repository> r = db.find<repository> (rp.object_id ()))
         rep_remove (c, t, r);
     };
 
-    for (const lazy_shared_ptr<repository>& cr: rf->complements)
+    for (const lazy_weak_ptr<repository>& 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<repository>& pr: rf->prerequisites)
-      remove (lazy_shared_ptr<repository> (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<repository_fragment>::name != "");
 
       shared_ptr<repository_fragment> root (db.load<repository_fragment> (""));
-      repository_fragment::complements_type& ua (root->complements);
+      repository_fragment::dependencies& ua (root->complements);
 
       for (shared_ptr<repository> r: pointer_result (db.query<repository> ()))
       {
         if (r->name == "")
           l5 ([&]{trace << "skipping root repository";});
-        else if (ua.find (lazy_shared_ptr<repository> (db, r)) != ua.end ())
+        else if (ua.find (lazy_weak_ptr<repository> (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<repository_fragment> root (db.load<repository_fragment> (""));
-    repository_fragment::complements_type& ua (root->complements);
+    repository_fragment::dependencies& ua (root->complements);
 
     if (o.all ())
     {
-      for (const lazy_shared_ptr<repository>& r: ua)
-        repos.push_back (r);
+      for (const lazy_weak_ptr<repository>& r: ua)
+        repos.push_back (lazy_shared_ptr<repository> (r));
     }
     else
     {
@@ -435,11 +433,11 @@ namespace bpkg
             repository_url u (a);
             assert (!u.empty ());
 
-            for (const lazy_shared_ptr<repository>& rp: ua)
+            for (const lazy_weak_ptr<repository>& rp: ua)
             {
               if (rp.load ()->location.url () == u)
               {
-                r = rp;
+                r = lazy_shared_ptr<repository> (rp);
                 break;
               }
             }