On Mon, 1 Jan 2024 14:29:50 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please review this test-only PR which adds test coverage for >> `ZipFile.getEntry` under certain charset conditions. >> >> When `ZipFile.getEntry` is called for an entry which has the `Language >> encoding flag` general purpose bit flag set, then `ZipCoder.UTF8` is used >> unconditionally, even when a different charset was supplied to the `ZipFile` >> constructor. >> >> It turns out we do not have any testing for this particular case, as can be >> verified by commenting out the following line of code in >> `ZipFile.Source.getEntryPos`: >> >> >> //ZipCoder zc = zipCoderForPos(pos); >> ``` >> >> and then running `make test TEST="test/jdk/java/util/zip"` >> >> The current test verifies that the correct ZipCoder is used by >> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way. >> >> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was >> (accidentally?) fixed by >> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is >> worthwhile to add explicit testing for this case to avoid regressions. >> >> While visiting `ZipCoding.java`, I took the opportunity to convert it to >> JUnit 5. The conversion and modernization of the code is done in the first >> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second >> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code >> required to verify the `Language encoding flag` condition for >> `ZipFile.getEntry`. >> >> Testing: Verified that the test indeed fails when >> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as >> suggested above. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Add @bug tag for 8322802 This is a test-only change PR and the changes being proposed look good to me. On a general note - I hadn't paid attention to this ZipFile constructor which accepts a character encoding, before. I read up on the Zip file specific today (https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT) to understand it a bit more. Interestingly, in that spec (if I read it correctly), unless the `0x0008` "extended language encoding" header is set, there isn't anything that says/allows the entry name or entry comment to be anything other than either UTF-8 or "IBM Code Page 437" character encoding: APPENDIX D - Language Encoding (EFS) ------------------------------------ D.1 The ZIP format has historically supported only the original IBM PC character encoding set, commonly referred to as IBM Code Page 437. This limits storing file name characters to only those within the original MS-DOS range of values and does not properly support file names in other character encodings, or languages. To address this limitation, this specification will support the following change. D.2 If general purpose bit 11 is unset, the file name and comment SHOULD conform to the original ZIP character encoding. If general purpose bit 11 is set, the filename and comment MUST support The Unicode Standard, Version 4.1.0 or greater using the character encoding form defined by the UTF-8 storage specification. .... .... D.3 Applications MAY choose to supplement this file name storage through the use of the 0x0008 Extra Field. Storage for this optional field is currently undefined, however it will be used to allow storing extended information on source or target encoding that MAY further assist applications with file name, or file content encoding tasks. Please contact PKWARE with any requirements on how this field SHOULD be used. D.4 The 0x0008 Extra Field storage MAY be used with either setting for general purpose bit 11. Examples of the intended usage for this field is to store whether "modified-UTF-8" (JAVA) is used, or UTF-8-MAC. Similarly, other commonly used character encoding (code page) designations can be indicated through this field. Formalized values for use of the 0x0008 record remain undefined at this time. The definition for the layout of the 0x0008 field will be published when available. Use of the 0x0008 Extra Field provides for storing data within a ZIP file in an encoding other than IBM Code Page 437 or UTF-8. The change to allow user/application specific arbritary charsets to the `ZipFile` constructor seems to have been done long back in Java 1.7 days as part of https://bugs.openjdk.org/browse/JDK-4244499. This is merely an observation and I don't expect any changes to the ZipFile source or any of your proposed tests, in this context. ------------- PR Comment: https://git.openjdk.org/jdk/pull/17207#issuecomment-1873711203
