When running unzip on certain malformed .zip files, under Valgrind, Valgrind reports warnings about use of uninitialized values. This may indicate a bug in 'unzip'.  I apologize that I have not investigated further, as I'm not familiar with the unzip source code.

I'm attaching three examples of .zip files that trigger these warnings.  Note that I'm using unzip-5.52-12.fc12.x86_64 as installed on Fedora 12.  See https://bugzilla.redhat.com/show_bug.cgi?id=558738 for more.

I'm also attaching the output from Valgrind for the first .zip file, bug1.zip.  (The forum would not accept this attachment with the original post, for some reason.)

I'm attaching the output of Valgrind on the second and third .zip file, bug2.zip and bug3.zip.  (The forum would not accept these in any previous post, for some reason.)

   Complaints against the current released version (6.0) might be more
interesting.  UnZip 5.52 is about five years old.

Thanks for the response.
I get similar Valgrind warnings with unzip 6.0, so this is not specific to unzip 5.52.
This time I'm not going to upload output from Valgrind with source code filenames and line numbers for unzip 6.0.  I can't figure out how to build unzip with "-g" and without stripping the binary; it appears the unzip "make generic" build system ignores the CFLAGS and LD2 options in the Makefile, and I think that after spending 30 minutes wrestling with the obscure build system, I've done about as much as can be expected.  If you know how to use the build system, it should be easy enough; here is the valgrind command line to run:

valgrind -q --leak-check=no unzip -o bug1.zip

> I can't figure out how to build unzip [...]
> [...] obscure build system [...]

   It's not GNU-auto<everything>, and it's not particularly well
documented, but I wouldn't call it particularly obscure, either.

      # Read comments (mostly accurate) in "unix/Makefile".
      make -f unix/Makefile flags
      # Edit "flags".  Look for "CC", "CF", "LFLAGS1", and "LF2".
      make -f unix/Makefile generic

> This time I'm not going to upload output from Valgrind with source
> code filenames and line numbers for unzip 6.0.  [...]

   I'm not likely to download the (apparently modified)
"unzip-5.52-12.fc12.x86_64" source, either, so the line numbers you
reported the first time probably won't help much.

   We tend to put more effort into trying to add useful features and
fixing actual bugs reported by actual users than we put into fixing
vaguely described potential bugs in contrived, artificial test cases.
But, when hit over the head with a concise, helpful problem report,
ideally with a suggested patch, we're usually willing to do a little
work on it

Quoted from sms

# Read comments (mostly accurate) in "unix/Makefile".
      make -f unix/Makefile flags
      # Edit "flags".  Look for "CC", "CF", "LFLAGS1", and "LF2".
      make -f unix/Makefile generic

Ahh, that was what I needed to know!  I didn't realize I should edit the "flags" file after running "make flags".  Thanks.

On documentation: Yeah, this could be documented somewhere.  This procedure (editing the "flags" file) is not documented in README, INSTALL, or the Makefile anywhere.

What I tried that didn't work: I tried changing CFLAGS and LF2 so that it would invoke gcc with "-g" and avoid passing "-s" (so I could generate unstripped binaries with debugging symbols), but the Makefile seemed to ignore my changes, which was quite puzzling.

Anyway, I was now able to generate backtraces for each of these three files.  They're generated with:

$ valgrind -q --leak-check=no ./unzip60/unzip -o bug1.zip > backtrace1.txt 2>&1
$ valgrind -q --leak-check=no ./unzip60/unzip -o bug2.zip > backtrace2.txt 2>&1
$ valgrind -q --leak-check=no ./unzip60/unzip -o bug3.zip > backtrace3.txt 2>&1

I will upload backtrace1.txt, backtrace2.txt, and backtrace3.txt.  Thanks for your time.

Here is backtrace2.txt.

And here is backtrace3.txt.

I'm going to make a wild hypothesis about a potential cause of this Valgrind warning.  Here is a code excerpt from do_string() in fileio.c:2287:



       if ((G.extra_field = (uch *)malloc(length)) == (uch *)NULL) {
            [...irrelevant...]
        } else {
            if (readbuf(__G__ (char *)G.extra_field, length) == 0)
                return PK_EOF;
            /* Looks like here is where extra fields are read */
            getZip64Data(__G__ G.extra_field, length);



readbuf() might read less than the number of bytes requested, in which case it will return the number of bytes read.  However if readbuf() returns some value in the range 1..'length-1', the code above ignores the return value of readbuf() and (incorrectly) assumes that the full 'length' bytes have been read.  Now it tells getZip64Data() that G.extra_field contains 'length' bytes of data, but actually it might have less than that (the rest might be uninitialized).  This looks like a potential bug.

What do you think?  Does this analysis look right to you, or have I gone astray? Caution: I'm making wild unfounded conjectures here; I haven't attempted to verify that this is actually the cause of the bug.

How I found this: I ran


  valgrind --track-origins=yes --read-var-info=yes --db-attach=yes -q --leak-check=no ./unzip60/unzip  -o bug1.zip


and then I peeked around in the source code a little bit.

On a separate topic -- do you happen to maintain an archive of interesting .zip files (for testing purposes, to exercise the many code paths of unzip well)?

Could be a potential bug.  Probably should verify all the data was read.  However, this is in theory being read from the middle of the file, so something regarding the file probably has to change in the middle of reading data, which probably is not likely for a static file sitting on the file system.  Added a note to the TODO list to check this.

We used to have various test archives on the ftp site.  I'd have to check.  It's on the list to put a set of test archives and scripts together.  No idea when we'll get to that.

Ed,
Since z/OS MVS files are mapped as records with a given size (rather than a contiguous bytestream), this does sound like an area where we could be affected.  
There are still unresolved issues in the older zip related to growing existing archives that I need to track down.  I'll keep this one in mind.
Al

Quoted from Al Dunsmuir

Since z/OS MVS files are mapped as records with a given size (rather than a contiguous bytestream), this does sound like an area where we could be affected.

Perhaps, but I think the C runtime calls probably hide that issue from us.  Seems to work now.

Quoted from Al Dunsmuir

There are still unresolved issues in the older zip related to growing existing archives that I need to track down.  I'll keep this one in mind.

If you're referring to the Zip -g option, then definitely of concern.  The -g mode is the only one that updates the original archive file (instead of creating a new temp file first) and doing that reliably and reasonably safely has been a trick.

Ed,

That's the one.  

We had found and fixed a couple of issues related to this on z/OS, but when I went to zip up the 4 source files for zip (C+H) and unzip (C+H) so I could download to work on them at home, I found at least one more:

    zip warning: missing end signature--probably not a zip file (did you 
    zip warning: remember to use binary mode when you transferred it?)   
                                                                      
   zip error: Zip file structure invalid (DD:ZIPFILE)                    

one of the bugs previously fixed was a place where the zip file was being opened in text mode (due to a bogus #define).  That's fatal when not on a bytestream filesystem.   I suspect we didn't get them all, and that has to be fixed as part of my 3.1x work.

Al