Re: Problems installing from Cygwinports

Re: Problems installing from Cygwinports

[Yaakov (Cygwin/X)]

http://cygwin.com/acronyms/#PPIOSPE

As a potential setup.exe bug, redirecting to cygwin-apps.

On Thu, 2011-03-31 at 00:54 +0200, Angelo Graziosi wrote:
> It is sometime I have installed a few packages from Cygwinports 
> (QT,GCC), but today upgrading (mainly *QT*-4.7.2-1 and *mesa*-7.10.1-2) 
> 'setup.exe' seems to hang in extracting /usr/include/GL/glxext.h from 
> libGL-devel-7.10.1-2.

Confirmed.

(The tarball in question is here:)
ftp://ftp.cygwinports.org/pub/cygwinports/release-2/_DISTRO_/X.Org/mesa/libGL-devel/libGL-devel-7.10.1-2.tar.bz2

> Result: my hard drive (C:) is almost full, X does not start any more and 
> I can use only Cygwin.bat and mintty. To be short, my Cygwin 
> installation is broken :(

Start by deleting /usr/include/GL/glxext.h, that will free up your hard
drive.  Then re-run setup, choosing to KEEP libGL-devel instead of
upgrading.

Now, as for the cause, the only thing I can think of is that I just
started using pbzip2 for compression in cygport[1].  The bz2's are
supposed to be fully compatible, and neither Cygwin nor MinGW bzip2 have
any problems with the tarball in question.

So my wild guess is that setup uses libbz2 in a way which is
incompatible with pbzip2-compressed tarballs, as unlikely as it seems.  

Can any setup.exe authors shed any light on this?


Yaakov
Cygwin Ports

[1] http://cygwin-ports.git.sourceforge.net/git/gitweb.cgi?p=cygwin-ports/cygport;a=commit;h=6f1f87e

Re: Problems installing from Cygwinports

[Jon TURNEY]

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

On 31/03/2011 06:15, Yaakov (Cygwin/X) wrote:
> As a potential setup.exe bug, redirecting to cygwin-apps.
> 
> On Thu, 2011-03-31 at 00:54 +0200, Angelo Graziosi wrote:
>> It is sometime I have installed a few packages from Cygwinports 
>> (QT,GCC), but today upgrading (mainly *QT*-4.7.2-1 and *mesa*-7.10.1-2) 
>> 'setup.exe' seems to hang in extracting /usr/include/GL/glxext.h from 
>> libGL-devel-7.10.1-2.
> 
> Confirmed.
> 
> (The tarball in question is here:)
> ftp://ftp.cygwinports.org/pub/cygwinports/release-2/_DISTRO_/X.Org/mesa/libGL-devel/libGL-devel-7.10.1-2.tar.bz2
> 
>> Result: my hard drive (C:) is almost full, X does not start any more and 
>> I can use only Cygwin.bat and mintty. To be short, my Cygwin 
>> installation is broken :(
> 
> Start by deleting /usr/include/GL/glxext.h, that will free up your hard
> drive.  Then re-run setup, choosing to KEEP libGL-devel instead of
> upgrading.
> 
> Now, as for the cause, the only thing I can think of is that I just
> started using pbzip2 for compression in cygport[1].  The bz2's are
> supposed to be fully compatible, and neither Cygwin nor MinGW bzip2 have
> any problems with the tarball in question.
> 
> So my wild guess is that setup uses libbz2 in a way which is
> incompatible with pbzip2-compressed tarballs, as unlikely as it seems.  
> 
> Can any setup.exe authors shed any light on this?

I do actually want to install that package, so I took a brief look at this:

The first problem is that .bz2 file and the decompressor don't like each
other. The decompressor reports the stream has ended partway through
decompressing glxext.h.  It's very mysterious that it should fail but bunzip2
has no problems with the same file.

The second problem is a setup bug: If we get an unexpected short read from the
archive stream decompressor, we'll sit in an infinite loop writing garbage to
the output file 5 bytes at a time.  A patch is attached which fixes that, but
since these errors are  unreported (see comment in io_stream::copy), we just
stop expanding the archive, leaving an incomplete glxext.h

[-- Attachment #2: 0001-Don-t-hang-if-something-goes-wrong-reading-from-a-ta.patch --]
[-- Type: text/plain, Size: 1577 bytes --]

From 3ca6eaca538d496867ac4e5e9faf2cf51162ad2f Mon Sep 17 00:00:00 2001
From: Jon TURNEY <jon.turney@dronecode.org.uk>
Date: Thu, 31 Mar 2011 14:45:47 +0100
Subject: [PATCH] Don't hang if something goes wrong reading from a tar archive

Returning EIO (a small positive integer) as the result of archive_tar_file::read()
on an error isn't a good idea, it's indistinguishable from successfully reading
a small number of bytes.

This causes io_stream::copy() to spin forever if it's reading from an archive stream
which terminates unexpectedly early, which can happen on an decompression error.

So, return 0 instead

2011-03-31  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* archive_tar_file.cc (read, peek): Return 0 on an error,
	not EIO.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 archive_tar_file.cc |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/archive_tar_file.cc b/archive_tar_file.cc
index 29a2df7..ed6e515 100644
--- a/archive_tar_file.cc
+++ b/archive_tar_file.cc
@@ -65,7 +65,7 @@ ssize_t archive_tar_file::read (void *buffer, size_t len)
 	  /* unexpected EOF or read error in the tar parent stream */
 	  /* the user can query the parent for the error */
 	  state.lasterr = EIO;
-	  return EIO;
+	  return 0;
 	}
     }
   return 0;
@@ -96,7 +96,7 @@ ssize_t archive_tar_file::peek (void *buffer, size_t len)
 	  /* unexpected EOF or read error in the tar parent stream */
 	  /* the user can query the parent for the error */
 	  state.lasterr = EIO;
-	  return EIO;
+	  return 0;
 	}
     }
   return 0;
