Request for Discussion: 8253952: Work around wrong usage of ZipOutputStream.putNextEntry() in user code

Lance Andersen lance.andersen at oracle.com
Tue Oct 6 21:00:58 UTC 2020 

Hi Volker, Max,

Perhaps as part of the clarification and the example of the proper way to copy a ZipEntry, used with putNextEntry, it might be worth clarifying that any values that are needed from the ZipEntry returned from ZipInputStream::getNextEntry should be set(copied) in the new ZipEntry prior to passing the ZipEntry to putNextEntry?   

> On Oct 6, 2020, at 1:50 PM, Volker Simonis <volker.simonis at gmail.com> wrote:
> 
> On Tue, Oct 6, 2020 at 6:00 PM Weijun Wang <WEIJUN.WANG at oracle.com <mailto:WEIJUN.WANG at oracle.com>> wrote:
>> 
>> 
>> 
>>> On Oct 6, 2020, at 11:42 AM, Weijun Wang <WEIJUN.WANG at ORACLE.COM> wrote:
>>> 
>>> 
>>> 
>>>> On Oct 6, 2020, at 10:48 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
>>>> 
>>>> On Tue, Oct 6, 2020 at 4:21 PM Weijun Wang <WEIJUN.WANG at oracle.com> wrote:
>>>>> 
>>>>> Hi Volker,
>>>>> 
>>>>> The size unmatch could throw an exception, but writing an existing ZipEntry into a new ZipOutputStream could have other side effects, including time, comment, and other things we added recently, like POSIX permissions and file type flag (is it a symlink?). Do you meant to copy all these things to the new entry automatically?
>>>>> 
>>>> 
>>>> Hi Max,
>>>> 
>>>> this proposed change is just about fixing the unmatch for the
>>>> compressed size attribute. All the other fields will be left
>>>> untouched.
>>>> 
>>>> Do you see similar problems with other fields which should be
>>>> addressed in a similar way or do you just want to make sure that all
>>>> the other fields are passed and processed correctly?
>>> 
>>> No.
>>> 
>>> I was just saying your code change could encourage more people to use the bad code pattern and it might have other unexpected behaviors. Even the zip command requires this or that option to copy certain attributes. For example, should symlinks always be copied as symlinks?
>> 
>> Sorry, my comparison is not precise. The zip tool is adding files to a zip but your code change is about copying existing zip entries.
>> 
>> Still, I wish people are aware that other attributes are copied as well, and they could be different from version to vesion.
>> 
> 
> As described in the java.util.zp package documentation [1]
> ZipOutputStream is aiming to create zip files according to the
> "Info-ZIP Application Note 970311" [2] with "ZIP64 format extensions"
> [3]. Are you aware of any ZipEntry attributes which won't be valid in
> the target zip file after they have been read from a source zip file.
> I'd be happy to add a warning for them when changing the api
> documentation.
> 
> Best regards,
> Volker
> 
> [1] https://urldefense.com/v3/__http://www.pkware.com/documents/casestudies/APPNOTE.TXT__;!!GqivPVa7Brio!OJfXNZ7-9LMuBGhCX4FjlUeEXLO5uWvKxwcNJXQYhTe4Y9W1-VzQH2Yr4YAGVXwtDA$ <https://urldefense.com/v3/__http://www.pkware.com/documents/casestudies/APPNOTE.TXT__;!!GqivPVa7Brio!OJfXNZ7-9LMuBGhCX4FjlUeEXLO5uWvKxwcNJXQYhTe4Y9W1-VzQH2Yr4YAGVXwtDA$> 
> [2] https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/zip/package-summary.html <https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/zip/package-summary.html>
> [3] https://urldefense.com/v3/__http://www.info-zip.org/doc/appnote-19970311-iz.zip__;!!GqivPVa7Brio!OJfXNZ7-9LMuBGhCX4FjlUeEXLO5uWvKxwcNJXQYhTe4Y9W1-VzQH2Yr4YCEIfG0JQ$ <https://urldefense.com/v3/__http://www.info-zip.org/doc/appnote-19970311-iz.zip__;!!GqivPVa7Brio!OJfXNZ7-9LMuBGhCX4FjlUeEXLO5uWvKxwcNJXQYhTe4Y9W1-VzQH2Yr4YCEIfG0JQ$> 
> 
>> Thanks,
>> Max
>> 
>>> 
>>> Thanks,
>>> Max
>>> 
>>>> 
>>>> Regards,
>>>> Volker
>>>> 
>>>>> Thanks,
>>>>> Max
>>>>> 
>>>>>> On Oct 6, 2020, at 9:56 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
>>>>>> 
>>>>>> On Tue, Oct 6, 2020 at 1:21 PM Lance Andersen <LANCE.ANDERSEN at oracle.com> wrote:
>>>>>>> 
>>>>>>> Hi Volker
>>>>>>> 
>>>>>>> On Oct 6, 2020, at 6:08 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
>>>>>>> 
>>>>>>> On Mon, Oct 5, 2020 at 9:53 PM Lance Andersen <LANCE.ANDERSEN at oracle.com> wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> Hi Volker,
>>>>>>> 
>>>>>>> Thank you for looking into this and creating a draft PR.
>>>>>>> 
>>>>>>> On the surface, I don’t see a reason to introduce a  System property.  What challenges do you see if you used the DataDescriptor unless ZipEntry::setCompressedSize is called?  That seems to address the issue that you discovered without having to introduce a new System property. Or is there an additional concern that is not obvious?  This seems like it would address the problem for existing code that is not well behaved.
>>>>>>> 
>>>>>>> 
>>>>>>> Sure. That's obviously the simplest and best solution and I'd be
>>>>>>> totally happy with it. I just thought there might be a need for
>>>>>>> somehow preserving the old and buggy behaviour, that's why I added the
>>>>>>> system property.
>>>>>>> 
>>>>>>> I've created a simpler version of the fix without property and
>>>>>>> adjusted the @apiNote accordingly:
>>>>>>> 
>>>>>>> 
>>>>>>> Thank you for updating your proposal.  We will need to update the javadoc so that the  description of the change is normative and not part of the API note itself.
>>>>>> 
>>>>>> Changing the updated doc to be normativ instead of just an @apiNote
>>>>>> will make it impossible to downport this change but if you think it is
>>>>>> really necessary I will accept that. I've removed the @apiNote tag
>>>>>> from the java doc and changed the draft PR into a real PR.
>>>>>> 
>>>>>> Without the @apiNote tag I also had to copy the extra documentation
>>>>>> verbatim to JarOutputStream.putNextEntry() which is not very elegant
>>>>>> but inheriting the whole api doc from ZipOutputStream didn't seem
>>>>>> right either. Please let me know if you have a better idea.
>>>>>> 
>>>>>> I'll open a CSR once we've agreed on the exact text for the java doc change.
>>>>>> 
>>>>>> Please let's continue the discussion on the PR or the new mail that
>>>>>> was created for it.
>>>>>> 
>>>>>> Best regards,
>>>>>> Volker
>>>>>> 
>>>>>>> We will also need to create a CSR for the proposed change in the spec and functionality.  I believe we should also create a release note for this change
>>>>>>> 
>>>>>>> Best
>>>>>>> Lance
>>>>>>> 
>>>>>>> 
>>>>>>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/520__;!!GqivPVa7Brio!O7epzMWqwxuS6aPQIllYZLdRTncNGXj5ioqVYY72igGBVGZgUGAHlQnCJ8mhzRoAvw$
>>>>>>> 
>>>>>>> Would you be fine with such a solution?
>>>>>>> 
>>>>>>> Thank you and best regards,
>>>>>>> Volker
>>>>>>> 
>>>>>>> The other option is to add an additional method to ZipOutputStream or ZipEntry to address the issue.
>>>>>>> 
>>>>>>> Thoughts?
>>>>>>> 
>>>>>>> Best
>>>>>>> Lance
>>>>>>> 
>>>>>>> On Oct 5, 2020, at 2:13 PM, Alan Bateman <Alan.Bateman at oracle.com> wrote:
>>>>>>> 
>>>>>>> On 05/10/2020 11:04, Volker Simonis wrote:
>>>>>>> 
>>>>>>> :
>>>>>>> 
>>>>>>> I don't
>>>>>>> think the proposal to add a system property to change the behavior is
>>>>>>> feasible, at least not without changing the spec but even then it would
>>>>>>> be very icky.
>>>>>>> 
>>>>>>> Why do you think so? The answer to this question is especially
>>>>>>> important to me because we are considering doing this in our internal
>>>>>>> version independently of upstream OpenJKD (and potentially also in 8 &
>>>>>>> 11).
>>>>>>> 
>>>>>>> Introducing system properties to change the behavior of APIs and "fix" existing code is really icky. For the main line, then I think it would be good to explore the impact of changing the existing putNextEntry or introducing a new API. The compatibility impact of the former may not be significant but I would expect the spec would be clarified as part of this (that is what I was trying to say above). Exploring new APIs would be good and a method on ZipEntry may not be too bad (you have a reference to the target ZOS in the implementation).
>>>>>>> 
>>>>>>> :
>>>>>>> 
>>>>>>> And just as a side note, we also use the "jdk.util.zip.inhibitZip64"
>>>>>>> property in ZipOutputStream for a similar reason. I just wonder how
>>>>>>> that could pass the CCC process as I couldn't even find any
>>>>>>> documentation for it :)
>>>>>>> 
>>>>>>> ZIP64 was very problematic at the time as it wasn't supported by several tools. The system property was introduced to disable the feature/support so that Java applications didn't create ZIP files that other tools couldn't consume. It would be good to get an up to date picture on ZIP64 support as it might be that this switch can be removed. It pre-dates the CSR  process but there was a CCC at the time.
>>>>>>> 
>>>>>>> -Alan
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> Best
>>>>>>> Lance
>>>>>>> ------------------
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>>>>>>> Oracle Java Engineering
>>>>>>> 1 Network Drive
>>>>>>> Burlington, MA 01803
>>>>>>> Lance.Andersen at oracle.com
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> Best
>>>>>>> Lance
>>>>>>> ------------------
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>>>>>>> Oracle Java Engineering
>>>>>>> 1 Network Drive
>>>>>>> Burlington, MA 01803
>>>>>>> Lance.Andersen at oracle.com





Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com

More information about the core-libs-dev mailing list