[PATCH setup v2] Query the user if a corrupt local file is found
Ken Brown
kbrown@cornell.edu
Tue Jan 9 22:25:00 GMT 2018
Also reorganize package validation.
Move the size-validation code in download.cc and the hash-validation
code in install.cc into new member functions of the packagesource
class. Add a bool member 'validated' to the class to make sure that
the checking is done only once.
Change download.cc:check_for_cached() so that it offers to delete a
corrupt package file instead of throwing an exception. The latter
previously caused a fatal error when check_for_cached() was called
from do_download_thread and download_one. Now we get a fatal error
only if the user chooses not to delete the file.
Also make check_for_cached() check the hash of the file in addition to
the size. Similarly, check the hash in addition to the size after
downloading a file.
---
download.cc | 118 ++++++++++++++++++++++++----------------
download.h | 5 +-
install.cc | 141 +++---------------------------------------------
package_meta.cc | 2 +-
package_source.cc | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
package_source.h | 17 +++++-
res.rc | 3 +-
resource.h | 1 +
8 files changed, 260 insertions(+), 186 deletions(-)
diff --git a/download.cc b/download.cc
index 322a4c1..0dd198e 100644
--- a/download.cc
+++ b/download.cc
@@ -54,29 +54,43 @@ extern ThreeBarProgressPage Progress;
BoolOption IncludeSource (false, 'I', "include-source", "Automatically install source for every package installed");
+// Return true if selected checks pass, false if they don't and the
+// user chooses to delete the file; otherwise throw an exception.
static bool
-validateCachedPackage (const std::string& fullname, packagesource & pkgsource)
+validateCachedPackage (const std::string& fullname, packagesource & pkgsource,
+ HWND owner, bool check_hash, bool check_size)
{
- DWORD size = get_file_size(fullname);
- if (size != pkgsource.size)
- {
- Log (LOG_BABBLE) << "INVALID PACKAGE: " << fullname
- << " - Size mismatch: Ini-file: " << pkgsource.size
- << " != On-disk: " << size << endLog;
- return false;
- }
- return true;
+ try
+ {
+ if (check_size)
+ pkgsource.check_size_and_cache (fullname);
+ if (check_hash)
+ pkgsource.check_hash ();
+ return true;
+ }
+ catch (Exception *e)
+ {
+ const char *filename = fullname.c_str ();
+ if (strncmp (filename, "file://", 7) == 0)
+ filename += 7;
+ if (e->errNo() == APPERR_CORRUPT_PACKAGE
+ && yesno (owner, IDS_QUERY_CORRUPT, filename) == IDYES)
+ {
+ pkgsource.set_cached ("");
+ remove (filename);
+ }
+ else
+ throw e;
+ }
+ return false;
}
-/* 0 on failure
+/* 0 if not cached; may throw exception if validation fails.
*/
int
-check_for_cached (packagesource & pkgsource, bool mirror_mode)
+check_for_cached (packagesource & pkgsource, HWND owner, bool mirror_mode,
+ bool check_hash)
{
- // Already found one.
- if (pkgsource.Cached())
- return 1;
-
/* Note that the cache dir is represented by a mirror site of file://local_dir */
std::string prefix = "file://" + local_dir + "/";
std::string fullname = prefix + (pkgsource.Canonical() ? pkgsource.Canonical() : "");
@@ -84,22 +98,28 @@ check_for_cached (packagesource & pkgsource, bool mirror_mode)
if (mirror_mode)
{
/* Just assume correctness of mirror. */
- pkgsource.set_cached (fullname);
+ if (!pkgsource.Cached())
+ pkgsource.set_cached (fullname);
return 1;
}
+ // Already found one, which we can assume to have the right size.
+ if (pkgsource.Cached())
+ {
+ if (validateCachedPackage (pkgsource.Cached(), pkgsource, owner,
+ check_hash, false))
+ return 1;
+ // If we get here, a corrupt file was deleted.
+ pkgsource.set_cached ("");
+ }
+
/*
1) is there a legacy version in the cache dir available.
*/
if (io_stream::exists (fullname))
{
- if (validateCachedPackage (fullname, pkgsource))
- pkgsource.set_cached (fullname);
- else
- throw new Exception (TOSTRING(__LINE__) " " __FILE__,
- "Package validation failure for " + fullname,
- APPERR_CORRUPT_PACKAGE);
- return 1;
+ if (validateCachedPackage (fullname, pkgsource, owner, check_hash, true))
+ return 1;
}
/*
@@ -111,15 +131,11 @@ check_for_cached (packagesource & pkgsource, bool mirror_mode)
std::string fullname = prefix + rfc1738_escape_part (n->key) + "/" +
pkgsource.Canonical ();
if (io_stream::exists(fullname))
- {
- if (validateCachedPackage (fullname, pkgsource))
- pkgsource.set_cached (fullname);
- else
- throw new Exception (TOSTRING(__LINE__) " " __FILE__,
- "Package validation failure for " + fullname,
- APPERR_CORRUPT_PACKAGE);
- return 1;
- }
+ {
+ if (validateCachedPackage (fullname, pkgsource, owner, check_hash,
+ true))
+ return 1;
+ }
}
return 0;
}
@@ -130,7 +146,7 @@ download_one (packagesource & pkgsource, HWND owner)
{
try
{
- if (check_for_cached (pkgsource))
+ if (check_for_cached (pkgsource, owner))
return 0;
}
catch (Exception * e)
@@ -163,26 +179,36 @@ download_one (packagesource & pkgsource, HWND owner)
}
else
{
- size_t size = get_file_size ("file://" + local + ".tmp");
- if (size == pkgsource.size)
+ try
{
- Log (LOG_PLAIN) << "Downloaded " << local << endLog;
if (_access (local.c_str(), 0) == 0)
remove (local.c_str());
rename ((local + ".tmp").c_str(), local.c_str());
+ pkgsource.check_size_and_cache ("file://" + local);
+ pkgsource.check_hash ();
+ Log (LOG_PLAIN) << "Downloaded " << local << endLog;
success = 1;
- pkgsource.set_cached ("file://" + local);
// FIXME: move the downloaded file to the
// original locations - without the mirror site dir in the way
continue;
}
- else
+ catch (Exception *e)
{
- Log (LOG_PLAIN) << "Download " << local << " wrong size (" <<
- size << " actual vs " << pkgsource.size << " expected)" <<
- endLog;
- remove ((local + ".tmp").c_str());
- continue;
+ remove (local.c_str());
+ pkgsource.set_cached ("");
+ if (e->errNo() == APPERR_CORRUPT_PACKAGE)
+ {
+ Log (LOG_PLAIN) << "Downloaded file " << local
+ << " is corrupt; deleting." << endLog;
+ continue;
+ }
+ else
+ {
+ Log (LOG_PLAIN) << "Unexpected exception while validating "
+ << "downloaded file " << local
+ << "; deleting." << endLog;
+ throw e;
+ }
}
}
}
@@ -266,12 +292,12 @@ do_download_thread (HINSTANCE h, HWND owner)
{
if (pkg.picked())
{
- if (!check_for_cached (*version.source()))
+ if (!check_for_cached (*version.source(), owner))
total_download_bytes += version.source()->size;
}
if (pkg.srcpicked () || IncludeSource)
{
- if (!check_for_cached (*sourceversion.source()))
+ if (!check_for_cached (*sourceversion.source(), owner))
total_download_bytes += sourceversion.source()->size;
}
}
diff --git a/download.h b/download.h
index 117a44d..9f4cb8e 100644
--- a/download.h
+++ b/download.h
@@ -16,7 +16,10 @@
#ifndef SETUP_DOWNLOAD_H
#define SETUP_DOWNLOAD_H
+#include "win32.h"
+
class packagesource;
-int check_for_cached (packagesource & pkgsource, bool = false);
+int check_for_cached (packagesource & pkgsource, HWND owner,
+ bool mirror_mode = false, bool check_hash = true);
#endif /* SETUP_DOWNLOAD_H */
diff --git a/install.cc b/install.cc
index d6af331..366aeb2 100644
--- a/install.cc
+++ b/install.cc
@@ -21,7 +21,6 @@
files in /etc/setup/\* and create the mount points. */
#include "getopt++/BoolOption.h"
-#include "csu_util/MD5Sum.h"
#include "LogFile.h"
#include "win32.h"
@@ -148,7 +147,6 @@ Installer::StandardDirs[] = {
};
static int num_installs, num_uninstalls;
-static void chksum_one (const packagemeta &pkg, const packagesource& pkgsource);
void
Installer::preremoveOne (packagemeta & pkg)
@@ -841,7 +839,10 @@ do_install_thread (HINSTANCE h, HWND owner)
}
/* md5sum the packages, build lists of packages to install and uninstall
- and calculate the total amount of data to install */
+ and calculate the total amount of data to install.
+ The hash checking is relevant only for local installs. For a
+ net install, the hashes will have already been verified at download
+ time, and all calls to check_hash() below should instantly return. */
long long int md5sum_total_bytes_sofar = 0;
for (packagedb::packagecollection::iterator i = db.packages.begin ();
i != db.packages.end (); ++i)
@@ -852,7 +853,7 @@ do_install_thread (HINSTANCE h, HWND owner)
{
try
{
- chksum_one (pkg, *pkg.desired.source ());
+ (*pkg.desired.source ()).check_hash ();
}
catch (Exception *e)
{
@@ -872,7 +873,7 @@ do_install_thread (HINSTANCE h, HWND owner)
bool skiprequested = false ;
try
{
- chksum_one (pkg, *pkg.desired.sourcePackage ().source ());
+ (*pkg.desired.sourcePackage ().source ()).check_hash ();
}
catch (Exception *e)
{
@@ -1013,133 +1014,3 @@ do_install (HINSTANCE h, HWND owner)
DWORD threadID;
CreateThread (NULL, 0, do_install_reflector, context, 0, &threadID);
}
-
-static char *
-sha512_str (const unsigned char *in, char *buf)
-{
- char *bp = buf;
- for (int i = 0; i < SHA512_DIGEST_LENGTH; ++i)
- bp += sprintf (bp, "%02x", in[i]);
- *bp = '\0';
- return buf;
-}
-
-static void
-sha512_one (const packagemeta &pkg, const packagesource& pkgsource)
-{
- std::string fullname (pkgsource.Cached ());
-
- io_stream *thefile = io_stream::open (fullname, "rb", 0);
- if (!thefile)
- throw new Exception (TOSTRING (__LINE__) " " __FILE__,
- std::string ("IO Error opening ") + fullname,
- APPERR_IO_ERROR);
- SHA2_CTX ctx;
- unsigned char sha512result[SHA512_DIGEST_LENGTH];
- char ini_sum[SHA512_DIGEST_STRING_LENGTH],
- disk_sum[SHA512_DIGEST_STRING_LENGTH];
-
- SHA512Init (&ctx);
-
- Log (LOG_BABBLE) << "Checking SHA512 for " << fullname << endLog;
-
- Progress.SetText1 ((std::string ("Checking SHA512 for ")
- + pkg.name).c_str ());
- Progress.SetText4 ("Progress:");
- Progress.SetBar1 (0);
-
- unsigned char buffer[64 * 1024];
- ssize_t count;
- while ((count = thefile->read (buffer, sizeof (buffer))) > 0)
- {
- SHA512Update (&ctx, buffer, count);
- Progress.SetBar1 (thefile->tell (), thefile->get_size ());
- }
- delete thefile;
- if (count < 0)
- throw new Exception (TOSTRING(__LINE__) " " __FILE__,
- "IO Error reading " + fullname,
- APPERR_IO_ERROR);
-
- SHA512Final (sha512result, &ctx);
-
- if (memcmp (pkgsource.sha512sum, sha512result, sizeof sha512result))
- {
- Log (LOG_BABBLE) << "INVALID PACKAGE: " << fullname
- << " - SHA512 mismatch: Ini-file: "
- << sha512_str (pkgsource.sha512sum, ini_sum)
- << " != On-disk: "
- << sha512_str (sha512result, disk_sum)
- << endLog;
- throw new Exception (TOSTRING(__LINE__) " " __FILE__,
- "SHA512 failure for " + fullname,
- APPERR_CORRUPT_PACKAGE);
- }
-
- Log (LOG_BABBLE) << "SHA512 verified OK: " << fullname << " "
- << sha512_str (pkgsource.sha512sum, ini_sum) << endLog;
-}
-
-static void
-md5_one (const packagemeta &pkg, const packagesource& pkgsource)
-{
- std::string fullname (pkgsource.Cached ());
-
- io_stream *thefile = io_stream::open (fullname, "rb", 0);
- if (!thefile)
- throw new Exception (TOSTRING (__LINE__) " " __FILE__,
- std::string ("IO Error opening ") + fullname,
- APPERR_IO_ERROR);
- MD5Sum tempMD5;
- tempMD5.begin ();
-
- Log (LOG_BABBLE) << "Checking MD5 for " << fullname << endLog;
-
- Progress.SetText1 ((std::string ("Checking MD5 for ")
- + pkg.name).c_str ());
- Progress.SetText4 ("Progress:");
- Progress.SetBar1 (0);
-
- unsigned char buffer[64 * 1024];
- ssize_t count;
- while ((count = thefile->read (buffer, sizeof (buffer))) > 0)
- {
- tempMD5.append (buffer, count);
- Progress.SetBar1 (thefile->tell (), thefile->get_size ());
- }
- delete thefile;
- if (count < 0)
- throw new Exception (TOSTRING(__LINE__) " " __FILE__,
- "IO Error reading " + fullname,
- APPERR_IO_ERROR);
-
- tempMD5.finish ();
-
- if (pkgsource.md5 != tempMD5)
- {
- Log (LOG_BABBLE) << "INVALID PACKAGE: " << fullname
- << " - MD5 mismatch: Ini-file: " << pkgsource.md5.str()
- << " != On-disk: " << tempMD5.str() << endLog;
- throw new Exception (TOSTRING(__LINE__) " " __FILE__,
- "MD5 failure for " + fullname,
- APPERR_CORRUPT_PACKAGE);
- }
-
- Log (LOG_BABBLE) << "MD5 verified OK: " << fullname << " "
- << pkgsource.md5.str() << endLog;
-}
-
-static void
-chksum_one (const packagemeta &pkg, const packagesource& pkgsource)
-{
- if (!pkgsource.Cached ())
- return;
- if (pkgsource.sha512_isSet)
- sha512_one (pkg, pkgsource);
- else if (pkgsource.md5.isSet())
- md5_one (pkg, pkgsource);
- else
- Log (LOG_BABBLE) << "No checksum recorded for " << pkg.name
- << ", cannot determine integrity of package!"
- << endLog;
-}
diff --git a/package_meta.cc b/package_meta.cc
index 7b79738..af494f4 100644
--- a/package_meta.cc
+++ b/package_meta.cc
@@ -606,7 +606,7 @@ packagemeta::scan (const packageversion &pkg, bool mirror_mode)
*/
try
{
- if (!check_for_cached (*(pkg.source ()), mirror_mode)
+ if (!check_for_cached (*(pkg.source ()), NULL, mirror_mode, false)
&& ::source == IDC_SOURCE_LOCALDIR)
pkg.source ()->sites.clear ();
}
diff --git a/package_source.cc b/package_source.cc
index 15540f6..dca1945 100644
--- a/package_source.cc
+++ b/package_source.cc
@@ -18,6 +18,15 @@
*/
#include "package_source.h"
+#include "sha2.h"
+#include "csu_util/MD5Sum.h"
+#include "LogFile.h"
+#include "threebar.h"
+#include "Exception.h"
+#include "filemanip.h"
+#include "io_stream.h"
+
+extern ThreeBarProgressPage Progress;
site::site (const std::string& newkey) : key(newkey)
{
@@ -27,6 +36,8 @@ void
packagesource::set_canonical (char const *fn)
{
canonical = fn;
+ size_t found = canonical.find_last_of ("/");
+ shortname = canonical.substr (found + 1);
}
void
@@ -34,3 +45,151 @@ packagesource::set_cached (const std::string& fp)
{
cached = fp;
}
+
+void
+packagesource::check_size_and_cache (const std::string fullname)
+{
+ DWORD fnsize = get_file_size (fullname);
+ if (fnsize != size)
+ {
+ Log (LOG_BABBLE) << "INVALID PACKAGE: " << fullname
+ << " - Size mismatch: Ini-file: " << size
+ << " != On-disk: " << fnsize << endLog;
+ throw new Exception (TOSTRING(__LINE__) " " __FILE__,
+ "Size mismatch for " + fullname,
+ APPERR_CORRUPT_PACKAGE);
+ }
+ cached = fullname;
+ validated = false;
+}
+
+void
+packagesource::check_hash ()
+{
+ if (validated || cached.empty ())
+ return;
+
+ if (sha512_isSet)
+ {
+ check_sha512 (cached);
+ validated = true;
+ }
+ else if (md5.isSet())
+ {
+ check_md5 (cached);
+ validated = true;
+ }
+ else
+ Log (LOG_BABBLE) << "No checksum recorded for " << cached
+ << ", cannot determine integrity of package!"
+ << endLog;
+}
+
+static char *
+sha512_str (const unsigned char *in, char *buf)
+{
+ char *bp = buf;
+ for (int i = 0; i < SHA512_DIGEST_LENGTH; ++i)
+ bp += sprintf (bp, "%02x", in[i]);
+ *bp = '\0';
+ return buf;
+}
+
+void
+packagesource::check_sha512 (const std::string fullname) const
+{
+ io_stream *thefile = io_stream::open (fullname, "rb", 0);
+ if (!thefile)
+ throw new Exception (TOSTRING (__LINE__) " " __FILE__,
+ std::string ("IO Error opening ") + fullname,
+ APPERR_IO_ERROR);
+ SHA2_CTX ctx;
+ unsigned char sha512result[SHA512_DIGEST_LENGTH];
+ char ini_sum[SHA512_DIGEST_STRING_LENGTH],
+ disk_sum[SHA512_DIGEST_STRING_LENGTH];
+
+ SHA512Init (&ctx);
+
+ Log (LOG_BABBLE) << "Checking SHA512 for " << fullname << endLog;
+
+ Progress.SetText1 (("Checking SHA512 for " + shortname).c_str ());
+ Progress.SetText4 ("Progress:");
+ Progress.SetBar1 (0);
+
+ unsigned char buffer[64 * 1024];
+ ssize_t count;
+ while ((count = thefile->read (buffer, sizeof (buffer))) > 0)
+ {
+ SHA512Update (&ctx, buffer, count);
+ Progress.SetBar1 (thefile->tell (), thefile->get_size ());
+ }
+ delete thefile;
+ if (count < 0)
+ throw new Exception (TOSTRING(__LINE__) " " __FILE__,
+ "IO Error reading " + fullname,
+ APPERR_IO_ERROR);
+
+ SHA512Final (sha512result, &ctx);
+
+ if (memcmp (sha512sum, sha512result, sizeof sha512result))
+ {
+ Log (LOG_BABBLE) << "INVALID PACKAGE: " << fullname
+ << " - SHA512 mismatch: Ini-file: "
+ << sha512_str (sha512sum, ini_sum)
+ << " != On-disk: "
+ << sha512_str (sha512result, disk_sum)
+ << endLog;
+ throw new Exception (TOSTRING(__LINE__) " " __FILE__,
+ "SHA512 failure for " + fullname,
+ APPERR_CORRUPT_PACKAGE);
+ }
+
+ Log (LOG_BABBLE) << "SHA512 verified OK: " << fullname << " "
+ << sha512_str (sha512sum, ini_sum) << endLog;
+}
+
+void
+packagesource::check_md5 (const std::string fullname) const
+{
+ io_stream *thefile = io_stream::open (fullname, "rb", 0);
+ if (!thefile)
+ throw new Exception (TOSTRING (__LINE__) " " __FILE__,
+ std::string ("IO Error opening ") + fullname,
+ APPERR_IO_ERROR);
+ MD5Sum tempMD5;
+ tempMD5.begin ();
+
+ Log (LOG_BABBLE) << "Checking MD5 for " << fullname << endLog;
+
+ Progress.SetText1 (("Checking MD5 for " + shortname).c_str ());
+ Progress.SetText4 ("Progress:");
+ Progress.SetBar1 (0);
+
+ unsigned char buffer[64 * 1024];
+ ssize_t count;
+ while ((count = thefile->read (buffer, sizeof (buffer))) > 0)
+ {
+ tempMD5.append (buffer, count);
+ Progress.SetBar1 (thefile->tell (), thefile->get_size ());
+ }
+ delete thefile;
+ if (count < 0)
+ throw new Exception (TOSTRING(__LINE__) " " __FILE__,
+ "IO Error reading " + fullname,
+ APPERR_IO_ERROR);
+
+ tempMD5.finish ();
+
+ if (md5 != tempMD5)
+ {
+ Log (LOG_BABBLE) << "INVALID PACKAGE: " << fullname
+ << " - MD5 mismatch: Ini-file: " << md5.str()
+ << " != On-disk: " << tempMD5.str() << endLog;
+ throw new Exception (TOSTRING(__LINE__) " " __FILE__,
+ "MD5 failure for " + fullname,
+ APPERR_CORRUPT_PACKAGE);
+ }
+
+ Log (LOG_BABBLE) << "MD5 verified OK: " << fullname << " "
+ << md5.str() << endLog;
+}
diff --git a/package_source.h b/package_source.h
index 8675c51..fae46f7 100644
--- a/package_source.h
+++ b/package_source.h
@@ -41,7 +41,7 @@ public:
class packagesource
{
public:
- packagesource ():size (0), canonical (), cached ()
+ packagesource ():size (0), canonical (), shortname (), cached (), validated (false)
{
memset (sha512sum, 0, sizeof sha512sum);
sha512_isSet = false;
@@ -58,7 +58,12 @@ public:
return NULL;
};
- /* what is the cached filename, to prevent directory scanning during install */
+ /* What is the cached filename, to prevent directory scanning during
+ install? Except in mirror mode, we never set this without
+ checking the size. The more expensive hash checking is reserved
+ for verifying the integrity of downloaded files and sources of
+ packages about to be installed. Set the 'validated' flag to
+ avoid checking the hash twice. */
char const *Cached () const
{
/* Pointer-coerce-to-boolean is used by many callers. */
@@ -72,12 +77,20 @@ public:
unsigned char sha512sum[SHA512_DIGEST_LENGTH];
bool sha512_isSet;
MD5Sum md5;
+ /* The next two functions throw exceptions on failure. */
+ void check_size_and_cache (const std::string fullname);
+ void check_hash ();
typedef std::vector <site> sitestype;
sitestype sites;
private:
std::string canonical;
+ /* For progress reporting. */
+ std::string shortname;
std::string cached;
+ bool validated;
+ void check_sha512 (const std::string fullname) const;
+ void check_md5 (const std::string fullname) const;
};
#endif /* SETUP_PACKAGE_SOURCE_H */
diff --git a/res.rc b/res.rc
index 82a9757..9648871 100644
--- a/res.rc
+++ b/res.rc
@@ -550,7 +550,8 @@ BEGIN
IDS_DOWNLOAD_INCOMPLETE_EXIT "Download incomplete. Check %s for details"
IDS_INSTALL_ERROR "Installation error (%s), Continue with other packages?"
IDS_INSTALL_INCOMPLETE "Installation incomplete. Check %s for details"
- IDS_CORRUPT_PACKAGE "Package file %s has a corrupt local copy, please remove and retry."
+ IDS_CORRUPT_PACKAGE "Package %s has a corrupt local copy, please remove and retry."
+ IDS_QUERY_CORRUPT "The file %s is corrupt. Delete it?."
IDS_SKIP_PACKAGE "%s\nDo you want to skip this package ?"
IDS_UNCAUGHT_EXCEPTION "Fatal Error: Uncaught Exception\nThread: %s\nType: %s\nMessage: %s"
IDS_UNCAUGHT_EXCEPTION_WITH_ERRNO "Fatal Error: Uncaught Exception\nThread: %s\nType: %s\nMessage: %s\nAppErrNo: %d"
diff --git a/resource.h b/resource.h
index 79b876d..70d90ca 100644
--- a/resource.h
+++ b/resource.h
@@ -39,6 +39,7 @@
#define IDS_ELEVATED 139
#define IDS_INSTALLEDB_VERSION 140
#define IDS_DOWNLOAD_INCOMPLETE_EXIT 141
+#define IDS_QUERY_CORRUPT 142
// Dialogs
--
2.15.1
More information about the Cygwin-apps
mailing list