Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads
Thank for the feedback Peter. Comments inline.
On 22/06/2021 22:40, Peter Firmstone wrote:
Was ever to run with SecurityManager?
I found the issue while porting to jdk8u where Solaris uses a
configuration file with the SunPKCS11 Provider by default - We have
tests to register Providers while SecurityManager is in place. This
failed for SunPKCS11.
When you see an AccessControlException, I'd recommend setting the
following security debug property, so you can capture the
ProtectionDomain that failed the access check:
-Djava.security.debug=access:failure Clearly there's a
ProtectionDomain on the calling threads stack that doesn't have the
necessary permission. If you knew which one it was, you could just
grant it java.lang.RuntimePermission "setContextClassLoader"
permission in the policy file.
Yes - that was one of my first actions. [1]. The jdk.crypto.cryptoki
ProtectionDomain lacks the permission and rightly so IMO. The default
policy doesn't grant "setContextClassLoader" permission to any JDK
module. It's not required when we use InnocuousThread.
In NativeResourceCleaner the original constructor is setting the
Context ClassLoader of the calling thread to null, prior to calling
the Thread superclass constructor, so that both the calling thread and
new thread will nave a null context ClassLoader. In your new
implementation, you are asserting the context class loader of the
created thread is null, without setting the context ClassLoader of the
original calling thread, is that the intended behaviour?
Alternatively you could set the context ClassLoader of the calling
thread to null using a PrivilegedAction, prior to creating the new
thread and restore it?
Use of InnocuousThread is made in various JDK classes for similar
purpose where daemon threads need to be run with limited privilege.
Similar use seen in networking and ldap classes.
If the original intent was to set the context ClassLoader of the new
thread to null, then why not just do that, rather than use an assertion?
InnocuousThread sets this to null. The assert is just a belt and braces
approach which is a useful check during test runs. Again, similar
approach done in other JDK libraries.
If assertions are not enabled it may run with a non null context
ClassLoader? What are the consequences? It appears to me, the fix
has created a bigger problem, rather than fixed it. Just my 2 cents.
see above. We shouldn't have an issue. A non-null classloader would lead
to classloader memory leak in some environments.
regards,
Sean.
We use SecurityManager by default following principles of least
privilege (only the code paths we need to execute), and the original
reported bug is a non problem for us, we would capture the missing
permission and grant it, these are permission grants for Java 16:
grant codebase "jrt:/jdk.crypto.cryptoki"
{
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.util";
};
grant codebase "jrt:/jdk.crypto.ec"
{
permission java.security.SecurityPermission
"putProviderProperty.SunEC";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.jca";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.pkcs";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.util";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.util.math";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.util.math.intpoly";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.x509";
};
Good call making NativeResourceCleaner implement Runnable instead of
extending Thread though.
[1]
access: domain that failed ProtectionDomain (jrt:/jdk.crypto.cryptoki
)
jdk.internal.loader.ClassLoaders$PlatformClassLoader@5433274e
java.security.Permissions@7006c658 (
("java.io.FilePermission" "<>" "read")
("java.net.SocketPermission" "localhost:0" "listen,resolve")
("java.security.SecurityPermission" "clearProviderProperties.*")
("java.security.SecurityPermission"
"getProperty.auth.login.defaultCallbackHandler")
("java.security.SecurityPermission" "putProviderProperty.*")
("java.security.SecurityPermission" "authProvider.*")
("java.security.SecurityPermission" "removeProviderProperty.*")
("java.util.PropertyPermission" "java.specification.version" "read")
("java.util.PropertyPermission" "java.vm.vendor" "read")
("java.util.PropertyPermission" "path.separator" "read")
("java.util.PropertyPermission" "os.version" "read")
("java.util.PropertyPermission" "java.vendor.url" "read")
("java.util.PropertyPermission" "java.vm.name" "read")
("java.util.PropertyPermission" "java.vm.specification.version" "read")
("java.util.PropertyPermission" "os.name" "read")
("java.util.PropertyPermission"
"sun.security.pkcs11.allowSingleThreadedModules" "read")
("java.util.PropertyPermission"
"sun.security.pkcs11.disableKeyExtraction"
Re: RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML [v3]
> Hi all, > > Could you please review the 8268457 bug fixes? > > The problem is that ToHTMLStream applies processing for non-surrogate pairs > to the surrogate pair. > This fix changes the processing for non-surrogate pairs to the else condition. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: remove unnecessally comments and add eof line - Changes: - all: https://git.openjdk.java.net/jdk/pull/4474/files - new: https://git.openjdk.java.net/jdk/pull/4474/files/d5792a87..1183c2f6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4474&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4474&range=01-02 Stats: 14 lines in 4 files changed: 0 ins; 11 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4474.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4474/head:pull/4474 PR: https://git.openjdk.java.net/jdk/pull/4474
Re: RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML [v3]
On Fri, 18 Jun 2021 21:09:39 GMT, Joe Wang wrote: >> src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/ToHTMLStream.java >> line 1454: >> >>> 1452: writer.write(ch); // no escaping in this case >>> 1453: } >>> 1454: else >> >> I was suggesting removing the entire comment-out block if it is not needed >> (and confusing), but I will defer the decision to Joe. > > I agree. It's very obsolete. The comment-out block from line 1445 to 1454 can > be removed. I was mistaken. I deleted the entire comment. - PR: https://git.openjdk.java.net/jdk/pull/4474
Re: RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML [v2]
On Fri, 18 Jun 2021 20:28:06 GMT, Naoto Sato wrote: >> Masanori Yano has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reflect the review comments > > test/jaxp/javax/xml/jaxp/unittest/transform/SurrogateTest1.xml line 4: > >> 2: >> 3: 𠮟 >> 4: > > Add a new line at the end of the file (and other newly created files too). I added a new line at the end of every new file. - PR: https://git.openjdk.java.net/jdk/pull/4474
Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v3]
On Tue, 22 Jun 2021 16:18:11 GMT, Roger Riggs wrote: >> Add java.util.Objects.newIdentity to supply a unique object with identity. >> This is a replacement code can be used today for the traditional new >> Object() idiom, which will be deprecated under Project Valhalla. >> Refer to [JEP 401: Primitive Objects >> (Preview)](https://openjdk.java.net/jeps/401) for background. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright in BasicObjectsTest Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/112
Re: [jdk17] RFR: JDK-8266310: deadlock between System.loadLibrary and JNI FindClass loading another class
On Fri, 18 Jun 2021 09:50:49 GMT, Aleksei Voitylov wrote: > Resubmitting the following PR https://github.com/openjdk/jdk/pull/3976 > against JDK17. > > This fixes the deadlock in ClassLoader between the two lock objects - a lock > object associated with the class being loaded, and the > ClassLoader.loadedLibraryNames hash map, locked during the native library > load operation. > > Further details can be found in the original PR. > > Testing: jtreg and jck testing with no regressions. A new regression test was > developed. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/96
Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v2]
On Tue, 22 Jun 2021 17:27:58 GMT, Naoto Sato wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8269124: Added missing brace; fixed build issue
>
> src/java.base/share/classes/java/time/Instant.java line 562:
>
>> 560: public int get(TemporalField field) {
>> 561: if (field instanceof ChronoField) {
>> 562: return switch ((ChronoField) field) {
>
> Not really comment on the change itself, but Is this a leftover from the
> `instanceof` pattern variable exercise, or will we have another round for
> that? I see other locations which could be leftovers.
Hi Naoto, I decided to only introduce the`instanceof` pattern variable where I
thought it would add additional value to the code. In situations like this one,
I thought there wasn't much point as the cast variable is only used once (in
the switch). However, if you think I've overlooked something that would be
beneficial to change, I'd be happy to take a look.
-
PR: https://git.openjdk.java.net/jdk/pull/4552
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v4]
On Tue, 22 Jun 2021 20:43:03 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updating javadoc, as suggested.
>
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 175:
>
>> 173: * Bootstrap method for linking an {@code invokedynamic} call site
>> that
>> 174: * implements a {@code switch} on a target of an enum type. The
>> static
>> 175: * arguments are used to encode the case labels associated to the
>> switch
>
> `String` and `Class` should appear in code blocks perhaps, or link tags?
> Also, I think this text could be improved by splitting the sentence by using
> a bullet list:
>
>
> The static arguments are used to encode the case labels associated to the
> switch
> construct, where each label can be encoded in two ways:
> * as a String value, which represents the name of the enum constant
> associated with the label
> * as a Class value, which represents the enum type associated with a type
> test pattern
Thanks! I've (tried to) updated the javadoc as suggested.
-
PR: https://git.openjdk.java.net/jdk17/pull/81
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v5]
> Currently, an enum switch with patterns is desugared in a very non-standard, > and potentially slow, way. It would be better to use the standard > `typeSwitch` bootstrap to classify the enum constants. The bootstrap needs to > accept enum constants as labels in order to allow this. A complication is > that if an enum constant is missing, that is not an incompatible change for > the switch, and the switch should simply work as if the case for the missing > constant didn't exist. So, the proposed solution is to have a new bootstrap > `enumConstant` that converts the enum constant name to the enum constant, > returning `null`, if the constant does not exist. It delegates to > `ConstantBootstraps.enumConstant` to do the actual conversion. And > `typeSwitch` accepts `null`s as padding. > > How does this look? Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Improving javadoc. - Changes: - all: https://git.openjdk.java.net/jdk17/pull/81/files - new: https://git.openjdk.java.net/jdk17/pull/81/files/196e106f..0c371364 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=81&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=81&range=03-04 Stats: 13 lines in 1 file changed: 6 ins; 2 del; 5 mod Patch: https://git.openjdk.java.net/jdk17/pull/81.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/81/head:pull/81 PR: https://git.openjdk.java.net/jdk17/pull/81
Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v3]
On Tue, 22 Jun 2021 17:50:05 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review the second half of my update for the `java.time` >> package to make use of switch expressions? >> >> This PR was split into two parts due to the large number of files affected. >> >> Kind regards, >> >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8269124: Added missing return Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/4552
Re: [jdk17] RFR: JDK-8266310: deadlock between System.loadLibrary and JNI FindClass loading another class
On Mon, 21 Jun 2021 16:49:30 GMT, Mandy Chung wrote: >> Resubmitting the following PR https://github.com/openjdk/jdk/pull/3976 >> against JDK17. >> >> This fixes the deadlock in ClassLoader between the two lock objects - a lock >> object associated with the class being loaded, and the >> ClassLoader.loadedLibraryNames hash map, locked during the native library >> load operation. >> >> Further details can be found in the original PR. >> >> Testing: jtreg and jck testing with no regressions. A new regression test >> was developed. > > @dholmes-ora @ChrisHegarty @plevart reviewed PR openjdk/jdk#3976. Can you > re-review this PR? Thank you @mlchung @dholmes-ora @ChrisHegarty for re-reviews. @AlanBateman is it ok with you? - PR: https://git.openjdk.java.net/jdk17/pull/96
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v2]
> Hello, please review this PR; it extend the OSContainer API in order to also
> support the pids controller of cgroups.
>
> I noticed that unlike the other controllers "cpu", "cpuset", "cpuacct",
> "memory" on some older Linux distros (SLES 12.1, RHEL 7.1) the pids
> controller might not be there (or not fully supported) so it was added as
> optional , see the coding
>
>
> if (!cg_infos[PIDS_IDX]._data_complete) {
> log_debug(os, container)("Optional cgroup v1 pids subsystem not found");
> // keep the other controller info, pids is optional
> }
Matthias Baesken has updated the pull request incrementally with one additional
commit since the last revision:
Adjustments following Severins comments
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4518/files
- new: https://git.openjdk.java.net/jdk/pull/4518/files/0e6ecb8e..afd7bf61
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4518&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4518&range=00-01
Stats: 125 lines in 11 files changed: 56 ins; 48 del; 21 mod
Patch: https://git.openjdk.java.net/jdk/pull/4518.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4518/head:pull/4518
PR: https://git.openjdk.java.net/jdk/pull/4518
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v2]
On Wed, 23 Jun 2021 13:37:59 GMT, Matthias Baesken wrote:
>> Hello, please review this PR; it extend the OSContainer API in order to also
>> support the pids controller of cgroups.
>>
>> I noticed that unlike the other controllers "cpu", "cpuset", "cpuacct",
>> "memory" on some older Linux distros (SLES 12.1, RHEL 7.1) the pids
>> controller might not be there (or not fully supported) so it was added as
>> optional , see the coding
>>
>>
>> if (!cg_infos[PIDS_IDX]._data_complete) {
>> log_debug(os, container)("Optional cgroup v1 pids subsystem not found");
>> // keep the other controller info, pids is optional
>> }
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Adjustments following Severins comments
Hi Severin , thanks for all the comments. I prepared a second version with
those changes
added a couple of log_is_enabled checks like you suggested
moved limit_from_str to CgroupSubsystem
added helpers pids_max_val() and swicthed to GET_CONTAINER_INFO_CPTR
pids_max() now returns -1 for unlimited/max , and the -3 is gone
moved limitFromString java coding to
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystem.java
added a better comment to
test/hotspot/jtreg/containers/cgroup/CgroupSubsystemFactory.java about pids
hiearchy values
Regarding your questions about tests, I run the exisiting docker/cgroup related
tests; and also checked
the hs_err output (on SLES/Ubuntu) for new added "maximum number of tasks"
(this is present because systemd cgroup usage).
But I think that the testing needs to be enhanced (e.g. with some added docker
tests?). Do you have some good suggestions
where I could look at existing (docker?) tests and adjust those for the new
pids.max ?
-
PR: https://git.openjdk.java.net/jdk/pull/4518
Re: RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML [v3]
On Wed, 23 Jun 2021 07:34:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8268457 bug fixes? >> >> The problem is that ToHTMLStream applies processing for non-surrogate pairs >> to the surrogate pair. >> This fix changes the processing for non-surrogate pairs to the else >> condition. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > remove unnecessally comments and add eof line The updated changes look reasonable to me. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4474
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v2]
On Wed, 23 Jun 2021 13:38:58 GMT, Matthias Baesken wrote: > But I think that the testing needs to be enhanced (e.g. with some added > docker tests?). Do you have some good suggestions > where I could look at existing (docker?) tests and adjust those for the new > pids.max ? Have a look at `test/hotspot/jtreg/containers/docker/TestMisc.java` which already does some assertions on `print_container_info()` output. Either extend that test with some actual pid limits (`--pids-limit=` option) in place or write a similar one. That would cover the hotspot side. Then consider adding the pids limit to the `-Xshowsettings:system` output (see `LauncherHelper.printSystemMetrics()`) using the Java API and add a docker test using that in `test/jdk/jdk/internal/platform/docker/`. - PR: https://git.openjdk.java.net/jdk/pull/4518
Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v2]
On Wed, 23 Jun 2021 10:39:06 GMT, Patrick Concannon
wrote:
>> src/java.base/share/classes/java/time/Instant.java line 562:
>>
>>> 560: public int get(TemporalField field) {
>>> 561: if (field instanceof ChronoField) {
>>> 562: return switch ((ChronoField) field) {
>>
>> Not really comment on the change itself, but Is this a leftover from the
>> `instanceof` pattern variable exercise, or will we have another round for
>> that? I see other locations which could be leftovers.
>
> Hi Naoto, I decided to only introduce the`instanceof` pattern variable where
> I thought it would add additional value to the code. In situations like this
> one, I thought there wasn't much point as the cast variable is only used once
> (in the switch). However, if you think I've overlooked something that would
> be beneficial to change, I'd be happy to take a look.
I'd personally replace all the applicable locations, as otherwise, it would
confuse why there are two idioms. But it is outside of this PR so probably for
another day.
-
PR: https://git.openjdk.java.net/jdk/pull/4552
Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v3]
On Tue, 22 Jun 2021 17:50:05 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review the second half of my update for the `java.time` >> package to make use of switch expressions? >> >> This PR was split into two parts due to the large number of files affected. >> >> Kind regards, >> >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8269124: Added missing return Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4552
Re: RFR: 4847239: (spec) File.createTempFile() should make it clear that it doesn't create the temporary directory
On Wed, 23 Jun 2021 00:06:25 GMT, Brian Burkhalter wrote: > Augment the specification of > `java.io.File.createTempFile(String,String,File)` to clarify its behavior > with respect to the `File` parameter `directory`. Looks good. Do we have a test case that verifies the behavior? - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4561
Re: RFR: 4847239: (spec) File.createTempFile() should make it clear that it doesn't create the temporary directory
On Wed, 23 Jun 2021 00:06:25 GMT, Brian Burkhalter wrote: > Augment the specification of > `java.io.File.createTempFile(String,String,File)` to clarify its behavior > with respect to the `File` parameter `directory`. Surprisingly it does not look like there is a verifying test. I checked it manually but I had better add a specific test. - PR: https://git.openjdk.java.net/jdk/pull/4561
Re: RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML [v3]
On Wed, 23 Jun 2021 07:34:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8268457 bug fixes? >> >> The problem is that ToHTMLStream applies processing for non-surrogate pairs >> to the surrogate pair. >> This fix changes the processing for non-surrogate pairs to the else >> condition. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > remove unnecessally comments and add eof line Looks good. Thank you for the fix! - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4474
Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v2]
On Wed, 23 Jun 2021 16:35:30 GMT, Naoto Sato wrote: >> Hi Naoto, I decided to only introduce the`instanceof` pattern variable where >> I thought it would add additional value to the code. In situations like this >> one, I thought there wasn't much point as the cast variable is only used >> once (in the switch). However, if you think I've overlooked something that >> would be beneficial to change, I'd be happy to take a look. > > I'd personally replace all the applicable locations, as otherwise, it would > confuse why there are two idioms. But it is outside of this PR so probably > for another day. I agree with Naoto that it's a bit strange. - PR: https://git.openjdk.java.net/jdk/pull/4552
Re: RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML [v3]
On Wed, 23 Jun 2021 07:34:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8268457 bug fixes? >> >> The problem is that ToHTMLStream applies processing for non-surrogate pairs >> to the surrogate pair. >> This fix changes the processing for non-surrogate pairs to the else >> condition. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > remove unnecessally comments and add eof line Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4474
Re: [jdk17] RFR: JDK-8268826: Cleanup Override in Context-Specific Deserialization Filters [v4]
> Remove the unnecessary special case "OVERRIDE" in jdk.serialFilterFactory > property. > Fix description in the example of a filter allowing platform classes. > Suppress some warnings about use of SecurityManager in tests. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Improve exception cases and messages when the jdk.serialFilterFactory is misconfigured and test those faults. Fix typo in java.security-extra-factory test config - Changes: - all: https://git.openjdk.java.net/jdk17/pull/85/files - new: https://git.openjdk.java.net/jdk17/pull/85/files/cf6f5edb..0ac107e4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=85&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=85&range=02-03 Stats: 121 lines in 4 files changed: 112 ins; 4 del; 5 mod Patch: https://git.openjdk.java.net/jdk17/pull/85.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/85/head:pull/85 PR: https://git.openjdk.java.net/jdk17/pull/85
[jdk17] RFR: 8269246: Scoped ByteBuffer vector access
The Foreign Memory API supports viewing a `MemorySegment` as a `ByteBuffer`, an instance of which can then be passed to the vector load/store access methods. Such `ByteBuffer` access requires accesses are scoped (a method annotated with `ScopedMemoryAccess.Scoped`) and the `ByteBuffer`'s scope (instance of `ScopedMemoryAccess.Scope`) checked for validity. Thereby ensuring exceptional failure if the underlying segment is shared and is closed. All Vector tests pass on linux-x64, linux-aarch64, macosx-x64, and windows-x64. - Commit messages: - Move byte buffer test data specifics to separate class - Move scoped methods to ScopedMemoryAccess - 8269246: Scoped ByteBuffer vector access Changes: https://git.openjdk.java.net/jdk17/pull/129/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17&pr=129&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269246 Stats: 455 lines in 42 files changed: 186 ins; 95 del; 174 mod Patch: https://git.openjdk.java.net/jdk17/pull/129.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/129/head:pull/129 PR: https://git.openjdk.java.net/jdk17/pull/129
Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v4]
> Add java.util.Objects.newIdentity to supply a unique object with identity.
> This is a replacement code can be used today for the traditional new Object()
> idiom, which will be deprecated under Project Valhalla.
> Refer to [JEP 401: Primitive Objects
> (Preview)](https://openjdk.java.net/jeps/401) for background.
Roger Riggs has updated the pull request incrementally with one additional
commit since the last revision:
Updated spec of Objects.newIdentity with:
"The class does not override any of the methods of {@code java.lang.Object}."
-
Changes:
- all: https://git.openjdk.java.net/jdk17/pull/112/files
- new: https://git.openjdk.java.net/jdk17/pull/112/files/bc73f6dd..eef1029c
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=112&range=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=112&range=02-03
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk17/pull/112.diff
Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/112/head:pull/112
PR: https://git.openjdk.java.net/jdk17/pull/112
Re: RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML [v3]
On Wed, 23 Jun 2021 07:34:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8268457 bug fixes? >> >> The problem is that ToHTMLStream applies processing for non-surrogate pairs >> to the surrogate pair. >> This fix changes the processing for non-surrogate pairs to the else >> condition. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > remove unnecessally comments and add eof line Marked as reviewed by joehw (Reviewer). Thanks for the update. A full test passed. - PR: https://git.openjdk.java.net/jdk/pull/4474
Re: [jdk17] RFR: 8269246: Scoped ByteBuffer vector access
On Wed, 23 Jun 2021 19:10:41 GMT, Paul Sandoz wrote: > The Foreign Memory API supports viewing a `MemorySegment` as a `ByteBuffer`, > an instance of which can then be passed to the vector load/store access > methods. > > Such `ByteBuffer` access requires accesses are scoped (a method annotated > with `ScopedMemoryAccess.Scoped`) and the `ByteBuffer`'s scope (instance of > `ScopedMemoryAccess.Scope`) checked for validity. Thereby ensuring > exceptional failure if the underlying segment is shared and is closed. > > All Vector tests pass on linux-x64, linux-aarch64, macosx-x64, and > windows-x64. Changes look good - thanks for taking the time to centralize the various vector operations inside ScopedMemoryAccess. - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/129
RFR: 8266407: remove jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES
Enum constant: jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES was not removed when Sealed Classes were made final because the build was failing without it. Now that the feature is final we should be able to safely removed it. This is the intention of this patch. TIA, Vicente - Commit messages: - 8266407: remove jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES Changes: https://git.openjdk.java.net/jdk/pull/4578/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4578&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266407 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4578.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4578/head:pull/4578 PR: https://git.openjdk.java.net/jdk/pull/4578
Re: [jdk17] RFR: JDK-8268826: Cleanup Override in Context-Specific Deserialization Filters [v4]
On Wed, 23 Jun 2021 19:12:06 GMT, Roger Riggs wrote: >> Remove the unnecessary special case "OVERRIDE" in jdk.serialFilterFactory >> property. >> Fix description in the example of a filter allowing platform classes. >> Suppress some warnings about use of SecurityManager in tests. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Improve exception cases and messages when the jdk.serialFilterFactory > is misconfigured and test those faults. > Fix typo in java.security-extra-factory test config Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/85
Re: RFR: 4847239: (spec) File.createTempFile() should make it clear that it doesn't create the temporary directory [v2]
> Augment the specification of > `java.io.File.createTempFile(String,String,File)` to clarify its behavior > with respect to the `File` parameter `directory`. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 4847239: Add test - Changes: - all: https://git.openjdk.java.net/jdk/pull/4561/files - new: https://git.openjdk.java.net/jdk/pull/4561/files/cd931a77..453600c1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4561&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4561&range=00-01 Stats: 106 lines in 1 file changed: 106 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4561.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4561/head:pull/4561 PR: https://git.openjdk.java.net/jdk/pull/4561
Re: RFR: 4847239: (spec) File.createTempFile() should make it clear that it doesn't create the temporary directory
On Wed, 23 Jun 2021 00:06:25 GMT, Brian Burkhalter wrote: > Augment the specification of > `java.io.File.createTempFile(String,String,File)` to clarify its behavior > with respect to the `File` parameter `directory`. Test added. - PR: https://git.openjdk.java.net/jdk/pull/4561
Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads
Thanks Seán,
A good explanation. :)
Solaris was a very good platform for exposing and debugging race
conditions, of course we have very good static analysis now.
Regards,
Peter.
On 23/06/2021 5:10 pm, Seán Coffey wrote:
Thank for the feedback Peter. Comments inline.
On 22/06/2021 22:40, Peter Firmstone wrote:
Was ever to run with SecurityManager?
I found the issue while porting to jdk8u where Solaris uses a
configuration file with the SunPKCS11 Provider by default - We have
tests to register Providers while SecurityManager is in place. This
failed for SunPKCS11.
When you see an AccessControlException, I'd recommend setting the
following security debug property, so you can capture the
ProtectionDomain that failed the access check:
-Djava.security.debug=access:failure Clearly there's a
ProtectionDomain on the calling threads stack that doesn't have the
necessary permission. If you knew which one it was, you could just
grant it java.lang.RuntimePermission "setContextClassLoader"
permission in the policy file.
Yes - that was one of my first actions. [1]. The jdk.crypto.cryptoki
ProtectionDomain lacks the permission and rightly so IMO. The default
policy doesn't grant "setContextClassLoader" permission to any JDK
module. It's not required when we use InnocuousThread.
In NativeResourceCleaner the original constructor is setting the
Context ClassLoader of the calling thread to null, prior to calling
the Thread superclass constructor, so that both the calling thread
and new thread will nave a null context ClassLoader. In your new
implementation, you are asserting the context class loader of the
created thread is null, without setting the context ClassLoader of
the original calling thread, is that the intended behaviour?
Alternatively you could set the context ClassLoader of the calling
thread to null using a PrivilegedAction, prior to creating the new
thread and restore it?
Use of InnocuousThread is made in various JDK classes for similar
purpose where daemon threads need to be run with limited privilege.
Similar use seen in networking and ldap classes.
If the original intent was to set the context ClassLoader of the new
thread to null, then why not just do that, rather than use an assertion?
InnocuousThread sets this to null. The assert is just a belt and
braces approach which is a useful check during test runs. Again,
similar approach done in other JDK libraries.
If assertions are not enabled it may run with a non null context
ClassLoader? What are the consequences? It appears to me, the fix
has created a bigger problem, rather than fixed it. Just my 2 cents.
see above. We shouldn't have an issue. A non-null classloader would
lead to classloader memory leak in some environments.
regards,
Sean.
We use SecurityManager by default following principles of least
privilege (only the code paths we need to execute), and the original
reported bug is a non problem for us, we would capture the missing
permission and grant it, these are permission grants for Java 16:
grant codebase "jrt:/jdk.crypto.cryptoki"
{
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.util";
};
grant codebase "jrt:/jdk.crypto.ec"
{
permission java.security.SecurityPermission
"putProviderProperty.SunEC";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.jca";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.pkcs";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.util";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.util.math";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.util.math.intpoly";
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.x509";
};
Good call making NativeResourceCleaner implement Runnable instead of
extending Thread though.
[1]
access: domain that failed ProtectionDomain (jrt:/jdk.crypto.cryptoki
)
jdk.internal.loader.ClassLoaders$PlatformClassLoader@5433274e
java.security.Permissions@7006c658 (
("java.io.FilePermission" "<>" "read")
("java.net.SocketPermission" "localhost:0" "listen,resolve")
("java.security.SecurityPermission" "clearProviderProperties.*")
("java.security.SecurityPermission"
"getProperty.auth.login.defaultCallbackHandler")
("java.security.SecurityPermission" "putProviderProperty.*")
("java.security.SecurityPermission" "authProvider.*")
("java.security.SecurityPermission" "removeProviderProperty.*")
("java.util.PropertyPermission" "java.specification.version" "read")
("java.util.PropertyPermission" "java.vm.vendor" "read")
("java.util.PropertyPermission" "path.separator" "read")
("java.util.PropertyPermission" "os.version" "read")
("java.util.PropertyPermission" "java.vendor.url" "read")
("java.util.PropertyPermission" "java.vm.name" "read")
("java.util.PropertyPermission" "java.vm.specification.version
Re: RFR: 8268276: Base64 Decoding optimization for x86 using AVX-512 [v7]
On Wed, 23 Jun 2021 00:31:55 GMT, Scott Gibbons wrote: >> Add the Base64 Decode intrinsic for x86 to utilize AVX-512 for acceleration. >> Also allows for performance improvement for non-AVX-512 enabled platforms. >> Due to the nature of MIME-encoded inputs, modify the intrinsic signature to >> accept an additional parameter (isMIME) for fast-path MIME decoding. >> >> A change was made to the signature of DecodeBlock in Base64.java to provide >> the intrinsic information as to whether MIME decoding was being done. This >> allows for the intrinsic to bypass the expensive setup of zmm registers from >> AVX tables, knowing there may be invalid Base64 characters every 76 >> characters or so. A change was also made here removing the restriction that >> the intrinsic must return an even multiple of 3 bytes decoded. This >> implementation handles the pad characters at the end of the string and will >> return the actual number of characters decoded. >> >> The AVX portion of this code will decode in blocks of 256 bytes per loop >> iteration, then in chunks of 64 bytes, followed by end fixup decoding. The >> non-AVX code is an assembly-optimized version of the java DecodeBlock and >> behaves identically. >> >> Running the Base64Decode benchmark, this change increases decode performance >> by an average of 2.6x with a maximum 19.7x for buffers > ~20k. The numbers >> are given in the table below. >> >> **Base Score** is without intrinsic support, **Optimized Score** is using >> this intrinsic, and **Gain** is **Base** / **Optimized**. >> >> >> Benchmark Name | Base Score | Optimized Score | Gain >> -- | -- | -- | -- >> testBase64Decode size 1 | 15.36 | 15.32 | 1.00 >> testBase64Decode size 3 | 17.00 | 16.72 | 1.02 >> testBase64Decode size 7 | 20.60 | 18.82 | 1.09 >> testBase64Decode size 32 | 34.21 | 26.77 | 1.28 >> testBase64Decode size 64 | 54.43 | 38.35 | 1.42 >> testBase64Decode size 80 | 66.40 | 48.34 | 1.37 >> testBase64Decode size 96 | 73.16 | 52.90 | 1.38 >> testBase64Decode size 112 | 84.93 | 51.82 | 1.64 >> testBase64Decode size 512 | 288.81 | 32.04 | 9.01 >> testBase64Decode size 1000 | 560.48 | 40.79 | 13.74 >> testBase64Decode size 2 | 9530.28 | 483.37 | 19.72 >> testBase64Decode size 5 | 24552.24 | 1735.07 | 14.15 >> testBase64MIMEDecode size 1 | 22.87 | 21.36 | 1.07 >> testBase64MIMEDecode size 3 | 27.79 | 25.32 | 1.10 >> testBase64MIMEDecode size 7 | 44.74 | 43.81 | 1.02 >> testBase64MIMEDecode size 32 | 142.69 | 129.56 | 1.10 >> testBase64MIMEDecode size 64 | 256.90 | 243.80 | 1.05 >> testBase64MIMEDecode size 80 | 311.60 | 310.80 | 1.00 >> testBase64MIMEDecode size 96 | 364.00 | 346.66 | 1.05 >> testBase64MIMEDecode size 112 | 472.88 | 394.78 | 1.20 >> testBase64MIMEDecode size 512 | 1814.96 | 1671.28 | 1.09 >> testBase64MIMEDecode size 1000 | 3623.50 | 3227.61 | 1.12 >> testBase64MIMEDecode size 2 | 70484.09 | 64940.77 | 1.09 >> testBase64MIMEDecode size 5 | 191732.34 | 158158.95 | 1.21 >> testBase64WithErrorInputsDecode size 1 | 1531.02 | 1185.19 | 1.29 >> testBase64WithErrorInputsDecode size 3 | 1306.59 | 1170.99 | 1.12 >> testBase64WithErrorInputsDecode size 7 | 1238.11 | 1176.62 | 1.05 >> testBase64WithErrorInputsDecode size 32 | 1346.46 | 1138.47 | 1.18 >> testBase64WithErrorInputsDecode size 64 | 1195.28 | 1172.52 | 1.02 >> testBase64WithErrorInputsDecode size 80 | 1469.00 | 1180.94 | 1.24 >> testBase64WithErrorInputsDecode size 96 | 1434.48 | 1167.74 | 1.23 >> testBase64WithErrorInputsDecode size 112 | 1440.06 | 1162.56 | 1.24 >> testBase64WithErrorInputsDecode size 512 | 1362.79 | 1193.42 | 1.14 >> testBase64WithErrorInputsDecode size 1000 | 1426.07 | 1194.44 | 1.19 >> testBase64WithErrorInputsDecode size 2 | 1398.44 | 1138.17 | 1.23 >> testBase64WithErrorInputsDecode size 5 | 1409.41 | 1114.16 | 1.26 > > Scott Gibbons has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing Windows build warnings I will run our internal testing before approving this. - PR: https://git.openjdk.java.net/jdk/pull/4368
Re: RFR: 8268276: Base64 Decoding optimization for x86 using AVX-512 [v7]
On Wed, 23 Jun 2021 00:31:55 GMT, Scott Gibbons wrote: >> Add the Base64 Decode intrinsic for x86 to utilize AVX-512 for acceleration. >> Also allows for performance improvement for non-AVX-512 enabled platforms. >> Due to the nature of MIME-encoded inputs, modify the intrinsic signature to >> accept an additional parameter (isMIME) for fast-path MIME decoding. >> >> A change was made to the signature of DecodeBlock in Base64.java to provide >> the intrinsic information as to whether MIME decoding was being done. This >> allows for the intrinsic to bypass the expensive setup of zmm registers from >> AVX tables, knowing there may be invalid Base64 characters every 76 >> characters or so. A change was also made here removing the restriction that >> the intrinsic must return an even multiple of 3 bytes decoded. This >> implementation handles the pad characters at the end of the string and will >> return the actual number of characters decoded. >> >> The AVX portion of this code will decode in blocks of 256 bytes per loop >> iteration, then in chunks of 64 bytes, followed by end fixup decoding. The >> non-AVX code is an assembly-optimized version of the java DecodeBlock and >> behaves identically. >> >> Running the Base64Decode benchmark, this change increases decode performance >> by an average of 2.6x with a maximum 19.7x for buffers > ~20k. The numbers >> are given in the table below. >> >> **Base Score** is without intrinsic support, **Optimized Score** is using >> this intrinsic, and **Gain** is **Base** / **Optimized**. >> >> >> Benchmark Name | Base Score | Optimized Score | Gain >> -- | -- | -- | -- >> testBase64Decode size 1 | 15.36 | 15.32 | 1.00 >> testBase64Decode size 3 | 17.00 | 16.72 | 1.02 >> testBase64Decode size 7 | 20.60 | 18.82 | 1.09 >> testBase64Decode size 32 | 34.21 | 26.77 | 1.28 >> testBase64Decode size 64 | 54.43 | 38.35 | 1.42 >> testBase64Decode size 80 | 66.40 | 48.34 | 1.37 >> testBase64Decode size 96 | 73.16 | 52.90 | 1.38 >> testBase64Decode size 112 | 84.93 | 51.82 | 1.64 >> testBase64Decode size 512 | 288.81 | 32.04 | 9.01 >> testBase64Decode size 1000 | 560.48 | 40.79 | 13.74 >> testBase64Decode size 2 | 9530.28 | 483.37 | 19.72 >> testBase64Decode size 5 | 24552.24 | 1735.07 | 14.15 >> testBase64MIMEDecode size 1 | 22.87 | 21.36 | 1.07 >> testBase64MIMEDecode size 3 | 27.79 | 25.32 | 1.10 >> testBase64MIMEDecode size 7 | 44.74 | 43.81 | 1.02 >> testBase64MIMEDecode size 32 | 142.69 | 129.56 | 1.10 >> testBase64MIMEDecode size 64 | 256.90 | 243.80 | 1.05 >> testBase64MIMEDecode size 80 | 311.60 | 310.80 | 1.00 >> testBase64MIMEDecode size 96 | 364.00 | 346.66 | 1.05 >> testBase64MIMEDecode size 112 | 472.88 | 394.78 | 1.20 >> testBase64MIMEDecode size 512 | 1814.96 | 1671.28 | 1.09 >> testBase64MIMEDecode size 1000 | 3623.50 | 3227.61 | 1.12 >> testBase64MIMEDecode size 2 | 70484.09 | 64940.77 | 1.09 >> testBase64MIMEDecode size 5 | 191732.34 | 158158.95 | 1.21 >> testBase64WithErrorInputsDecode size 1 | 1531.02 | 1185.19 | 1.29 >> testBase64WithErrorInputsDecode size 3 | 1306.59 | 1170.99 | 1.12 >> testBase64WithErrorInputsDecode size 7 | 1238.11 | 1176.62 | 1.05 >> testBase64WithErrorInputsDecode size 32 | 1346.46 | 1138.47 | 1.18 >> testBase64WithErrorInputsDecode size 64 | 1195.28 | 1172.52 | 1.02 >> testBase64WithErrorInputsDecode size 80 | 1469.00 | 1180.94 | 1.24 >> testBase64WithErrorInputsDecode size 96 | 1434.48 | 1167.74 | 1.23 >> testBase64WithErrorInputsDecode size 112 | 1440.06 | 1162.56 | 1.24 >> testBase64WithErrorInputsDecode size 512 | 1362.79 | 1193.42 | 1.14 >> testBase64WithErrorInputsDecode size 1000 | 1426.07 | 1194.44 | 1.19 >> testBase64WithErrorInputsDecode size 2 | 1398.44 | 1138.17 | 1.23 >> testBase64WithErrorInputsDecode size 5 | 1409.41 | 1114.16 | 1.26 > > Scott Gibbons has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing Windows build warnings I hit strange failure in compiler/intrinsics/base64/TestBase64.java test on Windows machine which have Intel 8167M cpu (AVX512). # EXCEPTION_ACCESS_VIOLATION (0xc005) at pc=0x7ff92bcbd99e, pid=24628, tid=6804 # # Problematic frame: # V [jvm.dll+0xabd99e] ObjectMonitor::object_peek+0xe # Current thread (0x016c923de2c0): JavaThread "MainThread" [_thread_in_Java, id=6804, stack(0x0060df60,0x0060df70)] Stack: [0x0060df60,0x0060df70], sp=0x0060df6fcb50, free space=1010k Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) V [jvm.dll+0xabd99e] ObjectMonitor::object_peek+0xe (objectMonitor.cpp:304) V [jvm.dll+0xc48d5b] ObjectSynchronizer::quick_enter+0x9b (synchronizer.cpp:331) V [jvm.dll+0xb9b6f6] SharedRuntime::monitor_enter_helper+0x36 (sharedRuntime.cpp:2112) V [jvm.dll+0x389894] Runtime1::monitorenter+0x94 (c1_Runtime1.cpp:748) C 0x016c99c4a757 Java frames: (J=compiled Java code, j=i
