[setup topic/libsolv] Crash after incomplete download

[setup topic/libsolv] Crash after incomplete download

[Ken Brown]

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.

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.

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.

Ken

Re: [setup topic/libsolv] Crash after incomplete download

[Jon Turney]

[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]

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...

[-- Attachment #2: 0001-Empty-packagedb-and-underlying-solver-pool-if-we-go-.patch --]
[-- Type: text/plain, Size: 2539 bytes --]

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

Re: [setup topic/libsolv] Crash after incomplete download

[Ken Brown]

On 11/2/2017 1:22 PM, Jon Turney wrote:
> 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 :(

I guess we could move all that prep to OnActivate() but make sure it's 
only run when we've gotten to the chooser page from the site page (and 
not because the user pressed Back on the prereq page).  What about 
adding a 'prepped' data member to packagedb and using this to decide 
whether to run the prep code?

>> 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...
> 
> +void
> +packagedb::init ()
> +{
> +  installeddbread = 0;
> +  installeddbver = 0;
> +  packages.clear();
> +  sourcePackages.clear();
> +  categories.clear();
> +  solver.clear();
> +  basepkg = packageversion();
> +  dependencyOrderedPackages.clear();

I think you also need solution.clear(), where the latter does 
essentially what SolverSolution::~SolverSolution() does.  (Or is it 
enough to just set solv = NULL?)  Otherwise, solv will be an invalid 
pointer when update is next called.

Ken

Re: [setup topic/libsolv] Crash after incomplete download

[Jon Turney]

On 02/11/2017 19:25, Ken Brown wrote:
> On 11/2/2017 1:22 PM, Jon Turney wrote:
>> 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 :(
> 
> I guess we could move all that prep to OnActivate() but make sure it's 
> only run when we've gotten to the chooser page from the site page (and 
> not because the user pressed Back on the prereq page).  What about 
> adding a 'prepped' data member to packagedb and using this to decide 
> whether to run the prep code?

An attempt at this to follow, but this ended up touching rather more 
stuff than I wanted to, so I'm sure something else has broken :(

>> +void
>> +packagedb::init ()
>> +{
>> +  installeddbread = 0;
>> +  installeddbver = 0;
>> +  packages.clear();
>> +  sourcePackages.clear();
>> +  categories.clear();
>> +  solver.clear();
>> +  basepkg = packageversion();
>> +  dependencyOrderedPackages.clear();
> 
> I think you also need solution.clear(), where the latter does 
> essentially what SolverSolution::~SolverSolution() does.  (Or is it 
> enough to just set solv = NULL?)  Otherwise, solv will be an invalid 
> pointer when update is next called.

Yes, you're right, of course.

The initialization all gets a bit obscure, maybe making solver and 
solversolution directly members of this singleton isn't the right 
approach...

[PATCH setup 1/5] Make do_ini() succeed if found_ini_list is empty

[Jon Turney]

Rather than count the number of .ini files read in do_{local,remote}_ini,
and assume success if greater than zero, actually track if an error occured.

This is a subtle change of behaviour if more than one .ini file is read:
previously all we needed was one to succeed, now we need them all to
succeed.

(Note that site_list should never be empty, and it's still an error if we
don't find an .ini file from a site, to the practical effect is to make
do_local_ini with an empty found_ini_list succeed)
---
 ini.cc | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/ini.cc b/ini.cc
index 0f8b927..85325c3 100644
--- a/ini.cc
+++ b/ini.cc
@@ -205,10 +205,10 @@ check_ini_sig (io_stream* ini_file, io_stream* ini_sig_file,
   return ini_file;
 }
 
-static int
+static bool
 do_local_ini (HWND owner)
 {
-  size_t ini_count = 0;
+  bool ini_error = false;
   GuiParseFeedback myFeedback;
   IniDBBuilderPackage aBuilder (myFeedback);
   io_stream *ini_file, *ini_sig_file;
@@ -233,6 +233,7 @@ do_local_ini (HWND owner)
 	  // no setup found or signature invalid
 	  note (owner, IDS_SETUPINI_MISSING, SetupBaseName.c_str (),
 		"localdir");
+	  ini_error = true;
 	}
       else
 	{
@@ -245,12 +246,11 @@ do_local_ini (HWND owner)
 	    rfc1738_unescape (current_ini_name.substr (ldl, cap - ldl));
 	  ini_init (ini_file, &aBuilder, myFeedback);
 
-	  /*yydebug = 1; */
-
 	  if (yyparse () || yyerror_count > 0)
-	    myFeedback.error (yyerror_messages);
-	  else
-	    ++ini_count;
+	    {
+	      myFeedback.error (yyerror_messages);
+	      ini_error = true;
+	    }
 
 	  if (aBuilder.timestamp > setup_timestamp)
 	    {
@@ -261,13 +261,13 @@ do_local_ini (HWND owner)
 	  ini_file = NULL;
 	}
     }
-  return ini_count;
+  return ini_error;
 }
 
-static int
+static bool
 do_remote_ini (HWND owner)
 {
-  size_t ini_count = 0;
+  bool ini_error = false;
   GuiParseFeedback myFeedback;
   IniDBBuilderPackage aBuilder (myFeedback);
   io_stream *ini_file = NULL, *ini_sig_file;
@@ -303,6 +303,7 @@ do_remote_ini (HWND owner)
 	{
 	  // no setup found or signature invalid
 	  note (owner, IDS_SETUPINI_MISSING, SetupBaseName.c_str (), n->url.c_str ());
+	  ini_error = true;
 	}
       else
 	{
@@ -311,10 +312,11 @@ do_remote_ini (HWND owner)
 	  aBuilder.parse_mirror = n->url;
 	  ini_init (ini_file, &aBuilder, myFeedback);
 
-	  /*yydebug = 1; */
-
 	  if (yyparse () || yyerror_count > 0)
-	    myFeedback.error (yyerror_messages);
+	    {
+	      myFeedback.error (yyerror_messages);
+	      ini_error = true;
+	    }
 	  else
 	    {
 	      /* save known-good setup.ini locally */
@@ -329,7 +331,6 @@ do_remote_ini (HWND owner)
 		    io_stream::remove (fp);
 		  delete out;
 		}
-	      ++ini_count;
 	    }
 	  if (aBuilder.timestamp > setup_timestamp)
 	    {
@@ -340,19 +341,19 @@ do_remote_ini (HWND owner)
 	  ini_file = NULL;
 	}
     }
-  return ini_count;
+  return ini_error;
 }
 
 static bool
 do_ini_thread (HINSTANCE h, HWND owner)
 {
-  size_t ini_count = 0;
+  bool ini_error = true;
   if (source == IDC_SOURCE_LOCALDIR)
-    ini_count = do_local_ini (owner);
+    ini_error = do_local_ini (owner);
   else
-    ini_count = do_remote_ini (owner);
+    ini_error = do_remote_ini (owner);
 
-  if (ini_count == 0)
+  if (ini_error)
     return false;
 
   if (get_root_dir ().c_str ())
-- 
2.15.0

[PATCH setup 3/5] Ensure packagedb and underlying solver pool is empty before we read setup.ini

[Jon Turney]

We need to make sure the packagedb is empty if we step backwards to here.

v2:
Also clear solver stored in SolverSolution, which holds a pool pointer
---
 ini.cc        |  3 +++
 libsolv.cc    | 22 ++++++++++++++++++++++
 libsolv.h     |  3 +++
 package_db.cc | 14 ++++++++++++++
 package_db.h  |  1 +
 5 files changed, 43 insertions(+)

diff --git a/ini.cc b/ini.cc
index 85325c3..9bd72fb 100644
--- a/ini.cc
+++ b/ini.cc
@@ -347,6 +347,9 @@ do_remote_ini (HWND owner)
 static bool
 do_ini_thread (HINSTANCE h, HWND owner)
 {
+  packagedb db;
+  db.init();
+
   bool ini_error = true;
   if (source == IDC_SOURCE_LOCALDIR)
     ini_error = do_local_ini (owner);
diff --git a/libsolv.cc b/libsolv.cc
index 9dd1eeb..29a26a9 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,16 @@ SolverPool::SolverPool()
   pool_set_installed(pool, installed->repo);
 }
 
+void
+SolverPool::clear()
+{
+  repos.clear();
+  pool_free(pool);
+  pool = NULL;
+
+  init();
+}
+
 SolvRepo *
 SolverPool::getRepo(const std::string &name, bool test)
 {
@@ -583,6 +599,12 @@ SolverPool::use_test_packages(bool use_test_packages)
 // ---------------------------------------------------------------------------
 
 SolverSolution::~SolverSolution()
+{
+  clear();
+}
+
+void
+SolverSolution::clear()
 {
   if (solv)
     {
diff --git a/libsolv.h b/libsolv.h
index 65e1610..cddf76f 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;
 
@@ -236,6 +238,7 @@ class SolverSolution
  public:
   SolverSolution(SolverPool &_pool) : pool(_pool), solv(NULL) {};
   ~SolverSolution();
+  void clear();
 
   /* Reset package database to correspond to trans */
   void trans2db() const;
diff --git a/package_db.cc b/package_db.cc
index 66e7f0a..8fbec44 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -47,6 +47,20 @@ packagedb::packagedb ()
 {
 }
 
+void
+packagedb::init ()
+{
+  installeddbread = 0;
+  installeddbver = 0;
+  packages.clear();
+  sourcePackages.clear();
+  categories.clear();
+  solver.clear();
+  solution.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

[PATCH setup 2/5] Simplify LocalDirPage::OnNext()

[Jon Turney]

Never go directly to IDD_CHOOSE from LocalDirPage::OnNext(), go to
IDD_INSTATUS with WM_APP_START_SETUP_INI_DOWNLOAD (if source ==
IDC_SOURCE_LOCALDIR), otherwise IDD_NET (which eventually leads there)
---
 localdir.cc | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/localdir.cc b/localdir.cc
index 4a7ce2a..0561a30 100644
--- a/localdir.cc
+++ b/localdir.cc
@@ -266,21 +266,18 @@ LocalDirPage::OnNext ()
 	{
 	  if (source == IDC_SOURCE_LOCALDIR)
 	    {
-	      if (do_from_local_dir (GetInstance (), GetHWND (), local_dir))
-		{
-		  Progress.SetActivateTask (WM_APP_START_SETUP_INI_DOWNLOAD);
-		  return IDD_INSTATUS;
-		}
-	      return IDD_CHOOSE;
+	      do_from_local_dir (GetInstance (), GetHWND (), local_dir);
+	      Progress.SetActivateTask (WM_APP_START_SETUP_INI_DOWNLOAD);
+	      return IDD_INSTATUS;
 	    }
 	}
       else if (attr == INVALID_FILE_ATTRIBUTES
 	       && (GetLastError () == ERROR_FILE_NOT_FOUND
 		   || GetLastError () == ERROR_PATH_NOT_FOUND))
 	{
-	  if (source == IDC_SOURCE_LOCALDIR && unattended_mode)
-	    return IDD_CHOOSE;
-	  else if (source == IDC_SOURCE_LOCALDIR)
+	  if (source == IDC_SOURCE_LOCALDIR)
+	   {
+	   if (!unattended_mode)
 	    {
 	      // Check the user really wants only to uninstall.
 	      char msgText[1000];
@@ -290,8 +287,12 @@ LocalDirPage::OnNext ()
 	      snprintf (msg, sizeof (msg), msgText, local_dir.c_str (),
 		        is_64bit ? "x86_64" : "x86");
 	      int ret = MessageBox (h, msg, 0, MB_ICONEXCLAMATION | MB_OKCANCEL);
-	      return (ret == IDOK) ? IDD_CHOOSE : -1;
+	      if (ret == IDCANCEL)
+                return -1;
 	    }
+	   Progress.SetActivateTask (WM_APP_START_SETUP_INI_DOWNLOAD);
+	   return IDD_INSTATUS;
+	   }
 	  else if (offer_to_create (GetHWND (), local_dir.c_str ()))
 	    return -1;
 	  tryLocalDir = true;
@@ -322,10 +323,12 @@ LocalDirPage::OnNext ()
 	    return -1;
 	  else
 	    tryLocalDir = (ret == IDRETRY);
+	    // XXX: On IDIGNORE we drop through to IDD_NET, which is wrong in
+	    // the source == IDC_SOURCE_LOCALDIR case?
 	}
     }
 
-  return 0;
+  return 0; // IDD_NET
 }
 
 long
-- 
2.15.0

[PATCH setup 4/5] Correctly order preparing packagedb for chooser

[Jon Turney]

Do packagedb::prep() in ChooserPage::OnActivate(), as doing it in OnInit()
is wrong, as that only gets called once (but lazily).

Make packagedb::prep() idempotent after packagedb::init(), so it doesn't do
it's work again, if we come back to chooser from a later page.

Re-arrange applying command line package selection, and determining the
initial solution, which should only happen once, but after packagedb::prep()
---
 choose.cc     | 81 +++++++++++++++++++++++++++++++++--------------------------
 choose.h      |  3 +++
 package_db.cc | 30 ++++++++++++++++++++++
 package_db.h  | 23 +++++++++++------
 4 files changed, 93 insertions(+), 44 deletions(-)

diff --git a/choose.cc b/choose.cc
index 3a990d5..00e5977 100644
--- a/choose.cc
+++ b/choose.cc
@@ -61,7 +61,6 @@ static BoolOption UpgradeAlsoOption (false, 'g', "upgrade-also", "also upgrade i
 static BoolOption CleanOrphansOption (false, 'o', "delete-orphans", "remove orphaned packages");
 static BoolOption ForceCurrentOption (false, 'f', "force-current", "select the current version for all packages");
 static BoolOption PruneInstallOption (false, 'Y', "prune-install", "prune the installation to only the requested packages");
-static BoolOption MirrorOption (false, 'm', "mirror-mode", "Skip availability check when installing from local directory (requires local directory to be clean mirror!)");
 
 using namespace std;
 
@@ -89,7 +88,7 @@ static ControlAdjuster::ControlInfo ChooserControlsInfo[] = {
 
 ChooserPage::ChooserPage () :
   cmd_show_set (false), saved_geom (false), saw_geom_change (false),
-  timer_id (DEFAULT_TIMER_ID)
+  timer_id (DEFAULT_TIMER_ID), activated (false)
 {
   sizeProcessor.AddControlInfo (ChooserControlsInfo);
 
@@ -150,7 +149,12 @@ ChooserPage::createListview ()
   chooser->setViewMode (!is_new_install || UpgradeAlsoOption || hasManualSelections ?
 			PickView::views::PackagePending : PickView::views::Category);
   SendMessage (GetDlgItem (IDC_CHOOSE_VIEW), CB_SETCURSEL, (WPARAM)chooser->getViewMode(), 0);
+  ClearBusy ();
+}
 
+void
+ChooserPage::initialUpdateState()
+{
   // set the initial update state
   if (ForceCurrentOption)
     {
@@ -171,8 +175,6 @@ ChooserPage::createListview ()
 
   static int ta[] = { IDC_CHOOSE_KEEP, IDC_CHOOSE_BEST, IDC_CHOOSE_SYNC, 0 };
   rbset (GetHWND (), ta, update_mode_id);
-
-  ClearBusy ();
 }
 
 /* TODO: review ::overrides for possible consolidation */
@@ -263,20 +265,30 @@ ChooserPage::OnInit ()
       SendMessage(viewlist, CB_ADDSTRING, 0, (LPARAM)PickView::mode_caption((PickView::views)view));
     }
 
-  SetBusy ();
-  packagedb db;
-  db.makeBase();
-  db.read();
-  db.upgrade();
-  db.fixup_source_package_ids();
-  db.removeEmptyCategories();
+  if (source == IDC_SOURCE_DOWNLOAD)
+    setPrompt("Select packages to download ");
+  else
+    setPrompt("Select packages to install ");
 
-  if (source == IDC_SOURCE_DOWNLOAD || source == IDC_SOURCE_LOCALDIR)
-    packagemeta::ScanDownloadedFiles (MirrorOption);
+  createListview ();
 
-  db.setExistence ();
-  db.fillMissingCategory ();
+  AddTooltip (IDC_CHOOSE_KEEP, IDS_TRUSTKEEP_TOOLTIP);
+  AddTooltip (IDC_CHOOSE_BEST, IDS_TRUSTCURR_TOOLTIP);
+  AddTooltip (IDC_CHOOSE_SYNC, IDS_TRUSTSYNC_TOOLTIP);
+  AddTooltip (IDC_CHOOSE_EXP, IDS_TRUSTEXP_TOOLTIP);
+  AddTooltip (IDC_CHOOSE_VIEW, IDS_VIEWBUTTON_TOOLTIP);
+  AddTooltip (IDC_CHOOSE_HIDE, IDS_HIDEOBS_TOOLTIP);
+  AddTooltip (IDC_CHOOSE_SEARCH_EDIT, IDS_SEARCH_TOOLTIP);
+
+  /* Set focus to search edittext control. */
+  PostMessage (GetHWND (), WM_NEXTDLGCTL,
+	       (WPARAM) GetDlgItem (IDC_CHOOSE_SEARCH_EDIT), TRUE);
+}
 
+void
+ChooserPage::applyCommandLinePackageSelection()
+{
+  packagedb db;
   for (packagedb::packagecollection::iterator i = db.packages.begin ();
        i != db.packages.end (); ++i)
     {
@@ -303,32 +315,29 @@ ChooserPage::OnInit ()
       else
 	pkg.set_action (packagemeta::Default_action, pkg.installed);
     }
-
-  ClearBusy ();
-
-  if (source == IDC_SOURCE_DOWNLOAD)
-    setPrompt("Select packages to download ");
-  else
-    setPrompt("Select packages to install ");
-  createListview ();
-
-  AddTooltip (IDC_CHOOSE_KEEP, IDS_TRUSTKEEP_TOOLTIP);
-  AddTooltip (IDC_CHOOSE_BEST, IDS_TRUSTCURR_TOOLTIP);
-  AddTooltip (IDC_CHOOSE_SYNC, IDS_TRUSTSYNC_TOOLTIP);
-  AddTooltip (IDC_CHOOSE_EXP, IDS_TRUSTEXP_TOOLTIP);
-  AddTooltip (IDC_CHOOSE_VIEW, IDS_VIEWBUTTON_TOOLTIP);
-  AddTooltip (IDC_CHOOSE_HIDE, IDS_HIDEOBS_TOOLTIP);
-  AddTooltip (IDC_CHOOSE_SEARCH_EDIT, IDS_SEARCH_TOOLTIP);
-
-  /* Set focus to search edittext control. */
-  PostMessage (GetHWND (), WM_NEXTDLGCTL,
-	       (WPARAM) GetDlgItem (IDC_CHOOSE_SEARCH_EDIT), TRUE);
 }
 
 void
 ChooserPage::OnActivate()
 {
-  chooser->refresh();;
+  SetBusy();
+
+  packagedb db;
+  db.prep();
+
+  if (!activated)
+    {
+      // Do things which should only happen once, but rely on packagedb being
+      // ready to use, so OnInit() is too early
+      applyCommandLinePackageSelection();
+      initialUpdateState();
+
+      activated = true;
+    }
+
+  ClearBusy();
+
+  chooser->refresh();
   PlaceDialog (true);
 }
 
diff --git a/choose.h b/choose.h
index 6839b0b..32a1650 100644
--- a/choose.h
+++ b/choose.h
@@ -59,6 +59,8 @@ private:
   void logResults();
   void setPrompt(char const *aPrompt);
   void PlaceDialog (bool);
+  void applyCommandLinePackageSelection();
+  void initialUpdateState();
 
   PickView *chooser;
   static HWND ins_dialog;
@@ -74,6 +76,7 @@ private:
     UINT wpi[sizeof (WINDOWPLACEMENT) / sizeof (UINT)];
   };
   int update_mode_id;
+  bool activated;
 };
 
 #endif /* SETUP_CHOOSE_H */
diff --git a/package_db.cc b/package_db.cc
index 8fbec44..92fe4f9 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -40,6 +40,9 @@
 #include "resource.h"
 #include "libsolv.h"
 #include "csu_util/version_compare.h"
+#include "getopt++/BoolOption.h"
+
+static BoolOption MirrorOption (false, 'm', "mirror-mode", "Skip availability check when installing from local directory (requires local directory to be clean mirror!)");
 
 using namespace std;
 
@@ -52,6 +55,8 @@ packagedb::init ()
 {
   installeddbread = 0;
   installeddbver = 0;
+  prepped = false;
+
   packages.clear();
   sourcePackages.clear();
   categories.clear();
@@ -382,6 +387,7 @@ packagedb::findSourceVersion (PackageSpecification const &spec) const
 
 int packagedb::installeddbread = 0;
 int packagedb::installeddbver = 0;
+bool packagedb::prepped = false;
 packagedb::packagecollection packagedb::packages;
 packagedb::categoriesType packagedb::categories;
 packagedb::packagecollection packagedb::sourcePackages;
@@ -717,3 +723,27 @@ packagedb::fixup_source_package_ids()
         }
     }
 }
+
+void
+packagedb::prep()
+{
+  /* make packagedb ready for use for chooser */
+  if (prepped)
+    return;
+
+  makeBase();
+  read();
+  upgrade();
+  fixup_source_package_ids();
+  removeEmptyCategories();
+
+  /* XXX: this needs to be broken out somewhere where it can do progress
+     reporting, as it can take a long time... */
+  if (source == IDC_SOURCE_DOWNLOAD || source ==IDC_SOURCE_LOCALDIR)
+    packagemeta::ScanDownloadedFiles (MirrorOption);
+
+  setExistence ();
+  fillMissingCategory ();
+
+  prepped = true;
+}
diff --git a/package_db.h b/package_db.h
index a26c387..e500e4b 100644
--- a/package_db.h
+++ b/package_db.h
@@ -65,24 +65,21 @@ class packagedb
 public:
   packagedb ();
   void init();
-  void read();
-  void makeBase();
   /* 0 on success */
   int flush ();
-  void upgrade ();
+  void prep();
+
   packagemeta * findBinary (PackageSpecification const &) const;
   packageversion findBinaryVersion (PackageSpecification const &) const;
   packagemeta * findSource (PackageSpecification const &) const;
   packageversion findSourceVersion (PackageSpecification const &spec) const;
   packagemeta * addBinary (const std::string &pkgname, const SolverPool::addPackageData &pkgdata);
   packageversion addSource (const std::string &pkgname, const SolverPool::addPackageData &pkgdata);
-  void fixup_source_package_ids();
+
   PackageDBConnectedIterator connectedBegin();
   PackageDBConnectedIterator connectedEnd();
-  void fillMissingCategory();
+
   void defaultTrust (SolverTasks &q, SolverSolution::updateMode mode, bool test);
-  void setExistence();
-  void removeEmptyCategories();
 
   typedef std::map <std::string, packagemeta *> packagecollection;
   /* all seen binary packages */
@@ -100,11 +97,21 @@ public:
   static SolverSolution solution;
 
 private:
+  void makeBase();
+  void read();
+  void upgrade ();
+  void fixup_source_package_ids();
+  void removeEmptyCategories();
+  void fillMissingCategory();
+  void setExistence();
+  void guessUserPicked(void);
+
   static int installeddbread;	/* do we have to reread this */
   static int installeddbver;
+  static bool prepped;
+
   friend class ConnectedLoopFinder;
   static std::vector <packagemeta *> dependencyOrderedPackages;
-  void guessUserPicked(void);
 };
 
 #endif /* SETUP_PACKAGE_DB_H */
