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

[Volker Simonis]

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?

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
> >>
> >>
> >>
> >>
>