[setup topic/libsolv] Crash after incomplete download

[Jon Turney]

On 01/11/2017 20:38, Ken Brown wrote:
> If there is a download failure and the user clicks Yes in response to 
> "Download Incomplete.  Try again?", then setup will crash.  The crash 
> occurs at PickView.cc:447 because i->source() is NULL.

Thanks for finding all these problems!

> I haven't yet analyzed this in further detail, but the crux of the issue 
> seems to be that we call do_ini_thread a second time without having 
> cleaned out the package database and the libsolv pool.
> 
> This is probably a symptom of a more general problem, which is that we 
> haven't thought carefully (or at least I haven't) about what happens 
> when we visit certain pages for a second time, after the libsolv pool 
> has been created.

The fact that this isn't considered at all at the moment makes me wonder 
if it's working correctly in this situation currently.  But, yeah, I 
agree that packagedb state should be cleared in this case.

I tried with the attached, but it still segfaults at the same place.

This seems to be because all the packagedb prep (including 
fixup_source_package_ids) is in OnInit() (which is only called once per 
page, but lazily), rather than OnActivate(), so probably some more 
restructuring is needed there :(

> Before I work further on this, I have a UI question.  Is it really 
> reasonable that we go back to the mirror selection page after "Download 
> incomplete"?  I understand the rationale, which is that the user might 
> want to try a different mirror after a download failure.  But I 
> personally have always found this annoying.  I would rather just retry 
> the download, which is what the message ("Download Incomplete.  Try 
> again?") suggests is going to happen.

This makes perfect sense to me.

The path that an unattended install takes out of there is particularly 
convoluted, as well.

I suspect there's other terribleness in this area, like if you choose 
"No", the incomplete package download is left in the local package cache 
to fail it's checksum check on the next run...
-------------- next part --------------
From baec018e93c0e7cea7993c22a5e32f2065a1e7ae Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Thu, 2 Nov 2017 14:15:04 +0000
Subject: [PATCH setup] Empty packagedb and underlying solver pool if we go
 back to WM_APP_START_SETUP_INI_DOWNLOAD

XXX: this is probably still wrong as we can bypass this state?
---
 ini.cc        |  3 +++
 libsolv.cc    | 15 +++++++++++++++
 libsolv.h     |  2 ++
 package_db.cc | 13 +++++++++++++
 package_db.h  |  1 +
 5 files changed, 34 insertions(+)

diff --git a/ini.cc b/ini.cc
index 0f8b927..e2ab08f 100644
--- a/ini.cc
+++ b/ini.cc
@@ -346,6 +346,9 @@ do_remote_ini (HWND owner)
 static bool
 do_ini_thread (HINSTANCE h, HWND owner)
 {
+  packagedb db;
+  db.init();
+
   size_t ini_count = 0;
   if (source == IDC_SOURCE_LOCALDIR)
     ini_count = do_local_ini (owner);
diff --git a/libsolv.cc b/libsolv.cc
index 9dd1eeb..7eecc29 100644
--- a/libsolv.cc
+++ b/libsolv.cc
@@ -316,6 +316,12 @@ void debug_callback(Pool *pool, void *data, int type, const char *str)
 }
 
 SolverPool::SolverPool()
+{
+  init();
+}
+
+void
+SolverPool::init()
 {
   /* create a pool */
   pool = pool_create();
@@ -333,6 +339,15 @@ SolverPool::SolverPool()
   pool_set_installed(pool, installed->repo);
 }
 
+void
+SolverPool::clear()
+{
+  repos.clear();
+  pool_free(pool);
+
+  init();
+}
+
 SolvRepo *
 SolverPool::getRepo(const std::string &name, bool test)
 {
diff --git a/libsolv.h b/libsolv.h
index 65e1610..366cd59 100644
--- a/libsolv.h
+++ b/libsolv.h
@@ -130,6 +130,7 @@ class SolverPool
 {
 public:
   SolverPool();
+  void clear();
   SolvRepo *getRepo(const std::string &name, bool test = false);
 
   // Utility class for passing arguments to addPackage()
@@ -158,6 +159,7 @@ public:
 
 
 private:
+  void init();
   Id makedeps(Repo *repo, PackageDepends *requires);
   Pool *pool;
 
diff --git a/package_db.cc b/package_db.cc
index 66e7f0a..5bf9009 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -47,6 +47,19 @@ packagedb::packagedb ()
 {
 }
 
+void
+packagedb::init ()
+{
+  installeddbread = 0;
+  installeddbver = 0;
+  packages.clear();
+  sourcePackages.clear();
+  categories.clear();
+  solver.clear();
+  basepkg = packageversion();
+  dependencyOrderedPackages.clear();
+}
+
 void
 packagedb::read ()
 {
diff --git a/package_db.h b/package_db.h
index d7127a6..a26c387 100644
--- a/package_db.h
+++ b/package_db.h
@@ -64,6 +64,7 @@ class packagedb
 {
 public:
   packagedb ();
+  void init();
   void read();
   void makeBase();
   /* 0 on success */
-- 
2.15.0