-- 
1.7.4

Re: Problems installing from Cygwinports

[Eric Blake]

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

On 03/31/2011 08:35 AM, Jon TURNEY wrote:
> The second problem is a setup bug: If we get an unexpected short read from the
> archive stream decompressor, we'll sit in an infinite loop writing garbage to
> the output file 5 bytes at a time.  A patch is attached which fixes that, but
> since these errors are  unreported (see comment in io_stream::copy), we just
> stop expanding the archive, leaving an incomplete glxext.h

Would returning -EIO be better, since a negative value is a key that
something went wrong besides normal end of file?

-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: Problems installing from Cygwinports

[Corinna Vinschen]

On Mar 31 15:35, Jon TURNEY wrote:
> 2011-03-31  Jon TURNEY  
> 
> 	* archive_tar_file.cc (read, peek): Return 0 on an error,
> 	not EIO.

Thanks, please apply.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[PATCH 4/6] Report failure extracting a file from package to the user

[Jon TURNEY]

At the moment, all errors in archive::extract_file() are assumed to be due
to a failure to open the file for writing due to it being opened by another
process.

Distinguish when the error is due to an inability to read the file from the
source archive, and report that.

2011-04-08  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* install.cc (extract_replace_on_reboot): New function containg code
	extracted from...
	(installOne): Report read errors differently to write errors
	* archive.cc (extract_file): Distinguish read errors from write errors

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 archive.cc |    2 +-
 install.cc |  245 ++++++++++++++++++++++++++++++++++++------------------------
 2 files changed, 148 insertions(+), 99 deletions(-)

diff --git a/archive.cc b/archive.cc
index 7449c2c..aacf0b5 100644
--- a/archive.cc
+++ b/archive.cc
@@ -127,7 +127,7 @@ archive::extract_file (archive * source, const std::string& prefixURL,
 	    delete in;
 	    delete tmp;
 	    io_stream::remove (destfilename);
-	    return 1;
+	    return 2;
 	  }
 	tmp->set_mtime (in->get_mtime ());
 	delete in;
diff --git a/install.cc b/install.cc
index 6891fec..1b72456 100644
--- a/install.cc
+++ b/install.cc
@@ -96,6 +96,10 @@ class Installer
                      packagesource &source,
                      const std::string& , const std::string&, HWND );
     int errors;
+  private:
+    bool extract_replace_on_reboot(archive *, const std::string&,
+                                   const std::string&, std::string);
+
 };
 
 Installer::Installer() : errors(0)
@@ -241,6 +245,79 @@ create_allow_protected_renames ()
   RegCloseKey (key);
 }
 
+bool
+Installer::extract_replace_on_reboot (archive *tarstream, const std::string& prefixURL,
+                                      const std::string& prefixPath, std::string fn)
+{
+  /* Extract a copy of the file with extension .new appended and
+     indicate it should be replaced on the next reboot.  */
+  if (archive::extract_file (tarstream, prefixURL, prefixPath,
+                             ".new") != 0)
+    {
+      log (LOG_PLAIN) << "Unable to install file " << prefixURL
+                      << prefixPath << fn << ".new" << endLog;
+      ++errors;
+      return true;
+    }
+  else
+    {
+      std::string s = cygpath ("/" + fn + ".new"),
+        d = cygpath ("/" + fn);
+
+      if (!IsWindowsNT())
+        {
+          /* Get the short file names */
+          char s2[MAX_PATH], d2[MAX_PATH];
+          unsigned int slen =
+            GetShortPathName (s.c_str (), s2, MAX_PATH),
+            dlen = GetShortPathName (d.c_str (), d2, MAX_PATH);
+
+          if (!slen || slen > MAX_PATH || !dlen || dlen > MAX_PATH)
+            replaceOnRebootFailed(fn);
+          else
+            if (!WritePrivateProfileString ("rename", d2, s2,
+                                            "WININIT.INI"))
+              replaceOnRebootFailed (fn);
+            else
+              replaceOnRebootSucceeded (fn, rebootneeded);
+        }
+      else
+        {
+          /* XXX FIXME: prefix may not be / for in use files -
+           * although it most likely is
+           * - we need a io method to get win32 paths
+           * or to wrap this system call
+           */
+          WCHAR sname[s.size () + 7];
+          WCHAR dname[d.size () + 7];
+          /* Windows 2000 has a bug:  Prepending \\?\ does not
+           * work in conjunction with MOVEFILE_DELAY_UNTIL_REBOOT.
+           * So in case of Windows 2000 we just convert the path
+           * to wide char and hope for the best. */
+          if (OSMajorVersion () == 5 && OSMinorVersion () == 0)
+            {
+              mbstowcs (sname, s.c_str (), s.size () + 7);
+              mbstowcs (dname, d.c_str (), d.size () + 7);
+            }
+          else
+            {
+              mklongpath (sname, s.c_str (), s.size () + 7);
+              mklongpath (dname, d.c_str (), d.size () + 7);
+            }
+          if (!MoveFileExW (sname, dname,
+                            MOVEFILE_DELAY_UNTIL_REBOOT |
+                            MOVEFILE_REPLACE_EXISTING))
+            replaceOnRebootFailed (fn);
+          else
+            {
+              create_allow_protected_renames ();
+              replaceOnRebootSucceeded (fn, rebootneeded);
+            }
+        }
+    }
+  return false;
+}
+
 #undef MessageBox
 #define MessageBox _custom_MessageBox
 
