RFR JDK-8206389: JarEntry.setCreation/LastAccessTime without setLastModifiedTime causes Invalid CEN header
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
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 <xueming.shen@oracle.com> 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
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
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 <xueming.shen@oracle.com> 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
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
participants (3)
-
Alan Bateman
-
Martin Buchholz
-
Xueming Shen