[PATCH] Java - allow compiling a compressed .jar/.zip archive

Wed Jan 24 11:25:00 GMT 2001

Per Bothner wrote:
> You're skeptical - but you actually don't have a reason to believe
> there is a bug.

Yes I do. It broke the Alpha build, and I've pinpointed the failure in gdb. (I
apologize if that wasn't clear in the first message.)
It fails in find_zip_file_start (zextract.c:266):
 266: if (buffer[0] != 'P' || strncmp (&buffer[1], LOCAL_HDR_SIG, 3))
 267: return -1;
This is called from read_zip_archive, which unfortunately does not test for
errors in the return value, so the actual symptom occurs much later in
read_zip_member().
> If you look at the code you will find that it actually
> does take into account __alignof__(ZipDirectory).

Understood. However __alignof__ actually contributed to the problem, as did
increasing the size of ZipDirectory. Look at what read_zip_archive is doing. 
Instead of allocating memory for each ZipDirectory it overlays them on top of
the central directory buffer. When the filename_length member is written (at
offset 32 in ZipDirectory), it can be anywhere from 33 to 40 in the header
because of structure alignment. That's far enough to clobber the file offset
some of the time, since it is read from offset 42 in the directory header. We
got lucky on x86, and before adding the zipf member to ZipDirectory, because
writing filename_length overwrote a portion of the central directory header we
don't use.
> > Are compressed jar files an essential feature for 3.0?
>> Not essential, but then Java isn't either. This patch adds valuable
> functionality to Java and cannot break non-Java.

Agreed, and I'm happy to have it. But if it breaks Alpha (and probably IA-64
too), is it worth it to commit this now, or put it off? I've spent time on this
I had hoped to spend investigating problems with the new ABI... <sigh>.
This struct/buffer overlapping in read_zip_archive is ugly and fragile, clever
as it may be. If ZipDirectory grows any larger it can start overwriting the
filename too. It seems to me that allocating ZipDirectory on the heap would be
the right thing to do, the memory overhead is negigible and the code cleanup
would be substantial. I can't do that now. The patch below may be good enough
for now (I can't test it until I get home, which is where my aging Alpha lives
these days):
2000年01月24日 Jeff Sturm <jeff.sturm@commerceone.com>
	* zextract.c (read_zip_archive): Read file_offset before writing zipd
	and consequently clobbering the header contents.
Index: zextract.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/java/zextract.c,v
retrieving revision 1.11
diff -u -p -r1.11 zextract.c

--- zextract.c 2001年01月21日 21:50:36 1.11
+++ zextract.c 2001年01月24日 19:05:07
@@ -323,6 +323,7 @@ read_zip_archive (zipf)
 long uncompressed_size = makelong (&dir_ptr[4+C_UNCOMPRESSED_SIZE]);
 long filename_length = makeword (&dir_ptr[4+C_FILENAME_LENGTH]);
 long extra_field_length = makeword (&dir_ptr[4+C_EXTRA_FIELD_LENGTH]);
+ long file_offset = makelong (&dir_ptr[4+C_RELATIVE_OFFSET_LOCAL_HEADER]);
 int unpadded_direntry_length;
 if
((dir_ptr-zipf->central_directory)+filename_length+CREC_SIZE+4>zipf->dir_size)
 return -1;
@@ -337,8 +338,7 @@ read_zip_archive (zipf)
 #else
 #define DIR_ALIGN sizeof(long)
 #endif
- zipd->filestart = find_zip_file_start (zipf->fd, 
- makelong
(&dir_ptr[4+C_RELATIVE_OFFSET_LOCAL_HEADER]));
+ zipd->filestart = find_zip_file_start (zipf->fd, file_offset);
 zipd->filename_offset = CREC_SIZE+4 - dir_last_pad;
 unpadded_direntry_length 
 = zipd->filename_offset + zipd->filename_length + extra_field_length;
--
Jeff Sturm
jeff.sturm@commerceone.com