@@ -352,6 +429,7 @@ Installer::installOne (packagemeta &pkgm, const packageversion &ver,
     }
 
   bool error_in_this_package = false;
+  bool ignoreInUseErrors = unattended_mode;
   bool ignoreExtractErrors = unattended_mode;
 
   package_bytes = source.size;
@@ -373,107 +451,78 @@ Installer::installOne (packagemeta &pkgm, const packageversion &ver,
         pkgm.desired.addScript (Script (canonicalfn));
 
       bool firstIteration = true;
-      while (archive::extract_file (tarstream, prefixURL, prefixPath) != 0)
+      int extract_error = 0;
+      while ((extract_error = archive::extract_file (tarstream, prefixURL, prefixPath)) != 0)
         {
-          if (!ignoreExtractErrors)
+          switch (extract_error)
             {
-              char msg[fn.size() + 300];
-              sprintf (msg,
-                       "%snable to extract /%s -- the file is in use.\r\n"
-                       "Please stop %s Cygwin processes and select \"Retry\", or\r\n"
-                       "select \"Continue\" to go on anyway (you will need to reboot).\r\n",
-                       firstIteration?"U":"Still u", fn.c_str(), firstIteration?"all":"ALL");
-              switch (MessageBox (owner, msg, "In-use files detected",
-                       MB_RETRYCONTINUE | MB_ICONWARNING | MB_TASKMODAL))
-                {
-                  case IDRETRY:
-                    // retry
-                    firstIteration = false;
-                    continue;
-                  case IDCONTINUE:
-                    ignoreExtractErrors = true;
-                    break;
-                  default:
-                    break;
-                }
-              // fall through to previous functionality
-            }
-          if (NoReplaceOnReboot)
-            {
-              ++errors;
-              error_in_this_package = true;
-              log (LOG_PLAIN) << "Not replacing in-use file " << prefixURL
-                  << prefixPath << fn << endLog;
-            }
-          else
-            {
-              /* Extract a copy of the file with extension .new appended and
-                 indicate it should be replaced on the next reboot.  */
-              if (archive::extract_file (tarstream, prefixURL, prefixPath,
-                                         ".new") != 0)
-                {
-                  log (LOG_PLAIN) << "Unable to install file " << prefixURL
-                      << prefixPath << fn << ".new" << endLog;
-                  ++errors;
-                  error_in_this_package = true;
-                }
-              else
-                {
-                  std::string s = cygpath ("/" + fn + ".new"),
-                              d = cygpath ("/" + fn);
-
-                  if (!IsWindowsNT())
-                    {
-                      /* Get the short file names */
-                      char s2[MAX_PATH], d2[MAX_PATH];
-                      unsigned int slen =
-                                GetShortPathName (s.c_str (), s2, MAX_PATH),
-                         dlen = GetShortPathName (d.c_str (), d2, MAX_PATH);
-
-                      if (!slen || slen > MAX_PATH || !dlen || dlen > MAX_PATH)
-                        replaceOnRebootFailed(fn);
-                      else
-                        if (!WritePrivateProfileString ("rename", d2, s2,
-                                                        "WININIT.INI"))
-                          replaceOnRebootFailed (fn);
-                        else
-                          replaceOnRebootSucceeded (fn, rebootneeded);
-                    }
-                  else
-                    {
-                      /* XXX FIXME: prefix may not be / for in use files -
-                       * although it most likely is
-                       * - we need a io method to get win32 paths
-                       * or to wrap this system call
-                       */
-		      WCHAR sname[s.size () + 7];
-		      WCHAR dname[d.size () + 7];
-		      /* Windows 2000 has a bug:  Prepending \\?\ does not
-		       * work in conjunction with MOVEFILE_DELAY_UNTIL_REBOOT.
-		       * So in case of Windows 2000 we just convert the path
-		       * to wide char and hope for the best. */
-		      if (OSMajorVersion () == 5 && OSMinorVersion () == 0)
-		      	{
-			  mbstowcs (sname, s.c_str (), s.size () + 7);
-			  mbstowcs (dname, d.c_str (), d.size () + 7);
-			}
-		      else
-			{
-			  mklongpath (sname, s.c_str (), s.size () + 7);
-			  mklongpath (dname, d.c_str (), d.size () + 7);
-			}
-                      if (!MoveFileExW (sname, dname,
-					MOVEFILE_DELAY_UNTIL_REBOOT |
-					MOVEFILE_REPLACE_EXISTING))
-                        replaceOnRebootFailed (fn);
-                      else
-			{
-			  create_allow_protected_renames ();
-			  replaceOnRebootSucceeded (fn, rebootneeded);
-			}
-                    }
-                }
+            case 1: // in use
+              {
+                if (!ignoreInUseErrors)
+                  {
+                    char msg[fn.size() + 300];
+                    sprintf (msg,
+                             "%snable to extract /%s -- the file is in use.\r\n"
+                             "Please stop %s Cygwin processes and select \"Retry\", or\r\n"
+                             "select \"Continue\" to go on anyway (you will need to reboot).\r\n",
+                             firstIteration?"U":"Still u", fn.c_str(), firstIteration?"all":"ALL");
+
+                    switch (MessageBox (owner, msg, "In-use files detected",
+                                        MB_RETRYCONTINUE | MB_ICONWARNING | MB_TASKMODAL))
+                      {
+                      case IDRETRY:
+                        // retry
+                        firstIteration = false;
+                        continue;
+                      case IDCONTINUE:
+                        ignoreInUseErrors = true;
+                        break;
+                      default:
+                        break;
+                      }
+                    // fall through to previous functionality
+                  }
+
+                if (NoReplaceOnReboot)
+                  {
+                    ++errors;
+                    error_in_this_package = true;
+                    log (LOG_PLAIN) << "Not replacing in-use file " << prefixURL
+                                    << prefixPath << fn << endLog;
+                  }
+                else
+                  {
+                    error_in_this_package = extract_replace_on_reboot(tarstream, prefixURL, prefixPath, fn);
+                  }
+              }
+              break;
+            case 2: // extract failed
+              {
+                if (!ignoreExtractErrors)
+                  {
+                    char msg[fn.size() + 300];
+                    sprintf (msg,
+                             "Unable to extract /%s -- corrupt package?\r\n",
+                             fn.c_str());
+
+                    // XXX: We should offer the option to retry,
+                    // continue without extracting this particular archive,
+                    // or ignore all extraction errors.
+                    // Unfortunately, we don't currently know how to rewind
+                    // the archive, so we can't retry at present,
+                    // and ignore all errors is mis-implemented at present
+                    // to only apply to errors arising from a single archive,
+                    // so we degenerate to the continue option.
+                    MessageBox (owner, msg, "File extraction error",
+                                MB_OK | MB_ICONWARNING | MB_TASKMODAL);
+                  }
+
+                // don't mark this package as successfully installed
+                error_in_this_package = true;
+              }
+              break;
             }
+
           // We're done with this file
           break;
         }
-- 
1.7.4

[PATCH 1/6] Don't hang if something goes wrong reading from a tar archive

[Jon TURNEY]

Returning EIO (a small positive integer) as the result of archive_tar_file::read()
on an error isn't a good idea, it's indistinguishable from successfully reading
a small number of bytes.

This causes io_stream::copy() to spin forever if it's reading from an archive stream
which terminates unexpectedly early, which can happen on an decompression error.

So, return -1 and set lasterr instead.  This matches the behaviour of POSIX
read(), so shouldn't surprise anyone :-)

Make the same change for archive_tar_file::write(), archive_tar_file::tell()
and archive_tar_file::peek() for consistency

2011-04-08  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* archive_tar_file.cc (read, write, peek, seek): Consistently return -1
	and set lasterr on an error.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 archive_tar_file.cc |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/archive_tar_file.cc b/archive_tar_file.cc
index 29a2df7..5ffb3a1 100644
--- a/archive_tar_file.cc
+++ b/archive_tar_file.cc
@@ -65,7 +65,7 @@ ssize_t archive_tar_file::read (void *buffer, size_t len)
 	  /* unexpected EOF or read error in the tar parent stream */
 	  /* the user can query the parent for the error */
 	  state.lasterr = EIO;
-	  return EIO;
+	  return -1;
 	}
     }
   return 0;