-- 
2.15.0

[PATCH setup 5/5] More crash avoidance in SolvableVersion member functions

[Jon Turney]

The following call-stack means we end up calling SolvableVersion methods
before packagedb::prep(), i.e. before attributes have been internalized, so
their values aren't available.

PickView::init_headers
PickView::refresh
PickView::init
ChooserPage::createListview
ChooserPage::OnInit

We call PickView::refresh again after we've computed the initial solution,
so the columns always end up with the correct width.
---
 libsolv.cc | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/libsolv.cc b/libsolv.cc
index 29a26a9..41b42eb 100644
--- a/libsolv.cc
+++ b/libsolv.cc
@@ -172,6 +172,10 @@ SolvableVersion::SDesc () const
     return "";
   Solvable *solvable = pool_id2solvable(pool, id);
   const char *sdesc = repo_lookup_str(solvable->repo, id, SOLVABLE_SUMMARY);
+
+  if (!sdesc)
+    return "";
+
   return sdesc;
 }
 
@@ -225,14 +229,14 @@ SolvableVersion::fixup_spkg_id (SolvableVersion spkg_id) const
 packagesource *
 SolvableVersion::source() const
 {
+  static packagesource empty_source = packagesource();
   if (!id) {
-    static packagesource empty_source = packagesource();
     return &empty_source;
   }
 
   Solvable *solvable = pool_id2solvable(pool, id);
   Id psrc_attr = pool_str2id(pool, "solvable:packagesource", 1);
-  return (packagesource *)repo_lookup_num(solvable->repo, id, psrc_attr, 0);
+  return (packagesource *)repo_lookup_num(solvable->repo, id, psrc_attr, (unsigned long long)&empty_source);
 }
 
 bool
-- 
2.15.0

Re: [setup topic/libsolv] Crash after incomplete download

[Ken Brown]

On 11/7/2017 1:15 PM, Jon Turney wrote:
> On 02/11/2017 19:25, Ken Brown wrote:
>> On 11/2/2017 1:22 PM, Jon Turney wrote:
>>> 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 :(
>>
>> I guess we could move all that prep to OnActivate() but make sure it's 
>> only run when we've gotten to the chooser page from the site page (and 
>> not because the user pressed Back on the prereq page).  What about 
>> adding a 'prepped' data member to packagedb and using this to decide 
>> whether to run the prep code?
> 
> An attempt at this to follow, but this ended up touching rather more 
> stuff than I wanted to, so I'm sure something else has broken :(

Well, at least the crash is gone.

Ken