RFR JDK-8186464: ZipFile cannot read some InfoZip ZIP64 zip files

[Martin Buchholz]

Thanks!  Ship it.

On Wed, Sep 6, 2017 at 9:25 AM, Xueming Shen <xueming.shen at oracle.com>
wrote:

> Thanks Martin!
>
> webrev has been updated as suggested (the old webrev has been renamed to
> webrev00)
>
> http://cr.openjdk.java.net/~sherman/8186464/webrev
>
> The only thing is not updated is the centot, we are using it as "int"
> anyway at other place.
> Let's keep it for couple more years till the 32-bit is totally gone.
>
> -Sherman
>
>
> On 9/5/17, 5:51 PM, Martin Buchholz wrote:
>
> Thanks for doing this.
> I'm also hoping to contribute some testing effort, but time ... time ...
> This Looks Good To Me, but as always I have comments.
> ---
> I wouldn't create an echo process.  Just create a zip subprocess and write
> the bytes from java to its stdin.
>
>  181         ProcessBuilder echo = new ProcessBuilder("echo", "hello");
>
> ---
> You should check whether there's a usable zip program, e.g. I once wrote
> code like this:
>
> if (! new File("/usr/bin/perl").canExecute()
>
>  182         ProcessBuilder zip = new ProcessBuilder("zip", path.toString().toString(), "-");
>
> oh wait, now I see
>
>  196         } catch (IOException x) { 197             // ignore, probably from process.start() for "Cannot run program"
>
> well ... I would still add some checks for /usr/bin/zip and elide the
> catch.
> ---
>
>
> """Always specify the charset""" (here "ASCII" is fine).
>
>  163                 if (!"hello".equals(new String(zf.getInputStream(new ZipEntry("hello")) 164                                                  .readAllBytes())))
>
>
> ---
> Can we say something like
> // We must always check for a zip64 EOCD record; it is always permitted to
> be present
>
> 1125                         // try if we have a zip64 end;
>
> ---
> Maybe
> // end64 candidate found
>
> 1139                             // end64 found,
>
> ---
> It's 2017. Time to just make centot a long?
>
> 1152                             end.centot = (int)centot64; // assume total < 2g
>
> ---
> Here's a typo to fix:
>
> entires
>
>
> On Fri, Sep 1, 2017 at 4:17 PM, Xueming Shen <xueming.shen at oracle.com>
> wrote:
>
>> On 8/22/17, 5:54 PM, Martin Buchholz wrote:
>>
>>
>>
>> On Tue, Aug 22, 2017 at 3:27 PM, Xueming Shen <xueming.shen at oracle.com>
>> wrote:
>>
>>>
>>> How about to add an option to our zipfs to force the ZIP64 end record
>>> when
>>> enabled. Harmless if not specified.
>>>
>>
>> I agree that adding more options for testability is good.  Since our
>> users are likely to need such things as well, I'd also like to see such
>> features become public, analogous to zip's -fz- flag.
>>
>> OTOH, interoperability testing is very valuable, so despite the troubles
>> involved writing tests that involve both jdk and zipinfo code, I think we
>> should do it.
>>
>>
>> Martin,
>>
>> I have added a test case that tests "echo hello | zip infozip.zip -" via
>> pb.startPipeline()
>> in ReadZip.java, which should add some coverage for the "interoperability
>> testing".
>>
>> Would you please help take a look?
>>
>> http://cr.openjdk.java.net/~sherman/8186464/webrev/
>>
>> Thanks,
>> Sherman
>>
>
>
>