@@ -75,7 +75,8 @@ ssize_t archive_tar_file::read (void *buffer, size_t len)
 ssize_t archive_tar_file::write (const void *buffer, size_t len)
 {
   /* write not supported */
-  return EBADF;
+  state.lasterr = EBADF;
+  return -1;
 }
 
 /* read data without removing it from the class's internal buffer */
@@ -96,7 +97,7 @@ ssize_t archive_tar_file::peek (void *buffer, size_t len)
 	  /* unexpected EOF or read error in the tar parent stream */
 	  /* the user can query the parent for the error */
 	  state.lasterr = EIO;
-	  return EIO;
+	  return -1;
 	}
     }
   return 0;
@@ -113,6 +114,7 @@ archive_tar_file::seek (long where, io_stream_seek_t whence)
 {
   /* nothing needs seeking here yet. Implement when needed 
    */
+  state.lasterr = EINVAL;
   return -1;
 }
 
-- 
1.7.4

[PATCH 5/6] Handle bzip2 multi-stream files

[Jon TURNEY]

bzip2 compressed files are allowed to contain multiple streams of
compressed data.  So if we arrive at BZ_STREAM_END but are not at
EOF, restart the decompressor.  Decompress any data which remains
in the stream before reading any more.

Also remove write only member bufN

2011-04-08  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* compress_bz.h (compress): Remove unused bufN member.
	* compress_bz.cc (read): Handle bzip2 files containing multiple
	streams

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 compress_bz.cc |   35 ++++++++++++++++++++---------------
 compress_bz.h  |    1 -
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/compress_bz.cc b/compress_bz.cc
index f45f434..fb23529 100644
--- a/compress_bz.cc
+++ b/compress_bz.cc
@@ -37,7 +37,6 @@ compress_bz::compress_bz (io_stream * parent) : peeklen (0), position (0)
 
   initialisedOk = 0;
   endReached = 0;
-  bufN = 0;
   writing = 0;
   strm.bzalloc = 0;
   strm.bzfree = 0;
@@ -78,34 +77,26 @@ compress_bz::read (void *buffer, size_t len)
       else
         return tmpread;
   }
