Better checking for corrupt packages

Log an error if we need to access a package file in the release area which
isn't a valid compressed archive.  Ideally, we'd use tarfile.is_tarfile() to
check that, but that can propagate exceptions from the decompressor.

(Perhaps we should also ignore these invalid files, but I'm not sure that is
a good idea.)

Note that we avoid opening the package files in the release area unless
unavoidable, as it's expensive.  Validating a tar file involves reading the
entire file.

Always validate uploaded package files, and reject attempts to upload such
invalid files

Add appropriate tests

21 files changed, 104 insertions, 32 deletions

diff --git a/TODO b/TODO
index 60015d1..9c6b60b 100644
--- a/TODO
+++ b/TODO
@@ -5,3 +5,4 @@
 * run more often, option to not do anything if no uploads (to avoid reading the release area if we don't need to), lockfile to avoid colliding runs
 * work out some way to package it as a replacement for genini
 * use irkerd to report when calm failed due to an error?
+* upload a hash at the same time as package, and pass that through to setup.ini
diff --git a/calm/package.py b/calm/package.py
index 27a28fb..7cdd987 100755
--- a/calm/package.py
+++ b/calm/package.py
@@ -245,9 +245,13 @@ def tarfile_is_empty(tf):
         return False
 
     # if it's really a tar file, does it contain zero files?
-    with tarfile.open(tf) as a:
-        if any(a) == 0:
-            return True
+    try:
+        with tarfile.open(tf) as a:
+            if any(a) == 0:
+                return True
+    except Exception as e:
+        logging.error("exception %s while reading %s" % (type(e).__name__, tf))
+        logging.debug('', exc_info=True)
 
     return False
 
diff --git a/calm/pkg2html.py b/calm/pkg2html.py
index c5bedb6..f961ff7 100755
--- a/calm/pkg2html.py
+++ b/calm/pkg2html.py
@@ -150,21 +150,26 @@ def update_package_listings(args, packages, arch):
 
                         tf = os.path.join(args.rel_area, packages[p].path, t)
                         if not os.path.exists(tf):
-                            # XXX: this shouldn't happen with a full mirror...
-                            print('tarfile %s not found' % tf, file=f)
+                            # this shouldn't happen with a full mirror
+                            logging.error("tarfile %s not found %s" % (tf))
                         elif os.path.getsize(tf) <= 32:
                             # compressed empty files aren't a valid tar file,
                             # but we can just ignore them
                             pass
                         else:
-                            with tarfile.open(tf) as a:
-                                for i in a:
-                                    print('    %-16s%12d %s' % (time.strftime('%Y-%m-%d %H:%M', time.gmtime(i.mtime)), i.size, i.name), file=f, end='')
-                                    if i.isdir():
-                                        print('/', file=f, end='')
-                                    if i.issym() or i.islnk():
-                                        print(' -> %s' % i.linkname, file=f, end='')
-                                    print('', file=f)
+                            try:
+                                with tarfile.open(tf) as a:
+                                    for i in a:
+                                        print('    %-16s%12d %s' % (time.strftime('%Y-%m-%d %H:%M', time.gmtime(i.mtime)), i.size, i.name), file=f, end='')
+                                        if i.isdir():
+                                            print('/', file=f, end='')
+                                        if i.issym() or i.islnk():
+                                            print(' -> %s' % i.linkname, file=f, end='')
+                                        print('', file=f)
+                            except Exception as e:
+                                print('package is corrupted', file=f)
+                                logging.error("exception %s while reading %s" % (type(e).__name__, tf))
+                                logging.debug('', exc_info=True)
 
                         print(textwrap.dedent('''\
                                                  </pre></tt>
diff --git a/calm/uploads.py b/calm/uploads.py
index b35d11e..6759f69 100644
--- a/calm/uploads.py
+++ b/calm/uploads.py
@@ -31,6 +31,7 @@ import os
 import logging
 import re
 import shutil
+import tarfile
 import time
 
 from . import package
@@ -150,6 +151,7 @@ def scan(m, all_packages, arch, args):
                 files.remove(f)
                 continue
 
+            # a remove file, which indicates some other file should be removed
             if f.startswith('-'):
                 if ('*' in f) or ('?' in f):
                     logging.error("remove file %s name contains metacharacters, which are no longer supported" % fn)
@@ -161,25 +163,44 @@ def scan(m, all_packages, arch, args):
                     vault[relpath].append(f[1:])
                     remove_success.append(fn)
                 files.remove(f)
-            else:
-                dest = os.path.join(releasedir, relpath, f)
-                if os.path.isfile(dest):
-                    if f != 'setup.hint':
-                        if filecmp.cmp(dest, fn, shallow=False):
-                            logging.info("ignoring, identical %s is already in release area" % fn)
-                        else:
-                            logging.error("ignoring, different %s is already in release area (perhaps you should rebuild with a different version-release identifier?)" % fn)
-                            error = True
-                        files.remove(f)
+                continue
+
+            # verify compressed archive files are valid
+            if re.search(r'\.tar\.(bz2|gz|lzma|xz)$', f):
+                valid = True
+                try:
+                    # we need to extract all of an archive contents to validate
+                    # it
+                    with tarfile.open(fn) as a:
+                        a.getmembers()
+                except Exception as e:
+                    valid = False
+                    logging.error("exception %s while reading %s" % (type(e).__name__, fn))
+                    logging.debug('', exc_info=True)
+
+                if not valid:
+                    files.remove(f)
+                    continue
+
+            # does file already exist in release area?
+            dest = os.path.join(releasedir, relpath, f)
+            if os.path.isfile(dest):
+                if f != 'setup.hint':
+                    if filecmp.cmp(dest, fn, shallow=False):
+                        logging.info("ignoring, identical %s is already in release area" % fn)
                     else:
-                        if filecmp.cmp(dest, fn, shallow=False):
-                            logging.debug("identical %s is already in release area" % fn)
-                        else:
-                            logging.debug("different %s is already in release area" % fn)
-                        # we always consider setup.hint, as we can't have a valid package without it
-                        move[relpath].append(f)
+                        logging.error("ignoring, different %s is already in release area (perhaps you should rebuild with a different version-release identifier?)" % fn)
+                        error = True
+                    files.remove(f)
                 else:
+                    if filecmp.cmp(dest, fn, shallow=False):
+                        logging.debug("identical %s is already in release area" % fn)
+                    else:
+                        logging.debug("different %s is already in release area" % fn)
+                    # we always consider setup.hint, as we can't have a valid package without it
                     move[relpath].append(f)
+            else:
+                move[relpath].append(f)
 
         # read and validate package
         if files:
diff --git a/test/test_calm.py b/test/test_calm.py
index b97b4b7..60246d1 100755
--- a/test/test_calm.py
+++ b/test/test_calm.py
@@ -196,7 +196,8 @@ class CalmTest(unittest.TestCase):
 
         ready_fns = [(os.path.join(m.homedir(), 'x86', 'release', 'testpackage', '!ready'), ''),
                      (os.path.join(m.homedir(), 'x86', 'release', 'testpackage2', 'testpackage2-subpackage', '!ready'), ''),
-                     (os.path.join(m.homedir(), 'x86', 'release', 'after-ready', '!ready'), '-t 198709011700')]
+                     (os.path.join(m.homedir(), 'x86', 'release', 'after-ready', '!ready'), '-t 198709011700'),
+                     (os.path.join(m.homedir(), 'x86', 'release', 'corrupt', '!ready'), '')]
         for (f, t) in ready_fns:
             os.system('touch %s "%s"' % (t, f))
 
@@ -259,7 +260,8 @@ class CalmTest(unittest.TestCase):
         ready_fns = [(os.path.join(m_homedir, 'x86', 'release', 'testpackage', '!ready'), ''),
                      (os.path.join(m_homedir, 'x86', 'release', 'testpackage2', 'testpackage2-subpackage', '!ready'), ''),
                      (os.path.join(m_homedir, 'x86', 'release', 'after-ready', '!ready'), '-t 198709011700'),
-                     (os.path.join(m_homedir, 'noarch', 'release', 'perl-Net-SMTP-SSL', '!ready'), '')]
+                     (os.path.join(m_homedir, 'noarch', 'release', 'perl-Net-SMTP-SSL', '!ready'), ''),
+                     (os.path.join(m_homedir, 'x86', 'release', 'corrupt', '!ready'), '')]
         for (f, t) in ready_fns:
             os.system('touch %s "%s"' % (t, f))
 
diff --git a/test/testdata/hints/x86/release/corrupt/expected b/test/testdata/hints/x86/release/corrupt/expected
new file mode 100644
index 0000000..945213b
--- /dev/null
+++ b/test/testdata/hints/x86/release/corrupt/expected
@@ -0,0 +1,3 @@
+{'sdesc': '"A corrupt package"',
+ 'ldesc': '"A package containing corrupt archives"',
+ 'category': 'Devel'}
diff --git a/test/testdata/homes/Blooey McFooey/x86/release/corrupt/corrupt-2.1.0-1-src.tar.xz b/test/testdata/homes/Blooey McFooey/x86/release/corrupt/corrupt-2.1.0-1-src.tar.xz
new file mode 100644
index 0000000..3e99050
--- /dev/null
+++ b/test/testdata/homes/Blooey McFooey/x86/release/corrupt/corrupt-2.1.0-1-src.tar.xz
diff --git a/test/testdata/homes/Blooey McFooey/x86/release/corrupt/corrupt-2.1.0-1.tar.xz b/test/testdata/homes/Blooey McFooey/x86/release/corrupt/corrupt-2.1.0-1.tar.xz
new file mode 100644
index 0000000..a5e2e66
--- /dev/null
+++ b/test/testdata/homes/Blooey McFooey/x86/release/corrupt/corrupt-2.1.0-1.tar.xz
diff --git a/test/testdata/htdocs.expected/x86/corrupt/.htaccess b/test/testdata/htdocs.expected/x86/corrupt/.htaccess
new file mode 100644
index 0000000..3196d64
--- /dev/null
+++ b/test/testdata/htdocs.expected/x86/corrupt/.htaccess
@@ -0,0 +1,3 @@
+Options Indexes
+IndexOptions -FancyIndexing
+AddType text/html 1 2 3 4 5 6 7 8 9
diff --git a/test/testdata/htdocs.expected/x86/corrupt/corrupt-2.0.0-1 b/test/testdata/htdocs.expected/x86/corrupt/corrupt-2.0.0-1
new file mode 100644
index 0000000..a4e4ace
--- /dev/null
+++ b/test/testdata/htdocs.expected/x86/corrupt/corrupt-2.0.0-1
@@ -0,0 +1,6 @@
+<html>
+<h1>corrupt: A corrupt package (installed binaries and support files)</h1>
+<tt><pre>
+package is corrupted
+</pre></tt>
+</html>
diff --git a/test/testdata/htdocs.expected/x86/corrupt/corrupt-2.0.0-1-src b/test/testdata/htdocs.expected/x86/corrupt/corrupt-2.0.0-1-src
new file mode 100644
index 0000000..1d53e47
--- /dev/null
+++ b/test/testdata/htdocs.expected/x86/corrupt/corrupt-2.0.0-1-src
@@ -0,0 +1,8 @@
+<html>
+<h1>corrupt: A corrupt package (source code)</h1>
+<tt><pre>
+    2016-06-11 10:34           0 perl-Business-ISBN-2.011-1.src/
+    2016-06-11 10:34      364175 perl-Business-ISBN-2.011-1.src/Business-ISBN-2.011.tar.gz
+package is corrupted
+</pre></tt>
+</html>
diff --git a/test/testdata/htdocs.expected/x86/packages.inc b/test/testdata/htdocs.expected/x86/packages.inc
index 289a07e..367e3fd 100755
--- a/test/testdata/htdocs.expected/x86/packages.inc
+++ b/test/testdata/htdocs.expected/x86/packages.inc
@@ -8,6 +8,7 @@
 <table class="pkglist">
 <tr><td><a href="x86/arc">arc</a></td><td>The ARC archive utility</td></tr>
 <tr><td><a href="x86/base-cygwin">base-cygwin</a></td><td>Initial base installation helper script</td></tr>
+<tr><td><a href="x86/corrupt">corrupt</a></td><td>A corrupt package</td></tr>
 <tr><td><a href="x86/cygwin">cygwin</a></td><td>The UNIX emulation engine</td></tr>
 <tr><td><a href="x86/cygwin-debuginfo">cygwin-debuginfo</a></td><td>Debug info for cygwin</td></tr>
 <tr><td><a href="x86/cygwin-devel">cygwin-devel</a></td><td>Core development files</td></tr>
diff --git a/test/testdata/inifile/setup.ini.expected b/test/testdata/inifile/setup.ini.expected
index c41865a..3c60a3c 100644
--- a/test/testdata/inifile/setup.ini.expected
+++ b/test/testdata/inifile/setup.ini.expected
@@ -34,6 +34,16 @@
  'install: x86/release/base-cygwin/base-cygwin-3.6-1.tar.xz 228 '
  'e675b0ac4bc2c3e1c4971bc56d77b0cd53a9bdf5632873a235d7582e29dfd3e8a7bb04b28f6cdee3e6b3d14c25ed39392538e3f628a9bfda6c905646ebc3c225\n'
  '\n'
+ '@ corrupt\n'
+ 'sdesc: "A corrupt package"\n'
+ 'ldesc: "A package containing corrupt archives"\n'
+ 'category: Devel\n'
+ 'version: 2.0.0-1\n'
+ 'install: x86/release/corrupt/corrupt-2.0.0-1.tar.xz 128 '
+ '1f7a858f21049e9b13b8f4bbb9325582a304c8829d67c7cd302a33d78bc59808b6a84a0f6f19c87cc3c331aeaa60613437d35b0bae3390a78fd9118ac156d3f1\n'
+ 'source: x86/release/corrupt/corrupt-2.0.0-1-src.tar.xz 362012 '
+ '26a2a77ce2ab0691bbd20b720d34285dcf814888649810a70edadf398b7dca264dba7ba4449f51f20f2d1cd9617844919e2f2718b6014b37ab564d2463cc366b\n'
+ '\n'
  '@ cygwin\n'
  'sdesc: "The UNIX emulation engine"\n'
  'ldesc: "The UNIX emulation engine"\n'
diff --git a/test/testdata/pkglist/cygwin-pkg-maint b/test/testdata/pkglist/cygwin-pkg-maint
index c157e17..0dc42e7 100644
--- a/test/testdata/pkglist/cygwin-pkg-maint
+++ b/test/testdata/pkglist/cygwin-pkg-maint
@@ -183,6 +183,7 @@ copyright-update                             Jari Aalto
 corebird                                     Yaakov Selkowitz
 coreutils                                    Eric Blake
 corkscrew                                    Jari Aalto
+corrupt                                      Blooey McFooey
 cpio                                         Corinna Vinschen/Yaakov Selkowitz
 cppcheck                                     David Stacey
 cppi                                         Eric Blake
diff --git a/test/testdata/pkglist/expected b/test/testdata/pkglist/expected
index 47c296c..0f5bb1c 100644
--- a/test/testdata/pkglist/expected
+++ b/test/testdata/pkglist/expected
@@ -3,7 +3,7 @@
  'Adam Dinwoodie': maintainers.Maintainer('Adam Dinwoodie', [], ['git']),
  'Alexey Sokolov': maintainers.Maintainer('Alexey Sokolov', [], ['znc']),
  'Andrew Schulman': maintainers.Maintainer('Andrew Schulman', [], ['atool', 'autossh', 'bc', 'discus', 'fish', 'lftp', 'libargp', 'nosleep', 'orpie', 'pinfo', 'ploticus', 'ploticus-doc', 'screen', 'sitecopy', 'sng', 'socat', 'stow', 'stunnel', 'time', 'unison2.27', 'unison2.32', 'unison2.40', 'unison2.45', 'unison2.48']),
- 'Blooey McFooey': maintainers.Maintainer('Blooey McFooey', [], ['perl-Net-SMTP-SSL', 'testpackage']),
+ 'Blooey McFooey': maintainers.Maintainer('Blooey McFooey', [], ['corrupt', 'perl-Net-SMTP-SSL', 'testpackage']),
  'Bob Heckel': maintainers.Maintainer('Bob Heckel', [], ['libgc', 'w3m']),
  'Charles Wilson': maintainers.Maintainer('Charles Wilson', [], ['alternatives', 'autobuild', 'cygutils', 'gcc-tools-epoch1-autoconf', 'gcc-tools-epoch1-automake', 'gcc-tools-epoch2-autoconf', 'gcc-tools-epoch2-automake', 'inetutils', 'libassuan', 'libksba', 'libustr', 'libXpm-noX', 'mingw-binutils', 'mingw-bzip2', 'mingw-gcc', 'mingw-libgcrypt', 'mingw-libgpg-error', 'mingw-pthreads', 'mingw-xz', 'mingw-zlib', 'nfrotz', 'pinentry', 'pth', 'rsh', 'run2', 'rxvt', 'sunrpc', 'tcp_wrappers', 'xsri']),
  'Chris J. Breisch': maintainers.Maintainer('Chris J. Breisch', [], ['man-db']),
diff --git a/test/testdata/process_arch/homedir.expected b/test/testdata/process_arch/homedir.expected
index 1c52158..9110c87 100644
--- a/test/testdata/process_arch/homedir.expected
+++ b/test/testdata/process_arch/homedir.expected
@@ -6,6 +6,7 @@
  'Blooey McFooey/x86': [],
  'Blooey McFooey/x86/release': [],
  'Blooey McFooey/x86/release/after-ready': ['after-ready-1.0-1.tar.bz2', 'setup.hint'],
+ 'Blooey McFooey/x86/release/corrupt': ['corrupt-2.1.0-1-src.tar.xz', 'corrupt-2.1.0-1.tar.xz'],
  'Blooey McFooey/x86/release/not-on-maintainer-list': ['not-on-maintainer-list-1.0-1.tar.bz2', 'setup.hint'],
  'Blooey McFooey/x86/release/not-on-package-list': ['not-on-package-list-1.0-1.tar.bz2', 'setup.hint'],
  'Blooey McFooey/x86/release/not-ready': ['-not-ready-0.9-1.tar.bz2', 'not-ready-1.0-1.tar.bz2', 'setup.hint'],
diff --git a/test/testdata/process_arch/htdocs.expected b/test/testdata/process_arch/htdocs.expected
index 4670972..448a696 100644
--- a/test/testdata/process_arch/htdocs.expected
+++ b/test/testdata/process_arch/htdocs.expected
@@ -2,6 +2,7 @@
  'x86': ['.htaccess', 'packages.inc'],
  'x86/arc': ['.htaccess', 'arc-4.32.7-10', 'arc-4.32.7-10-src'],
  'x86/base-cygwin': ['.htaccess', 'base-cygwin-3.6-1', 'base-cygwin-3.8-1'],
+ 'x86/corrupt': ['.htaccess', 'corrupt-2.0.0-1', 'corrupt-2.0.0-1-src'],
  'x86/cygwin': ['.htaccess',
                 'cygwin-2.2.0-1',
                 'cygwin-2.2.0-1-src',
diff --git a/test/testdata/process_arch/rel_area.expected b/test/testdata/process_arch/rel_area.expected
index 44161bf..b147754 100644
--- a/test/testdata/process_arch/rel_area.expected
+++ b/test/testdata/process_arch/rel_area.expected
@@ -11,6 +11,7 @@
  'x86/release': ['sha512.sum'],
  'x86/release/arc': ['arc-4.32.7-10-src.tar.bz2', 'arc-4.32.7-10.tar.bz2', 'setup.hint', 'sha512.sum'],
  'x86/release/base-cygwin': ['base-cygwin-3.6-1.tar.xz', 'base-cygwin-3.8-1.tar.xz', 'setup.hint', 'sha512.sum'],
+ 'x86/release/corrupt': ['corrupt-2.0.0-1-src.tar.xz', 'corrupt-2.0.0-1.tar.xz', 'setup.hint', 'sha512.sum'],
  'x86/release/cygwin': ['.this-should-be-ignored',
                         'cygwin-2.2.0-1-src.tar.xz',
                         'cygwin-2.2.0-1.tar.xz',
diff --git a/test/testdata/relarea/x86/release/corrupt/corrupt-2.0.0-1-src.tar.xz b/test/testdata/relarea/x86/release/corrupt/corrupt-2.0.0-1-src.tar.xz
new file mode 100644
index 0000000..395b50d
--- /dev/null
+++ b/test/testdata/relarea/x86/release/corrupt/corrupt-2.0.0-1-src.tar.xz
diff --git a/test/testdata/relarea/x86/release/corrupt/corrupt-2.0.0-1.tar.xz b/test/testdata/relarea/x86/release/corrupt/corrupt-2.0.0-1.tar.xz
new file mode 100644
index 0000000..5c2814f
--- /dev/null
+++ b/test/testdata/relarea/x86/release/corrupt/corrupt-2.0.0-1.tar.xz
@@ -0,0 +1 @@
+f��S�h݂{Ȇ����U�CN�<�^�	�Ih�v��*f@C��ބ\�{�����K� �ya^Kź��x74�G�?t��=_̴z��n�w��ch�b��WQ���Qy�~�����8�q+v
\ No newline at end of file
diff --git a/test/testdata/relarea/x86/release/corrupt/setup.hint b/test/testdata/relarea/x86/release/corrupt/setup.hint
new file mode 100644
index 0000000..fa85bcc
--- /dev/null
+++ b/test/testdata/relarea/x86/release/corrupt/setup.hint
@@ -0,0 +1,3 @@
+sdesc: "A corrupt package"
+ldesc: "A package containing corrupt archives"
+category: Devel