From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7441 invoked by alias); 13 Jan 2018 18:36:55 -0000 Mailing-List: contact cygwin-apps-cvs-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: cygwin-apps-cvs-owner@sourceware.org Received: (qmail 7422 invoked by uid 9996); 13 Jan 2018 18:36:54 -0000 Date: Sat, 13 Jan 2018 18:36:00 -0000 Message-ID: <20180113183654.7396.qmail@sourceware.org> From: kbrown@sourceware.org To: cygwin-apps-cvs@sourceware.org Subject: [setup - the official Cygwin setup program] branch master, updated. release_2.884-1-gff2cd3f X-Git-Refname: refs/heads/master X-Git-Reftype: branch X-Git-Oldrev: b000ea175ccac3236a202c4c066d7c2b3a67e3ec X-Git-Newrev: ff2cd3f4d71cefc1a68d6454cb27115e88acdaea X-SW-Source: 2018-q1/txt/msg00016.txt.bz2 https://sourceware.org/git/gitweb.cgi?p=cygwin-apps/setup.git;h=ff2cd3f4d71cefc1a68d6454cb27115e88acdaea commit ff2cd3f4d71cefc1a68d6454cb27115e88acdaea Author: Ken Brown Date: Tue Jan 9 12:04:20 2018 -0500 Query the user if a corrupt local file is found 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. Diff: --- download.cc | 122 +++++++++++++++++++++++++--------------- 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, 264 insertions(+), 186 deletions(-) diff --git a/download.cc b/download.cc index 322a4c1..129b226 100644 --- a/download.cc +++ b/download.cc @@ -54,29 +54,41 @@ 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) + { + pkgsource.set_cached (""); + 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) + 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 +96,31 @@ 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, pkgsource.Cached() was corrupt and 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; + // If we get here, fullname was corrupt and deleted, but it + // might have been cached. + pkgsource.set_cached (""); } /* @@ -111,15 +132,14 @@ 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; + // If we get here, fullname was corrupt and deleted, but it + // might have been cached. + pkgsource.set_cached (""); + } } return 0; } @@ -130,7 +150,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 +183,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 +296,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 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..5b7d239 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 and download again?" 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