-  
+
   strm.avail_out = len;
   strm.next_out = (char *) buffer;
   int rlen = 1;
   while (1)
     {
-      if (original->error ())
-	{
-	  lasterr = original->error ();
-	  return -1;
-	}
+      int ret = BZ2_bzDecompress (&strm);
+
       if (strm.avail_in == 0 && rlen > 0)
 	{
 	  rlen = original->read (buf, 4096);
 	  if (rlen < 0)
 	    {
-	      if (original->error ())
-    		lasterr = original->error ();
-	      else
-		lasterr = rlen;
+              lasterr = original->error ();
 	      return -1;
 	    }
-	  bufN = rlen;
 	  strm.avail_in = rlen;
 	  strm.next_in = buf;
 	}
-      int
-	ret = BZ2_bzDecompress (&strm);
+
       if (ret != BZ_OK && ret != BZ_STREAM_END)
 	{
 	  lasterr = ret;
@@ -119,7 +110,21 @@ compress_bz::read (void *buffer, size_t len)
 	}
       if (ret == BZ_STREAM_END)
 	{
-	  endReached = 1;
+          /* Are we also at EOF? */
+          if (rlen == 0)
+            {
+              endReached = 1;
+            }
+          else
+            {
+              /* BZ_SSTREAM_END but not at EOF means the file contains
+                 another stream */
+              BZ2_bzDecompressEnd (&strm);
+              BZ2_bzDecompressInit (&(strm), 0, 0);
+              /* This doesn't reinitialize strm, so strm.next_in still
+                 points at strm.avail_in bytes left to decompress in buf */
+            }
+
 	  position += len - strm.avail_out;
 	  return len - strm.avail_out;
 	}
diff --git a/compress_bz.h b/compress_bz.h
index 24f68a7..39a0d5b 100644
--- a/compress_bz.h
+++ b/compress_bz.h
@@ -64,7 +64,6 @@ private:
   bz_stream strm;
   int initialisedOk;
   int endReached;
-  int bufN;
   char buf[4096];
   int writing;
   size_t position;
-- 
1.7.4

[PATCH 6/6] Handle short reads in archive_tar_file::read()

[Jon TURNEY]

archive_tar_file::read() currently considers any short read an error.

These can now occur if the underlying bz2 compressed io_stream starts a
new compression stream in the middle of the read

It also transparently reads to the next 512-byte block in the parent
io_stream after reading all the data expected for a file.

Teach it to do things correctly, even if a short read occurs.

2011-04-08  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* archive_tar_file.cc (read): Handle short reads

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 archive_tar_file.cc |   52 +++++++++++++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/archive_tar_file.cc b/archive_tar_file.cc
index 5ffb3a1..9a1721b 100644
--- a/archive_tar_file.cc
+++ b/archive_tar_file.cc
@@ -49,24 +49,40 @@ ssize_t archive_tar_file::read (void *buffer, size_t len)
   read_something = true;
   if (want)
     {
-      ssize_t
-	got = state.parent->read (buffer, want);
-      char
-	throwaway[512];
-      ssize_t
-	got2 = state.parent->read (throwaway, roundup);
-      if (got == want && got2 == roundup)
-	{
-	  state.file_offset += got;
-	  return got;
-	}
-      else
-	{
-	  /* unexpected EOF or read error in the tar parent stream */
-	  /* the user can query the parent for the error */
-	  state.lasterr = EIO;
-	  return -1;
-	}
+      int need = want;
+      char *p = (char *)buffer;
+      while (need > 0)
+        {
+          ssize_t got = state.parent->read (p, need);
+          if (got <= 0)
+            {
+              /* unexpected EOF or read error in the tar parent stream */
+              /* the user can query the parent for the error */
+              state.lasterr = EIO;
+              return -1;
+            }
+          p += got;
+          need -= got;
+        }
+
+      char throwaway[512];
+      p = &throwaway[0];
+      while (roundup > 0)
+        {
+          ssize_t got2 = state.parent->read (p, roundup);
+          if (got2 <= 0)
+            {
+              /* unexpected EOF or read error in the tar parent stream */
+              /* the user can query the parent for the error */
+              state.lasterr = EIO;
+              return -1;
+            }
+          p += got2;
+          roundup -= got2;
+        }
+
+      state.file_offset += want;
+      return want;
     }
   return 0;
 }
-- 
1.7.4

[PATCH 3/6] Propagate errors out of io_stream::copy()

[Jon TURNEY]

Despite the comment here, io_stream::read() is now implemented to provide the
same behaviour as POSIX read(): If the stream is at EOF, 0 is returned. If an
error occurred, -1 is returned.  So we can detect errors and propagate them to
our caller.

2011-04-08  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* io_stream.cc (copy): Propagate errors.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 io_stream.cc |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/io_stream.cc b/io_stream.cc
index 08ecae4..49cf0c4 100644
--- a/io_stream.cc
+++ b/io_stream.cc
@@ -184,18 +184,15 @@ ssize_t io_stream::copy (io_stream * in, io_stream * out)
 	  return countout ? countout : -1;
 	}
     }
-  
-  /* Here it would be nice to be able to do something like
-
-       if (countin < 0)
-         return -1;
 
-     in order to be able to detect a read error occured and pass it on
-     to the caller, however with the current io_stream class this will fail
-     spectacularly because there is no way to signal an EOF condition, and
-     thus the only way the while loop above ends is the resulting error from
-     trying to read past EOF.
+  /*
+    Loop above ends with countin = 0 if we have reached EOF, or -1 if an
+    read error occurred.
+  */
+  if (countin < 0)
+    return -1;
 
+   /* Here it would be nice to be able to do something like
      TODO:
      out->set_mtime (in->get_mtime ());
    */
-- 
1.7.4

[PATCH 0/6] Handle and report decompression errors; handle multistream bzip2 files

[Jon TURNEY]

On 31/03/2011 15:35, Jon TURNEY wrote:
> The first problem is that .bz2 file and the decompressor don't like each
> other. The decompressor reports the stream has ended partway through
> decompressing glxext.h.  It's very mysterious that it should fail but bunzip2
> has no problems with the same file.

The reason for this failure is that the pbzip compressed file contains multiple
compressed streams, which we need to decompress and concatenate to handle correctly.

While files like that can be made using bzip2 (merely by appending the output of
multiple runs), I don't think that tar -j will make one unless pbzip is in use.

On 31/03/2011 15:35, Jon TURNEY wrote:
> The second problem is a setup bug: If we get an unexpected short read from the
> archive stream decompressor, we'll sit in an infinite loop writing garbage to
> the output file 5 bytes at a time.  A patch is attached which fixes that, but
> since these errors are  unreported (see comment in io_stream::copy), we just
> stop expanding the archive, leaving an incomplete glxext.h

