On Mon, 14 Aug 2023 17:33:30 GMT, Lance Andersen <lan...@openjdk.org> wrote:
>> This PR updates the extra field validation added as part of >> [JDK-8302483](https://bugs.openjdk.org/browse/JDK-8302483) to deal with >> issues seen with 3rd party tools/libraries where a ZipException may be >> encountered when opening select APK, ZIP or JAR files. Please see refer to >> the links provided at the end the description for the more information :: >> >>> ZipException: Invalid CEN header (invalid zip64 extra data field size) >> >> 1. Extra field includes padding : >> >> >> ----------------#1-------------------- >> [Central Directory Header] >> 0x3374: Signature : 0x02014b50 >> 0x3378: Created Zip Spec : 0xa [1.0] >> 0x3379: Created OS : 0x0 [MS-DOS] >> 0x337a: VerMadeby : 0xa [0, 1.0] >> 0x337b: VerExtract : 0xa [1.0] >> 0x337c: Flag : 0x800 >> 0x337e: Method : 0x0 [STORED] >> 0x3380: Last Mod Time : 0x385ca437 [Thu Feb 28 20:33:46 EST 2008] >> 0x3384: CRC : 0x694c6952 >> 0x3388: Compressed Size : 0x624 >> 0x338c: Uncompressed Size: 0x624 >> 0x3390: Name Length : 0x1b >> 0x3392: Extra Length : 0x7 >> [tag=0xcafe, sz=0, data= ] >> ->[tag=cafe, size=0] >> 0x3394: Comment Length : 0x0 >> 0x3396: Disk Start : 0x0 >> 0x3398: Attrs : 0x0 >> 0x339a: AttrsEx : 0x0 >> 0x339e: Loc Header Offset: 0x0 >> 0x33a2: File Name : res/drawable/size_48x48.jpg >> >> >> The extra field tag of `0xcafe` has its data size set to `0`. and the extra >> length is `7`. It is expected that you can use the tag's data size to >> traverse the extra fields. >> >> >> 2. The [BND tool](https://github.com/bndtools/bnd) added [problematic data >> to the extra field](https://issues.apache.org/jira/browse/FELIX-6622): >> >> >> ----------------#359-------------------- >> [Central Directory Header] >> 0x600b4: Signature : 0x02014b50 >> 0x600b8: Created Zip Spec : 0x14 [2.0] >> 0x600b9: Created OS : 0x0 [MS-DOS] >> 0x600ba: VerMadeby : 0x14 [0, 2.0] >> 0x600bb: VerExtract : 0x14 [2.0] >> 0x600bc: Flag : 0x808 >> 0x600be: Method : 0x8 [DEFLATED] >> 0x600c0: Last Mod Time : 0x2e418983 [Sat Feb 01 17:12:06 EST 2003] >> 0x600c4: CRC : 0xd8f689cb >> 0x600c8: Compressed Size : 0x23e >> 0x600cc: Uncompressed Size: 0x392 >> 0x600d0: Name Length : 0x20 >> 0x600d2: Extra Length : 0x8 >> [tag=0xbfef, sz=61373, data= >> 0x600d4: Comment Length ... > > Lance Andersen has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'master' into extraHeaders-JDK-8313765 > - Minor comment word smithing > - Fix for JDK-8313765 Hi Lance, In general it looks good, but I have some suggestion that I think could slightly improve the patch. src/java.base/share/classes/java/util/zip/ZipFile.java line 1320: > 1318: zerror("Invalid CEN header (invalid zip64 extra data > field size)"); > 1319: } > 1320: // if ZIP64_EXTID blocksize == 0, validate csize and size If you put this block in front of the call to `isZip64ExtBlockSizeValid()` we don't have to handle the `blockSize == 0` case in `isZip64ExtBlockSizeValid()`. This will also make the following comment true again: // Note we do not need to check blockSize is >= 8 as // we know its length is at least 8 from the call to // isZip64ExtBlockSizeValid() src/java.base/share/classes/java/util/zip/ZipFile.java line 1364: > 1362: * As the fields must appear in order, the block size > indicates which > 1363: * fields to expect: > 1364: * 0 - May be written out by Ant and Apache Commons > Compress Library I don't like that `isZip64ExtBlockSizeValid()` still accepts `0` as *valid* input. I think we should fully handle the zero case in `checkZip64ExtraFieldValues()` (also see my comments there). src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 3099: > 3097: throw new ZipException("Invalid CEN header > (invalid zip64 extra data field size)"); > 3098: } > 3099: // if ZIP64_EXTID blocksize == 0, validate csize, > size and Same here. Just put this block before the call to isZip64ExtBlockSizeValid() than you don't have to handle the `sz == 0` case there. src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 3216: > 3214: */ > 3215: return switch(blockSize) { > 3216: case 0, 8, 16, 24, 28 -> true; Don't need to handle the zero case here if you rearrange the code in `readExtra()` as suggested above. ------------- PR Review: https://git.openjdk.org/jdk/pull/15273#pullrequestreview-1577293869 PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293755895 PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293751880 PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293761720 PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293762891