Request for Discussion: 8253952: Work around wrong usage of ZipOutputStream.putNextEntry() in user code
Volker Simonis
volker.simonis at gmail.com
Wed Oct 7 12:19:48 UTC 2020
On Tue, Oct 6, 2020 at 11:46 PM Weijun Wang <WEIJUN.WANG at oracle.com> wrote:
>
>
>
> > 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> 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.
>
> No.
>
> Maybe it’s just that we have being doing this with the “correct way” too long (in jar —update and jarsigner), and I cannot help worrying what problem it might cause.
>
ZipEntry has the following attributes:
String name; // entry name
long xdostime = -1; // last modification time (in extended DOS time,
FileTime mtime; // last modification time, from extra field data
FileTime atime; // last access time, from extra field data
FileTime ctime; // creation time, from extra field data
long crc = -1; // crc-32 of entry data
long size = -1; // uncompressed size of entry data
long csize = -1; // compressed size of entry data
int method = -1; // compression method
int flag = 0; // general purpose flag
byte[] extra; // optional extra field data for entry
String comment; // optional comment string for entry
int extraAttributes = -1; // e.g. POSIX permissions, sym links.
which can be read with:
String getComment()
long getCompressedSize()
long getCrc()
FileTime getCreationTime()
byte[] getExtra()
FileTime getLastAccessTime()
FileTime getLastModifiedTime()
int getMethod()
String getName()
long getSize()
long getTime()
LocalDateTime getTimeLocal()
and set with (the name attribute can only be set in the constructor):
void setComment(String comment)
void setCompressedSize(long csize)
void setCrc(long crc)
ZipEntry setCreationTime(FileTime time)
void setExtra(byte[] extra)
ZipEntry setLastAccessTime(FileTime time)
ZipEntry setLastModifiedTime(FileTime time)
void setMethod(int method)
void setSize(long size)
void setTime(long time)
void setTimeLocal(LocalDateTime time)
I think all the attributes except "csize" (i.e the compressed size)
can be safely copied to a new zip file and will be valid there as
well.
@Lance: I therefore don't think it will be required to mention any
other attribute in the updated api documentation because none of them
requires special handling. Users who want to copy zip entries from one
zip file into another can now safely do so and will by default keep
all the original attributes. I think this is the behaviour most people
expect and intend to have.
> With this problem resolved, do you still think this is a “bad coding pattern”?
I think if we solve the problem with the compressed size the coding
pattern I denoted as "bad" will become valid and behave "as expected".
@Lance @Max: can we please continue the discussion and review on the
new PR at https://github.com/openjdk/jdk/pull/520 (or the
corresponding new mail thread)?
Thank you and best regards,
Volker
>
> Thanks,
> Max
>
> > 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!MApVADAAXvhn79VDKgHNs4bAGksVrN6zvPNYQ3YkeSP9exgdNXwBtda7c6Fsuup9_A$
> > [2] 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!MApVADAAXvhn79VDKgHNs4bAGksVrN6zvPNYQ3YkeSP9exgdNXwBtda7c6GMEWjHoQ$
> >
> >> 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
>
More information about the core-libs-dev
mailing list