Silent failures are the devil's spawn, so I've done some more work to make it
so these failures can be reported to the user

On 31/03/2011 15:42, Eric Blake wrote:
> Would returning -EIO be better, since a negative value is a key that
> something went wrong besides normal end of file?

On reflection, I think that the correct thing to do is follow the model of POSIX
read() for all the io_stream derived classes read() method, returning 0 at EOF, o
returning -1 and setting the value returned by error() if an error occured.


I do not think that 5/6 and 6/6 should be applied without some serious review
and/or testing, as the possibilty that they break package installation in some way
needs to be balanced against the benefit of being able to use pbzip to make packages.

Jon TURNEY (6):
  Don't hang if something goes wrong reading from a tar archive
  Consistently return -1 and set lasterr instead in io_stream derived
    classes
  Propagate errors out of io_stream::copy()
  Report failure extracting a file from package to the user
  Handle bzip2 multi-stream files
  Handle short reads in archive_tar_file::read()

 archive.cc          |    2 +-
 archive_tar_file.cc |   58 ++++++++----
 compress_bz.cc      |   47 ++++++----
 compress_bz.h       |    1 -
 compress_gz.cc      |   20 ++++-
 install.cc          |  245 ++++++++++++++++++++++++++++++--------------------
 io_stream.cc        |   17 ++--
 7 files changed, 238 insertions(+), 152 deletions(-)

-- 
1.7.4

[PATCH 2/6] Consistently return -1 and set lasterr instead in io_stream derived classes

[Jon TURNEY]

2011-04-08  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* compress_bz.cc (read, peek): Consistently return -1 and set lasterr
	on an error.
	* compress_gz.cc (read, write, peek): Ditto.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 compress_bz.cc |   12 +++++++++---
 compress_gz.cc |   20 ++++++++++++++++----
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/compress_bz.cc b/compress_bz.cc
index 866a1ff..f45f434 100644
--- a/compress_bz.cc
+++ b/compress_bz.cc
@@ -57,7 +57,10 @@ ssize_t
 compress_bz::read (void *buffer, size_t len)
 {
   if (!initialisedOk || writing)
-    return EBADF;
+    {
+      lasterr = EBADF;
+      return -1;
+    }
   if (endReached)
     return 0;
   if (len == 0)
@@ -143,10 +146,13 @@ ssize_t compress_bz::peek (void *buffer, size_t len)
       lasterr = EBADF;
       return -1;
     }
-  
+
   /* can only peek 512 bytes */
   if (len > 512)
