Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Wed, 27 Oct 2021 20:16:54 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 43 commits: > > - fix copyright header and typo > - improve documentation of AccessorUtils > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Fall back to the VM native reflection support if method handle cannot be > created > - fix bug id in test > - Merge > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Separate paramFlgas into paramCount and flags fields > - Minor cleanup. Improve javadoc in CallerSensitiveAdapter > - ... and 33 more: > https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b I have already fixed our build with https://github.com/apache/wicket/commit/191de985e22b9e0801d5783fbcfa151a25d29ec8 and https://github.com/apache/wicket/commit/128125f25c33a4d886386291f24ffe2d195007ac Depending on your decision whether to make it possible again to drop `final` for `static` fields I will either revert these changes or not. The main purpose of my report is to let you know about this "regression". - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On 14/11/2021 22:56, Claes Redestad wrote: : Alan: changing `Field.modifiers` still works, but dropping the final modifier is not enough for this to work in the new impl. It won't be hard to adapt to the new world. Users who relies on this today could for example opt-out of the new MH-based impl using `-Djdk.reflect.useDirectMethodHandle=false` and get the old behavior. I've checked using a minimal reproducer I extracted from the Wicket sources that this works. Sure, but I don't think that would be enough as Wicket would also need to open java.lang and java.lang.reflect to allow it continue to access private members of Class and Field. I assume the test started emitting "Illegal reflective access ..." warnings in JDK 9 and it stopped working in JDK 16, and somewhere along the line the maintainers must have added --add-opens to get it to work. It's just not tenable, hopefully the project will find a way to re-write that test. -Alan
Re: RFR: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales [v3]
On Fri, 12 Nov 2021 19:11:16 GMT, Naoto Sato wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Force the jnu encoding to UTF-8 if the original one is not supported > > I reproduced those failures on Debian Linux. Corrected the issue by replacing > unsupported jnu encoding with `UTF-8` in `System::initPhase1`. @naotoj , sorry for bothering you again. Still I got exception. It seems value "jnuEncoding" should be overwritten or set "fastEncoding" to "FAST_UTF_8" on jnu_util.c. Could you check this part again ? $ env LC_ALL=kk_KZ.pt154 ./build/linux-x86_64-server-release/images/jdk/bin/java Hello.java WARNING: The encoding of the underlying platform's file system is not supported: PT154 Error: A JNI error has occurred, please check your installation and try again Exception in thread "main" java.util.ServiceConfigurationError: java.nio.charset.spi.CharsetProvider: Provider sun.nio.cs.ext.ExtendedCharsets not found at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:593) at java.base/java.util.ServiceLoader.loadProvider(ServiceLoader.java:875) at java.base/java.util.ServiceLoader$ModuleServicesLookupIterator.hasNext(ServiceLoader.java:1084) at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1309) at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1393) at java.base/java.nio.charset.Charset$ExtendedProviderHolder$1.run(Charset.java:428) at java.base/java.nio.charset.Charset$ExtendedProviderHolder$1.run(Charset.java:422) at java.base/java.security.AccessController.doPrivileged(AccessController.java:318) at java.base/java.nio.charset.Charset$ExtendedProviderHolder.extendedProviders(Charset.java:422) at java.base/java.nio.charset.Charset$ExtendedProviderHolder.(Charset.java:418) at java.base/java.nio.charset.Charset.lookupExtendedCharset(Charset.java:442) at java.base/java.nio.charset.Charset.lookup2(Charset.java:472) at java.base/java.nio.charset.Charset.lookup(Charset.java:460) at java.base/java.nio.charset.Charset.isSupported(Charset.java:501) at java.base/jdk.internal.loader.NativeLibraries.findBuiltinLib(Native Method) at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:157) at java.base/jdk.internal.loader.NativeLibraries.findFromPaths(NativeLibraries.java:322) at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:289) at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:274) at java.base/jdk.internal.loader.BootLoader.loadLibrary(BootLoader.java:149) at java.base/sun.nio.fs.UnixNativeDispatcher.(UnixNativeDispatcher.java:667) at java.base/sun.nio.fs.UnixFileSystem.(UnixFileSystem.java:65) at java.base/sun.nio.fs.LinuxFileSystem.(LinuxFileSystem.java:39) at java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSystemProvider.java:46) at java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSystemProvider.java:39) at java.base/sun.nio.fs.UnixFileSystemProvider.(UnixFileSystemProvider.java:55) at java.base/sun.nio.fs.LinuxFileSystemProvider.(LinuxFileSystemProvider.java:41) at java.base/sun.nio.fs.DefaultFileSystemProvider.(DefaultFileSystemProvider.java:35) at java.base/java.nio.file.FileSystems$DefaultFileSystemHolder.getDefaultProvider(FileSystems.java:114) at java.base/java.nio.file.FileSystems$DefaultFileSystemHolder$1.run(FileSystems.java:103) at java.base/java.nio.file.FileSystems$DefaultFileSystemHolder$1.run(FileSystems.java:101) at java.base/java.security.AccessController.doPrivileged(AccessController.java:318) at java.base/java.nio.file.FileSystems$DefaultFileSystemHolder.defaultFileSystem(FileSystems.java:101) at java.base/java.nio.file.FileSystems$DefaultFileSystemHolder.(FileSystems.java:94) at java.base/java.nio.file.FileSystems.getDefault(FileSystems.java:183) at java.base/java.nio.file.Path.of(Path.java:147) at java.base/java.nio.file.Paths.get(Paths.java:69) at java.base/jdk.internal.jimage.ImageReaderFactory.(ImageReaderFactory.java:51) at java.base/jdk.internal.module.SystemModuleFinders$SystemImage.(SystemModuleFinders.java:385) at java.base/jdk.internal.module.SystemModuleFinders$SystemModuleReader.findImageLocation(SystemModuleFinders.java:429) at java.base/jdk.internal.module.SystemModuleFinders$SystemModuleReader.read(SystemModuleFinders.java:483) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:809) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassInModuleOrNull(BuiltinClassLoader.java:741) at
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Sun, 14 Nov 2021 19:54:50 GMT, Martin Grigorov wrote: >> Mandy Chung has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 43 commits: >> >> - fix copyright header and typo >> - improve documentation of AccessorUtils >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Fall back to the VM native reflection support if method handle cannot be >> created >> - fix bug id in test >> - Merge >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Separate paramFlgas into paramCount and flags fields >> - Minor cleanup. Improve javadoc in CallerSensitiveAdapter >> - ... and 33 more: >> https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b > > Thank you for the comments! > There is no need to be harsh though! We just made use of old Reflection APIs. > They were working for many years and according to StackOverflow Apache Wicket > is not the only one doing this! > In our case it is done in a test case, so it is easy to rework it. > >> Are there many Wicket tests trying to modify static field fields? > > No! We have just one failing test with JDK 18 b23. > > In someone else's case it might be needed to "hack" a third party library. > > As a good citizen I'm reporting it back to you! @martin-g I wasn't making any judgement. To qualify why this is not something I think anyone should do: poking into reflection internals to make a static final field mutable opens you up to situations where some readers still see the old value. An optimizing compiler (JIT or AOT) may treat the value as constant and propagates it. Future JVM features might decide to be even more aggressive, and trust these fields even more completely. While it's unspecified what should happen after overwriting a static final field with reflection, it does work if you're lucky. The one-off use in Wicket for a test seem benign enough. Alan: changing `Field.modifiers` still works, but dropping the final modifier is not enough for this to work in the new impl. It won't be hard to adapt to the new world. Users who relies on this today could for example opt-out of the new MH-based impl using `-Djdk.reflect.useDirectMethodHandle=false` and get the old behavior. I've checked using a minimal reproducer I extracted from the Wicket sources that this works. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND
On Sun, 14 Nov 2021 15:12:16 GMT, Alan Bateman wrote: >> The ZipOutputStream class may create bogus zip data which cannot be opened >> by the ZipFile. The root cause is how the comment field is stored by the >> ZipOutputStream. According to the zip specification, the comment field >> should not be longer than 0x bytes, and we try to validate the length of >> the comment, but unfortunately, we do this after the comment was assigned >> already. So if the application saves the comment based on the user's input >> and then gets an exception from the ZipOutputStream.setComment() it may >> assume that the comment is too long and it will be ignored, but it will be >> saved as-is to the file. >> >> Please take a look at >> [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117) >> refactoring, and note: >> * The comment field is assigned before the length check >> * The null comment is ignored >> >> The current fix will move the length validation before being assigned and >> will use the null comment as an empty text. Note that the behavior of the >> null parameter is not specified in the method/class/package so we are free >> here to implement it in any way, any thoughts/suggestions on which is better? > > src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 154: > >> 152: } >> 153: } >> 154: this.comment = bytes; > > The implementation change looks okay. I assume this regression slipped > through due to lack of tests. > > The method description doesn't make it clear that the comment can be null > (ZipEntry.setComment has the same issue) so we should fix this while we are > in the area, as a separate JBS of course as it will need a CSR to track the > spec clarification. ZipEntry::setComment indicates that the comment will be truncated if needed and ZipOutputStream takes care of this. Perhaps writeEND() should also be updated to something like: `writeBytes(comment, 0, Math.min(comment.length, 0x))` Which is similar to what happens in writeCEN Yes it would be nice to clarify that a null is accepted by setComment. When null is specified, the comment length is written as 0 - PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Wed, 27 Oct 2021 20:16:54 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 43 commits: > > - fix copyright header and typo > - improve documentation of AccessorUtils > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Fall back to the VM native reflection support if method handle cannot be > created > - fix bug id in test > - Merge > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Separate paramFlgas into paramCount and flags fields > - Minor cleanup. Improve javadoc in CallerSensitiveAdapter > - ... and 33 more: > https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b Thank you for the comments! There is no need to be harsh though! We just made use of old Reflection APIs. They were working for many years and according to StackOverflow Apache Wicket is not the only one doing this! In our case it is done in a test case, so it is easy to rework it. > Are there many Wicket tests trying to modify static field fields? No! We have just one failing test with JDK 18 b23. In someone else's case it might be needed to "hack" a third party library. As a good citizen I'm reporting it back to you! - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v5]
On Wed, 10 Nov 2021 21:19:30 GMT, Naoto Sato wrote:
>> Ichiroh Takiguchi has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains five commits:
>>
>> - 8274544: Langtools command's usage were garbled on Japanese Windows
>> - 8274544: Langtools command's usage were garbled on Japanese Windows
>> - 8274544: Langtools command's usage were garbled on Japanese Windows
>> - 8274544: Langtools command's usage were garbled on Japanese Windows
>> - Langtools command's usage were grabled on Japanese Windows
>
> Good suggestions. Filed a JBS issue:
> https://bugs.openjdk.java.net/browse/JDK-8276970
Hello @naotoj .
For PrintStream.getCharset(), following changes may be required.
+++ src/java.base/share/classes/java/io/OutputStreamWriter.java
+Charset getCharset() {
+return se.getCharset();
+}
+++ src/java.base/share/classes/java/io/PrintStream.java
+public Charset getCharset() {
+return charOut.getCharset();
+}
+++ src/java.base/share/classes/sun/nio/cs/StreamEncoder.java
+public Charset getCharset() {
+return cs;
+}
For javac code, we may not use PrintStream.getCharset() directly because javac
code is compiled by boot compiler.
We need to use reflection, like:
+++ src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java
+private static Charset getCharset(PrintStream ps) {
+try {
+Method getCharset =
PrintStream.class.getDeclaredMethod("getCharset");
+return (Charset)getCharset.invoke(ps);
+} catch (Exception e) {
+return Charset.defaultCharset();
+}
+}
If we add following constructors against PrintWriter, we just change javap and
jshell code.
But I cannot evaluate this code changes.
+++ src/java.base/share/classes/java/io/PrintWriter.java
+public PrintWriter(PrintStream ps) {
+ this((OutputStream)ps, false, ps.getCharset());
+}
+public PrintWriter(PrintStream ps, boolean autoFlush) {
+ this((OutputStream)ps, autoFlush, ps.getCharset());
+}
I really appreciate if you handle this kind of code change via JEP-400.
-
PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND
On Sat, 13 Nov 2021 23:16:22 GMT, Sergey Bylokhov wrote: > The ZipOutputStream class may create bogus zip data which cannot be opened by > the ZipFile. The root cause is how the comment field is stored by the > ZipOutputStream. According to the zip specification, the comment field should > not be longer than 0x bytes, and we try to validate the length of the > comment, but unfortunately, we do this after the comment was assigned > already. So if the application saves the comment based on the user's input > and then gets an exception from the ZipOutputStream.setComment() it may > assume that the comment is too long and it will be ignored, but it will be > saved as-is to the file. > > Please take a look at > [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117) > refactoring, and note: > * The comment field is assigned before the length check > * The null comment is ignored > > The current fix will move the length validation before being assigned and > will use the null comment as an empty text. Note that the behavior of the > null parameter is not specified in the method/class/package so we are free > here to implement it in any way, any thoughts/suggestions on which is better? test/jdk/java/util/zip/ZipOutputStream/EmptyComment.java line 88: > 86: byte[] data = baos.toByteArray(); > 87: // Since the comment is empty this will be two last bytes > 88: int pos = data.length - ZipFile.ENDHDR + ZipFile.ENDCOM; I would probably check that I could find Phil Katz' initials at data.length - ZipFile.ENDHDR test/jdk/java/util/zip/ZipOutputStream/EmptyComment.java line 95: > 93: } > 94: } > 95: } Every source file should end in exactly one newline - PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND
On Sat, 13 Nov 2021 23:16:22 GMT, Sergey Bylokhov wrote: > The ZipOutputStream class may create bogus zip data which cannot be opened by > the ZipFile. The root cause is how the comment field is stored by the > ZipOutputStream. According to the zip specification, the comment field should > not be longer than 0x bytes, and we try to validate the length of the > comment, but unfortunately, we do this after the comment was assigned > already. So if the application saves the comment based on the user's input > and then gets an exception from the ZipOutputStream.setComment() it may > assume that the comment is too long and it will be ignored, but it will be > saved as-is to the file. > > Please take a look at > [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117) > refactoring, and note: > * The comment field is assigned before the length check > * The null comment is ignored > > The current fix will move the length validation before being assigned and > will use the null comment as an empty text. Note that the behavior of the > null parameter is not specified in the method/class/package so we are free > here to implement it in any way, any thoughts/suggestions on which is better? src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 154: > 152: } > 153: } > 154: this.comment = bytes; The implementation change looks okay. I assume this regression slipped through due to lack of tests. The method description doesn't make it clear that the comment can be null (ZipEntry.setComment has the same issue) so we should fix this while we are in the area, as a separate JBS of course as it will need a CSR to track the spec clarification. - PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND
On Sun, 14 Nov 2021 11:18:48 GMT, Jaikiran Pai wrote:
>> The ZipOutputStream class may create bogus zip data which cannot be opened
>> by the ZipFile. The root cause is how the comment field is stored by the
>> ZipOutputStream. According to the zip specification, the comment field
>> should not be longer than 0x bytes, and we try to validate the length of
>> the comment, but unfortunately, we do this after the comment was assigned
>> already. So if the application saves the comment based on the user's input
>> and then gets an exception from the ZipOutputStream.setComment() it may
>> assume that the comment is too long and it will be ignored, but it will be
>> saved as-is to the file.
>>
>> Please take a look at
>> [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117)
>> refactoring, and note:
>> * The comment field is assigned before the length check
>> * The null comment is ignored
>>
>> The current fix will move the length validation before being assigned and
>> will use the null comment as an empty text. Note that the behavior of the
>> null parameter is not specified in the method/class/package so we are free
>> here to implement it in any way, any thoughts/suggestions on which is better?
>
> test/jdk/java/util/zip/ZipOutputStream/EmptyComment.java line 69:
>
>> 67: test(zos -> {
>> 68: try {
>> 69: zos.setComment("X".repeat(count));
>
> Hello @mrserb, I think this is missing a "throw RuntimeException" or some
> such, after this line, to assert that the `setComment` does indeed throw the
> `IllegalArgumentExeption`.
Ideally assertThrows could be used hereto validate that the exception is
thrown as expected
-
PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND
On Sat, 13 Nov 2021 23:16:22 GMT, Sergey Bylokhov wrote:
> The ZipOutputStream class may create bogus zip data which cannot be opened by
> the ZipFile. The root cause is how the comment field is stored by the
> ZipOutputStream. According to the zip specification, the comment field should
> not be longer than 0x bytes, and we try to validate the length of the
> comment, but unfortunately, we do this after the comment was assigned
> already. So if the application saves the comment based on the user's input
> and then gets an exception from the ZipOutputStream.setComment() it may
> assume that the comment is too long and it will be ignored, but it will be
> saved as-is to the file.
>
> Please take a look at
> [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117)
> refactoring, and note:
> * The comment field is assigned before the length check
> * The null comment is ignored
>
> The current fix will move the length validation before being assigned and
> will use the null comment as an empty text. Note that the behavior of the
> null parameter is not specified in the method/class/package so we are free
> here to implement it in any way, any thoughts/suggestions on which is better?
Thank you for the proposed fix. I am curious as to how you encountered this
as comments are rarely used with Zip files.
test/jdk/java/util/zip/ZipOutputStream/EmptyComment.java line 35:
> 33: * @summary Verifies various use cases when the zip comment should be
> empty
> 34: */
> 35: public final class EmptyComment {
Please consider converting this to use TestNG and a DataProvider as it will
simply the code even further
-
Changes requested by lancea (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales [v5]
On Sun, 14 Nov 2021 14:41:21 GMT, Naoto Sato wrote: > Initially, I thought that was the case, but I mistook your suggestion as > setting only `notSupportedJnuEncoding` in initPhase1. Reverted the fallback > location into `initPhase1()`. np, it's always tricky to touch code that executes early in the startup. I think we are good now. - PR: https://git.openjdk.java.net/jdk/pull/6282
Re: RFR: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales [v6]
On Sun, 14 Nov 2021 14:44:59 GMT, Naoto Sato wrote: >> Please review the subject fix. In light of JEP400, Java runtime can/should >> start in UTF-8 charset if the underlying native encoding is not supported. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Move UTF-8 fallback up into initPhase1() Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6282
Re: RFR: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales [v5]
On Sun, 14 Nov 2021 07:23:18 GMT, Alan Bateman wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed indentation
>
> src/java.base/share/classes/java/lang/System.java line 2123:
>
>> 2121: var jnuEncoding = props.getProperty("sun.jnu.encoding");
>> 2122: if (jnuEncoding == null || !Charset.isSupported(jnuEncoding)) {
>> 2123: notSupportedJnuEncoding = jnuEncoding == null ? "null" :
>> jnuEncoding;
>
> I think we are nearly there. When setting notSupportedJnuEncoding then we
> also need to set the sun.jnu.encoding property to "UTF-8", setting it in
> initPhase3 is too late because the file system may be used using initPhase2.
Initially, I thought that was the case, but I mistook your suggestion as
setting only `notSupportedJnuEncoding` in initPhase1. Reverted the fallback
location into `initPhase1()`.
-
PR: https://git.openjdk.java.net/jdk/pull/6282
Re: RFR: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales [v6]
> Please review the subject fix. In light of JEP400, Java runtime can/should > start in UTF-8 charset if the underlying native encoding is not supported. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Move UTF-8 fallback up into initPhase1() - Changes: - all: https://git.openjdk.java.net/jdk/pull/6282/files - new: https://git.openjdk.java.net/jdk/pull/6282/files/26d79e99..3428cdf4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6282=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6282=04-05 Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6282.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6282/head:pull/6282 PR: https://git.openjdk.java.net/jdk/pull/6282
Re: RFR: 8276084: Linux DEB Bundler: release number in outputted .deb file should be optional
On Thu, 11 Nov 2021 04:07:18 GMT, Alexey Semenyuk wrote: > 8276084: Linux DEB Bundler: release number in outputted .deb file should be > optional Marked as reviewed by herrick (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6345
Re: RFR: 8274856: Failing jpackage tests with fastdebug/release build
On Fri, 5 Nov 2021 19:58:01 GMT, Alexey Semenyuk wrote: > The fix is to isolate C++ calls in the separate forked child process on > Linux. > This change requires the passing of JLI command line arguments and values of > environment variables between two processes. Marked as reviewed by herrick (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6283
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND
On Sat, 13 Nov 2021 23:16:22 GMT, Sergey Bylokhov wrote:
> The ZipOutputStream class may create bogus zip data which cannot be opened by
> the ZipFile. The root cause is how the comment field is stored by the
> ZipOutputStream. According to the zip specification, the comment field should
> not be longer than 0x bytes, and we try to validate the length of the
> comment, but unfortunately, we do this after the comment was assigned
> already. So if the application saves the comment based on the user's input
> and then gets an exception from the ZipOutputStream.setComment() it may
> assume that the comment is too long and it will be ignored, but it will be
> saved as-is to the file.
>
> Please take a look at
> [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117)
> refactoring, and note:
> * The comment field is assigned before the length check
> * The null comment is ignored
>
> The current fix will move the length validation before being assigned and
> will use the null comment as an empty text. Note that the behavior of the
> null parameter is not specified in the method/class/package so we are free
> here to implement it in any way, any thoughts/suggestions on which is better?
test/jdk/java/util/zip/ZipOutputStream/EmptyComment.java line 69:
> 67: test(zos -> {
> 68: try {
> 69: zos.setComment("X".repeat(count));
Hello @mrserb, I think this is missing a "throw RuntimeException" or some such,
after this line, to assert that the `setComment` does indeed throw the
`IllegalArgumentExeption`.
-
PR: https://git.openjdk.java.net/jdk/pull/6380
