Re: RFR JDK-8206389: JarEntry.setCreation/LastAccessTime without setLastModifiedTime causes Invalid CEN header

[Alan Bateman]

On 07/07/2018 00:46, Xueming Shen wrote:

Hi

Please help review the changeset for JDK-8206389

issue: https://bugs.openjdk.java.net/browse/JDK-8206389
webrev: http://cr.openjdk.java.net/~sherman/8206389/webrev

Cause: ZipOutputStream.writeCEN().writeCEN() incorrectly calculate the 
length
of bytes needed for the  "unix timestamps" when the "last modified 
time" is

NOT set.
The updated webrev looks okay to me, just a minor nit that "elem +=5" is 
missing a space. A suggestion to avoid repeating the comment is to bump 
elem by 5, then += 4 when there is a mtime.


-Alan

Re: RFR JDK-8206389: JarEntry.setCreation/LastAccessTime without setLastModifiedTime causes Invalid CEN header

[Martin Buchholz]

OK, looks good!

I would add something to check() to make sure that e.g. atime == null iff
ze.getLastAccessTime() == null

The zip time stuff is surprisingly messy.

On Fri, Jul 6, 2018 at 8:50 PM, Xueming Shen 
wrote:

> On 7/6/18, 5:43 PM, Martin Buchholz wrote:
>
> I would use different timestamps for the 4 possible times used in this
> test, if only to make it clearer which value comes from where.
>
>
> webrev has been updated accordingly.
>
>
> +// ze.setLastModifiedTime(now);+
> ze.setTime(now.toMillis());
>
>
> setTime only sets the DOS time?  Which only has a granularity of 2
> seconds?  If so, how do you get back the same value you put in if the
> current second is odd?  Or am I misunderstanding the test?
>
>
> no misunderstanding, good catch. The test does fail when hits the odd
> second.
> added a special "check" version for the 2 second granularity set/getTime().
>
> Thanks,
> Sherman
>
>
>

Re: RFR JDK-8206389: JarEntry.setCreation/LastAccessTime without setLastModifiedTime causes Invalid CEN header

[Xueming Shen]

On 7/6/18, 5:43 PM, Martin Buchholz wrote:
I would use different timestamps for the 4 possible times used in this 
test, if only to make it clearer which value comes from where.


webrev has been updated accordingly.



+// ze.setLastModifiedTime(now);
+ze.setTime(now.toMillis());

setTime only sets the DOS time?  Which only has a granularity of 2 
seconds?  If so, how do you get back the same value you put in if the 
current second is odd?  Or am I misunderstanding the test?




no misunderstanding, good catch. The test does fail when hits the odd 
second.

added a special "check" version for the 2 second granularity set/getTime().

Thanks,
Sherman

Re: RFR JDK-8206389: JarEntry.setCreation/LastAccessTime without setLastModifiedTime causes Invalid CEN header

[Martin Buchholz]

I would use different timestamps for the 4 possible times used in this
test, if only to make it clearer which value comes from where.

+// ze.setLastModifiedTime(now);+
ze.setTime(now.toMillis());


setTime only sets the DOS time?  Which only has a granularity of 2
seconds?  If so, how do you get back the same value you put in if the
current second is odd?  Or am I misunderstanding the test?


On Fri, Jul 6, 2018 at 4:46 PM, Xueming Shen 
wrote:

> Hi
>
> Please help review the changeset for JDK-8206389
>
> issue: https://bugs.openjdk.java.net/browse/JDK-8206389
> webrev: http://cr.openjdk.java.net/~sherman/8206389/webrev
>
> Cause: ZipOutputStream.writeCEN().writeCEN() incorrectly calculate the
> length
> of bytes needed for the  "unix timestamps" when the "last modified time" is
> NOT set. The info-zip spec specifies that
>
>   The central-header extra field contains the modification time
> only,
>   or no timestamp at all.  TSize is used to flag its presence or
>   absence.  But note:
>
> So in this case, when "creation time", "last access time" are set but the
> "last modified time" is not. only the "tag", "size" and "flag" should be
> output to
> the extra field, no real "mtime" timestamp, so the total size of the bytes
> needed
> is 5, not 9.
>
> Thanks,
> Sherman
>
>

RFR JDK-8206389: JarEntry.setCreation/LastAccessTime without setLastModifiedTime causes Invalid CEN header

[Xueming Shen]

Hi

Please help review the changeset for JDK-8206389

issue: https://bugs.openjdk.java.net/browse/JDK-8206389
webrev: http://cr.openjdk.java.net/~sherman/8206389/webrev

Cause: ZipOutputStream.writeCEN().writeCEN() incorrectly calculate the 
length

of bytes needed for the  "unix timestamps" when the "last modified time" is
NOT set. The info-zip spec specifies that

  The central-header extra field contains the modification time 
only,

  or no timestamp at all.  TSize is used to flag its presence or
  absence.  But note:

So in this case, when "creation time", "last access time" are set but the
"last modified time" is not. only the "tag", "size" and "flag" should be 
output to
the extra field, no real "mtime" timestamp, so the total size of the 
bytes needed

is 5, not 9.

Thanks,
Sherman