-    return ENOMEM;
+    {
+      lasterr = ENOMEM;
+      return -1;
+    }
 
   if (len > peeklen)
     {
diff --git a/compress_gz.cc b/compress_gz.cc
index cff772e..7686adf 100644
--- a/compress_gz.cc
+++ b/compress_gz.cc
@@ -231,7 +231,10 @@ compress_gz::read (void *buffer, size_t len)
   Byte *next_out;		/* == stream.next_out but not forced far (for MSDOS) */
 
   if (mode != 'r')
-    return Z_STREAM_ERROR;
+    {
+      z_err =  Z_STREAM_ERROR;
+      return -1;
+    }
 
   if (z_err == Z_DATA_ERROR || z_err == Z_ERRNO)
     return -1;
@@ -336,7 +339,10 @@ ssize_t
 compress_gz::write (const void *buffer, size_t len)
 {
   if (mode != 'w')
-    return Z_STREAM_ERROR;
+    {
+      z_err = Z_STREAM_ERROR;
+      return -1;
+    }
 
   stream.next_in = (Bytef *) buffer;
   stream.avail_in = len;
@@ -368,10 +374,16 @@ ssize_t
 compress_gz::peek (void *buffer, size_t len)
 {
   if (mode != 'r')
-    return Z_STREAM_ERROR;
+    {
+      z_err = Z_STREAM_ERROR;
+      return -1;
+    }
   /* can only peek 512 bytes */
   if (len > 512)
-    return ENOMEM;
+    {
+      z_err = ENOMEM;
+      return -1;
+    }
 
   if (len > peeklen)
     {
-- 
1.7.4

Re: [PATCH 1/6] Don't hang if something goes wrong reading from a tar archive

[Corinna Vinschen]

On Apr  8 15:43, Jon TURNEY wrote:
> Returning EIO (a small positive integer) as the result of archive_tar_file::read()
> on an error isn't a good idea, it's indistinguishable from successfully reading
> a small number of bytes.
> 
> This causes io_stream::copy() to spin forever if it's reading from an archive stream
> which terminates unexpectedly early, which can happen on an decompression error.
> 
> So, return -1 and set lasterr instead.  This matches the behaviour of POSIX
> read(), so shouldn't surprise anyone :-)
> 
> Make the same change for archive_tar_file::write(), archive_tar_file::tell()
> and archive_tar_file::peek() for consistency
> 
> 2011-04-08  Jon TURNEY  <...>
> 
> 	* archive_tar_file.cc (read, write, peek, seek): Consistently return -1
> 	and set lasterr on an error.

Please commit.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

Re: [PATCH 2/6] Consistently return -1 and set lasterr instead in io_stream derived classes

[Corinna Vinschen]

On Apr  8 15:43, Jon TURNEY wrote:
> 2011-04-08  Jon TURNEY  <...>
> 
> 	* compress_bz.cc (read, peek): Consistently return -1 and set lasterr
> 	on an error.
> 	* compress_gz.cc (read, write, peek): Ditto.

This one, too.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

Re: [PATCH 3/6] Propagate errors out of io_stream::copy()

[Corinna Vinschen]

On Apr  8 15:43, Jon TURNEY wrote:
> Despite the comment here, io_stream::read() is now implemented to provide the
> same behaviour as POSIX read(): If the stream is at EOF, 0 is returned. If an
> error occurred, -1 is returned.  So we can detect errors and propagate them to
> our caller.
> 
> 2011-04-08  Jon TURNEY  <...>
> 
> 	* io_stream.cc (copy): Propagate errors.

Shoot.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

Re: [PATCH 4/6] Report failure extracting a file from package to the user

[Corinna Vinschen]

On Apr  8 15:43, Jon TURNEY wrote:
> At the moment, all errors in archive::extract_file() are assumed to be due
> to a failure to open the file for writing due to it being opened by another
> process.
> 
> Distinguish when the error is due to an inability to read the file from the
> source archive, and report that.
> 
> 2011-04-08  Jon TURNEY  <...>
> 
> 	* install.cc (extract_replace_on_reboot): New function containg code
> 	extracted from...
> 	(installOne): Report read errors differently to write errors
> 	* archive.cc (extract_file): Distinguish read errors from write errors

This loooks good to me.  Just a question...

> +          /* XXX FIXME: prefix may not be / for in use files -
> +           * although it most likely is
> +           * - we need a io method to get win32 paths
> +           * or to wrap this system call

This comment doesn't make any sense to me anymore.  The incoming path
should already be a full Win32 path.  The function wouldn't work without
it and AFAIK the paths from the tar file are converted to full Win32
paths anyway.  There's also no reason to wrap this system call any further.
Bottom line is, just drop the comment, please.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

Re: [PATCH 5/6] Handle bzip2 multi-stream files

[Corinna Vinschen]

On Apr  8 15:43, Jon TURNEY wrote:
> bzip2 compressed files are allowed to contain multiple streams of
> compressed data.  So if we arrive at BZ_STREAM_END but are not at
> EOF, restart the decompressor.  Decompress any data which remains
> in the stream before reading any more.

I take this at face value since I'm not fluent in bzip compression.
So, if that helps to circumvent a potential problem, just go ahead.
Does that really occur in some of our tar.bz files?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

Re: [PATCH 6/6] Handle short reads in archive_tar_file::read()

[Corinna Vinschen]

On Apr  8 15:43, Jon TURNEY wrote:
> archive_tar_file::read() currently considers any short read an error.
> 
> These can now occur if the underlying bz2 compressed io_stream starts a
> new compression stream in the middle of the read
> 
> It also transparently reads to the next 512-byte block in the parent
> io_stream after reading all the data expected for a file.
> 
> Teach it to do things correctly, even if a short read occurs.
> 
> 2011-04-08  Jon TURNEY  <...>
>
> 	* archive_tar_file.cc (read): Handle short reads

Same here.  I guess it goes without saying that you tested it...


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

Re: [PATCH 5/6] Handle bzip2 multi-stream files

[Charles Wilson]

On 4/20/2011 12:39 PM, Corinna Vinschen wrote:
> On Apr  8 15:43, Jon TURNEY wrote:
>> bzip2 compressed files are allowed to contain multiple streams of
>> compressed data.  So if we arrive at BZ_STREAM_END but are not at
>> EOF, restart the decompressor.  Decompress any data which remains
>> in the stream before reading any more.
> 
> I take this at face value since I'm not fluent in bzip compression.
> So, if that helps to circumvent a potential problem, just go ahead.
> Does that really occur in some of our tar.bz files?

Apparently it happens when pbzip [*] is used (on a cross build
environment, since cygwin does not actually ship pbzip) to compress the
tarball.

[*] Parallel BZip2: http://www.compression.ca/pbzip2/

--
Chuck

Re: [PATCH 4/6] Report failure extracting a file from package to the user

[Jon TURNEY]

On 20/04/2011 17:36, Corinna Vinschen wrote:
> On Apr  8 15:43, Jon TURNEY wrote:
>> At the moment, all errors in archive::extract_file() are assumed to be due
>> to a failure to open the file for writing due to it being opened by another
>> process.
>>
>> Distinguish when the error is due to an inability to read the file from the
>> source archive, and report that.
>>
>> 2011-04-08  Jon TURNEY  <...>
>>
>> 	* install.cc (extract_replace_on_reboot): New function containg code
>> 	extracted from...
>> 	(installOne): Report read errors differently to write errors
>> 	* archive.cc (extract_file): Distinguish read errors from write errors
> 
> This loooks good to me.  Just a question...
> 
>> +          /* XXX FIXME: prefix may not be / for in use files -
>> +           * although it most likely is
>> +           * - we need a io method to get win32 paths
>> +           * or to wrap this system call
> 
> This comment doesn't make any sense to me anymore.  The incoming path
> should already be a full Win32 path.  The function wouldn't work without
> it and AFAIK the paths from the tar file are converted to full Win32
> paths anyway.  There's also no reason to wrap this system call any further.
> Bottom line is, just drop the comment, please.

Ok. I've applied patches 1-4, with 4 amended as suggested here.

For convenience, I have generated a snapshot at [1] (Note that this was built
with Charles Wilson's i686-pc-mingw32-gcc 4.5.1 toolchain, as that's what I
have installed)

[1] http://cygwin.com/setup/snapshots/setup-2.745.exe

Re: [PATCH 5/6] Handle bzip2 multi-stream files

[Jon TURNEY]

On 20/04/2011 21:16, Charles Wilson wrote:
> On 4/20/2011 12:39 PM, Corinna Vinschen wrote:
>> On Apr  8 15:43, Jon TURNEY wrote:
>>> bzip2 compressed files are allowed to contain multiple streams of
>>> compressed data.  So if we arrive at BZ_STREAM_END but are not at
>>> EOF, restart the decompressor.  Decompress any data which remains
>>> in the stream before reading any more.
>>
>> I take this at face value since I'm not fluent in bzip compression.

I am no expert either, but from 'man bzip2'

"bunzip2 will correctly decompress a file which is the concatenation of two or
more compressed files. The result is the concatenation of the corresponding
uncompressed files."

The technique which pbzip uses to parallelize compression should be obvious :-)

>> So, if that helps to circumvent a potential problem, just go ahead.
>> Does that really occur in some of our tar.bz files?

I think the only bzip2 files which can have this problem are those which have
been generated using pbzip2, hence this bug has been latent until that was
tried, as reported at the beginning of this thread.

> Apparently it happens when pbzip [*] is used (on a cross build
> environment, since cygwin does not actually ship pbzip) to compress the
> tarball.

I suspect that pbzip2 is available in cygwinports :-)

Re: [PATCH 6/6] Handle short reads in archive_tar_file::read()

[Jon TURNEY]

On 20/04/2011 17:40, Corinna Vinschen wrote:
> On Apr  8 15:43, Jon TURNEY wrote:
>> archive_tar_file::read() currently considers any short read an error.
>>
>> These can now occur if the underlying bz2 compressed io_stream starts a
>> new compression stream in the middle of the read
>>
>> It also transparently reads to the next 512-byte block in the parent
>> io_stream after reading all the data expected for a file.
>>
>> Teach it to do things correctly, even if a short read occurs.
>>
>> 2011-04-08  Jon TURNEY  <...>
>>
>> 	* archive_tar_file.cc (read): Handle short reads
> 
> Same here.  I guess it goes without saying that you tested it...

I've tested that a few randomly chosen existing packages can be installed, as
well as testing that pbzip2 compressed packages can be installed.

I suppose I ought to be more exhaustive and check that all current packages
can be installed, let me see if I can find a day or two to do that...

Re: [PATCH 6/6] Handle short reads in archive_tar_file::read()

[Jon TURNEY]

On 21/04/2011 11:07, Jon TURNEY wrote:
> On 20/04/2011 17:40, Corinna Vinschen wrote:
>> On Apr  8 15:43, Jon TURNEY wrote:
>>> archive_tar_file::read() currently considers any short read an error.
>>>
>>> These can now occur if the underlying bz2 compressed io_stream starts a
>>> new compression stream in the middle of the read
>>>
>>> It also transparently reads to the next 512-byte block in the parent
>>> io_stream after reading all the data expected for a file.
>>>
>>> Teach it to do things correctly, even if a short read occurs.
>>>
>>> 2011-04-08  Jon TURNEY  <...>
>>>
>>> 	* archive_tar_file.cc (read): Handle short reads
>>
>> Same here.  I guess it goes without saying that you tested it...
> 
> I've tested that a few randomly chosen existing packages can be installed, as
> well as testing that pbzip2 compressed packages can be installed.
> 
> I suppose I ought to be more exhaustive and check that all current packages
> can be installed, let me see if I can find a day or two to do that...

Ok, I've tested that all current packages install successfully with this
change, so I've applied patches 5 & 6.

For convenience, I have generated a snapshot at [1]

[1] http://cygwin.com/setup/snapshots/setup-2.747.exe

Some observations made installing all packages (was Re: [PATCH 6/6] Handle short reads in archive_tar_file::read())

[Jon TURNEY]

On 24/04/2011 12:29, Jon TURNEY wrote:
> Ok, I've tested that all current packages install successfully with this
> change, so I've applied patches 5 & 6.

current total download size: 1.77GB
current total install size: 5.77GB data, uses 6.32GB disk space on NTFS

There are still a few post-install script failures reported:

* boxes.sh: recently fixed [1]
* libglade2.0.sh: [2]
* exim.sh: looks like this needs an exit 0 at the end [3]
* mined.sh: non-existent registry key [4]

Some packages don't verify with 'cygcheck -c -v':

* apache: installs into /etc/apache.new and /var/www.new/ and moves to
/etc/apache and /var/www/ if they don't already exist.  Perhaps it should use
/etc/defaults and copy.  But I think perhaps this apache1 package should be
made obsolete given it's upstream EOL status and that it was last updated in 2005.

* jadetex: /usr/share/texmf/web2c/jadetex.fmt and pdfjadetex.fmt missing

* mutt: discussed at [5]

* tetex-bin: /usr/bin/latex.exe missing.  The tar file contains a symlink
named latex.exe pointing at pdfetex.exe, but it seems the .exe extension is
removed when the link is created. I guess this will probably be cured by
rebuilding the package.

I also noted a few setup issues:

* The progress gauges use int, so the overall progress gauge is going to break
when the total amount of data hits 2GB (One can already demonstrate this
failure by installing all of cygwinports :-))

* With a large number of packages selected there are a couple of places where
setup doesn't report progress well, patches to follow.

* On one occasion, the unpatched setup 2.738 created a zero-length file rather
than a symlink.  This is suggestive of [6], but I don't really see how that
could also occur within setup.

[1] http://cygwin.com/ml/cygwin-apps/2011-04/msg00044.html
[2] http://cygwin.com/ml/cygwin/2010-10/msg00053.html
[3] http://cygwin.com/ml/cygwin/2011-01/msg00001.html
[4] http://cygwin.com/ml/cygwin/2011-02/msg00334.html
[5] http://cygwin.com/ml/cygwin-apps/2010-11/msg00065.html
[6] http://cygwin.com/ml/cygwin/2010-07/msg00086.html