Re: RFR: 8289552: Make intrinsic conversions between bit representations of half precision values and floats [v13]
On Thu, 6 Oct 2022 06:28:04 GMT, Smita Kamath wrote: >> 8289552: Make intrinsic conversions between bit representations of half >> precision values and floats > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > Updated instruct to use kmovw Latest version v12 passed my tier1-4 testing. Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.org/jdk/pull/9781
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Sat, 8 Oct 2022 15:35:14 GMT, Tagir F. Valeev wrote:
>> Java 17 added RandomGenerator interface. However, existing method
>> Collections.shuffle accepts old java.util.Random class. While since Java 19,
>> it's possible to use Random.from(RandomGenerator) wrapper, it would be more
>> convenient to provide direct overload shuffle(List list, RandomGenerator
>> rnd).
>
> Tagir F. Valeev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove Random -> ThreadLocalRandom change
src/java.base/share/classes/java/util/Collections.java line 485:
> 483: * list-iterator does not support the {@code set} operation.
> 484: * @since 20
> 485: */
It looks like this comment was copied from `shuffle(List, Random)` and
`shuffle(List)` which is fine. But since this method is now preferred over the
others, maybe we can reduce the duplication and have those other methods simply
be defined in terms of this one. We'd have to come up with the right "weasel
words" to describe the source of randomness used by `shuffle(List)`. In
addition, if you don't want to deprecate the other methods, perhaps some
wording can be found that clearly indicates this new method is now the
preferred one.
-
PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8295017: Remove Windows specific workaround in JLI_Snprintf [v3]
On Mon, 10 Oct 2022 11:59:55 GMT, Julian Waters wrote: >> The C99 snprintf is available with Visual Studio 2015 and above, alongside >> Windows 10 and the UCRT, and is no longer identical to the outdated Windows >> _snprintf. Since support for the Visual C++ 2017 compiler was removed a >> while ago, we can now safely remove the compatibility workaround on Windows >> and have JLI_Snprintf simply delegate to snprintf. > > Julian Waters 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 five additional > commits since the last revision: > > - Comment documenting change isn't required > - Merge branch 'openjdk:master' into patch-1 > - Comment formatting > - Remove Windows specific JLI_Snprintf implementation > - Remove Windows JLI_Snprintf definition Looks good. Thanks. - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.org/jdk/pull/10625
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Sat, 8 Oct 2022 15:35:14 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Remove Random -> ThreadLocalRandom change test/jdk/java/util/Collections/Shuffle.java line 66: > 64: RandomGeneratorFactory factory = > RandomGeneratorFactory.getDefault(); > 65: list.sort(null); > 66: Collections.shuffle(list, factory.create(1)); This assumes that the default factory will accept a seed value that initializes its state so that the pseudorandom sequence is repeatable. Not an unreasonable assumption, but `create(long)` essentially says that it can ignore the seed, and `getDefault` says the algorithm may change over time. On the other hand if something does change such that the pseudorandom sequence isn't repeatable, this test will most likely fail immediately. I was poking around for an explicit way to request some kind of PRNG that's guaranteed to be started in repeatable state, but I couldn't find one. Hmmm. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Sat, 8 Oct 2022 15:35:14 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Remove Random -> ThreadLocalRandom change test/jdk/java/util/Collections/Shuffle.java line 92: > 90: throw new RuntimeException(list.getClass() + ": " + list + " > != " + copy); > 91: } > 92: } Is there a way to factor out the `shuffle` calls and thereby collapse these two methods into one? Is it worth it? I'm thinking you could pass in a `Consumer>`. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8291917: Windows - Improve error messages when the C Runtime Libraries or jvm.dll cannot be loaded [v14]
On Mon, 10 Oct 2022 11:58:33 GMT, Julian Waters wrote: >> src/java.base/windows/native/libjli/java_md.h line 48: >> >>> 46: */ >>> 47: >>> 48: void reportWithLastWindowsError(const char* message, ...); >> >> Why does this need to be exported in the header file? Are you expecting >> other code to call this? > > I left that in there since it would possibly be a useful utility to have for > other JLI code that might need to work with Windows errors in the future In that case shouldn't the `JLI_xxx` naming scheme be retained? - PR: https://git.openjdk.org/jdk/pull/9749
Re: [External] : Re: Sequenced Collections
It sounds like you're after some generalized notion of "consistency", and the fact
that offer*() return a boolean whereas add*() do not seems inconsistent.
Unfortunately, the methods have different semantics. After add(obj), obj is *always*
a member of the collection, whereas after offer*(obj), obj *might or might not* be a
member of the collection. The semantics of addFirst/addLast are more closely aligned
with those of the add*() methods on Collection, so I decided to promote
addFirst/addLast to SequencedCollection instead of offerFirst/offerLast.
On your other point, if you want to compare elements of a SequencedCollection to
those of another, in encounter order, you can just use regular Iterators for that.
ListIterator isn't necessary.
s'marks
On 10/5/22 3:36 PM, Ernie Rael wrote:
On 10/5/22 9:34 AM, Stuart Marks wrote:
On 10/4/22 9:38 PM, Ernie Rael wrote:
Summary of key points (maybe the mail was TL;DR)
OK thanks, I was still mulling over the previous email wondering which parts were
significant enough to reply to.
* SequencedCollection methods addFirst,addLast are the only methods in
Collection hierarchy (AFAIK) that might modify the collection and do
not return/signal if the collection was modified. Seems like
offerFirst,offerLast are more consistent with Collections and still
avoid method proliferation.
The problem is that the boolean return values from add() and from offerX() mean
different things, and having them be adjacent on List would be confusing. (Yes,
they're both present on Deque, which is one of the reasons Deque is too
complicated.) And we couldn't adjust the semantics of SequencedCollection.offerX()
to match add(), as that would clash with the existing semantics on Deque.
It is not uncommon for a sub-interface/sub-class to clarify the precise meaning of a
method. If there's a general definition of SequencedCollection.offerFirst(e), then
the Deque definition clarifies the meaning in that context. Consider:
Collections.add(e) - Returns true if this collection changed as a result of the
call
List.add(e) - Return true
Set.add(e) - Returns true if this set did not already contain the specified
element
From your other messages it seems like you want the boolean return value in order
to keep track of whether the collection changed such that it affects equals() and
hashCode().
No, I was just discussing the relationship of change and equals() when working with
a SequencedCollection; it's more observations around using SeqCol. It's interesting
that an addAll() can permute the structure, and end up at the same place.
There are other methods that might modify collections where you can't tell whether
they actually modified the collection; consider Collection::clear or
List::replaceAll.
I'll be more precise: methods that work with a single item return/signal change;
most bulk operators such as removeif(), retainAll(), removeAll(), addAll() also
return/signal change.
My main point is that "void SequencedCollection.addFirst(e)" is inconsistent with
Collections' design. clear() is, well, clear(). replaceAll() seems to be an
exception (a lone exception?).
So I don't think the boolean return value from add/offer is really buying you all
that much.
When I put together a class based on a Collection, I like to follow the general
design pattern. Not sure if/when I may have used the "return change" when using a
collection. But when sub-classing a collection, since everything does it, so do I;
I'll return change in any additional methods I might add. Consistent, least surprise...
* With LinkedHashSet, seems like listIterator is missing. Rather than
making that part of SequencedCollection, maybe an "interface
ListIterable { ListIterator listIterator(int index); }". In addition
to modification, bi-directional might also be optional, to support
it's use with list equals.
ListIterator has indexes and so it's pretty much tied to List. Maybe what you're
missing from LinkedHashSet
I want to be able to do List.equals(SequencedCollection) and vice versa (in
particular with LinkedHashSet). In the list equals() implementations I've looked at,
they all use two ListIterator to do the compare; only forward iteration. For
equals(), I think can wrap the SequencedCollection iterator in a forward,
uni-directional listIterator, a little messy, and use that for equals(); support
from the infrastructure would be nice. Which is where the idea of "ListIterator
Collections.listIterator(iterator, index)" in the other email comes from.
Some daydreaming: For equals(), indexes don't matter except for initialization. And
as far as "index ... tied to list", if SequencedCollection had a listIterator, I
think I could form sub-sequence from that, with only forward iteration. But
sub-SequencedCollection is a different topic. My usage requirement now is for equals().
Lists may include index, but they don't
RFR: 8295104: Break VarHandle tests into separate @test to reduce test execution time
This separates `@run` into its own `@test` that they can be run concurrently. - Commit messages: - 8295104: Break VarHandle tests into separate @test to reduce test execution time Changes: https://git.openjdk.org/jdk/pull/10641/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10641=00 Issue: https://bugs.openjdk.org/browse/JDK-8295104 Stats: 176 lines in 17 files changed: 176 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10641.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10641/head:pull/10641 PR: https://git.openjdk.org/jdk/pull/10641
Re: RFR: JDK-8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance [v2]
> The jarsigner and keytool are localized into English, German, Japanese and > Simplified Chinese. This task is to modify the existing i18n tests to > validate i18n compliance in these tools. > > In addition, this task also contains changes for manual test enhancement and > simplification which originated from > [JDK-8292663](https://bugs.openjdk.org/browse/JDK-8292663. Bill Huang has updated the pull request incrementally with one additional commit since the last revision: Implemented review comments. - Changes: - all: https://git.openjdk.org/jdk/pull/10635/files - new: https://git.openjdk.org/jdk/pull/10635/files/7812f3c4..a36cd005 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10635=01 - incr: https://webrevs.openjdk.org/?repo=jdk=10635=00-01 Stats: 203 lines in 2 files changed: 113 ins; 78 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/10635.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10635/head:pull/10635 PR: https://git.openjdk.org/jdk/pull/10635
Re: RFR: JDK-8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance
On Mon, 10 Oct 2022 19:47:37 GMT, Naoto Sato wrote:
>> The jarsigner and keytool are localized into English, German, Japanese and
>> Simplified Chinese. This task is to modify the existing i18n tests to
>> validate i18n compliance in these tools.
>>
>> In addition, this task also contains changes for manual test enhancement and
>> simplification which originated from
>> [JDK-8292663](https://bugs.openjdk.org/browse/JDK-8292663.
>
> test/jdk/sun/security/tools/keytool/i18n.java line 250:
>
>> 248: private Thread currentThread = null;
>> 249:
>> 250: public static class DialogBuilder {
>
> This seems to be a boilerplate for displaying the panel. Could this be
> separated from the test and converted into some library?
Sure, we can move this code to the library as there is no test code in it.
-
PR: https://git.openjdk.org/jdk/pull/10635
Re: RFR: JDK-8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance
On Mon, 10 Oct 2022 19:45:31 GMT, Naoto Sato wrote: >> The jarsigner and keytool are localized into English, German, Japanese and >> Simplified Chinese. This task is to modify the existing i18n tests to >> validate i18n compliance in these tools. >> >> In addition, this task also contains changes for manual test enhancement and >> simplification which originated from >> [JDK-8292663](https://bugs.openjdk.org/browse/JDK-8292663. > > test/jdk/sun/security/tools/keytool/i18n.java line 62: > >> 60: * @library /test/lib >> 61: * @run main/manual/othervm i18n zh CN >> 62: */ > > Do you need to triplicate these `@test` tags? Would 3-lines of `@run` suffice? > Also setting the locale by `-Duser.language/country` and `getProperty` them > in the main would be preferable to passing them as the test case arguments. Sure, I can make a change to use the system property. Regarding your first question, a test with multiple 'run' fails and terminates on the first test failure. So I would prefer to have multiple tests rather than multiple 'run' in a single test that it allows jtreg to run all the tests independently. - PR: https://git.openjdk.org/jdk/pull/10635
RFR: JDK-8295087: Manual Test to Automated Test Conversion
This task converts 5 manual tests to automated tests. sun/security/provider/PolicyParser/ExtDirsDefaultPolicy.java sun/security/provider/PolicyParser/ExtDirsChange.java sun/security/provider/PolicyParser/ExtDirs.java java/security/Policy/Root/Root.javajava/security/Policy/Root/Root.java javax/crypto/CryptoPermissions/InconsistentEntries.java - Commit messages: - Converted security manual tests to automated tests. Changes: https://git.openjdk.org/jdk/pull/10637/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10637=00 Issue: https://bugs.openjdk.org/browse/JDK-8295087 Stats: 210 lines in 13 files changed: 100 ins; 37 del; 73 mod Patch: https://git.openjdk.org/jdk/pull/10637.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10637/head:pull/10637 PR: https://git.openjdk.org/jdk/pull/10637
Re: RFR: 8289552: Make intrinsic conversions between bit representations of half precision values and floats [v13]
On Thu, 6 Oct 2022 06:28:04 GMT, Smita Kamath wrote: >> 8289552: Make intrinsic conversions between bit representations of half >> precision values and floats > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > Updated instruct to use kmovw I started new testing. - PR: https://git.openjdk.org/jdk/pull/9781
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 10 Oct 2022 19:19:44 GMT, Sean Coffey wrote: >> Agree, and alternatively, it seems cleaner to add a new SharedSecrets class >> for `java.security.Security` and remove the dependency on PD. > > modified code to have Security class hold the initial properties and provided > an accessor method What about creating a new `JavaSecurityPropertiesAccess` class and moving the accessor method there? It seems it would be cleaner to remove the dependency on PD in the long run. - PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: JDK-8294702 - BufferedInputStream uses undefined value range for markpos [v2]
On Sun, 9 Oct 2022 14:04:45 GMT, Markus KARG wrote: >>> This is true, but if a broken subclass sets markpos to less than -1 then >>> lots of code lines will work incorrectly -- including `transferTo` which >>> was the starting point of your change proposal. So do you really only want >>> undo the change _here_, or is it better drop this PR and even use `< 0` in >>> `transferTo`, as I originally proposed? >> >> This looks to be the only that would corrupt the current position (pos). >> >> @bplb plans to look at this too, another set of eyes would be good on this >> point. > > @AlanBateman Checking `< 0` just here, as per your proposal. > > @bplb Kindly requesting `/sponsor`. I don't see any problems here, but I question to a degree the value of the change which is not really fixing any observed broken behavior but cosmetically restricting `markpos` to the specified range `[-1, pos]` instead of `[Integer.MIN_VALUE, pos]`. - PR: https://git.openjdk.org/jdk/pull/10528
Re: RFR: 8294696 - BufferedInputStream.transferTo should drain buffer when mark set [v2]
On Mon, 3 Oct 2022 05:54:31 GMT, Markus KARG wrote: >> This PR implements JDK-8294696. > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > Checking explicitly -1 instead of < 0 Isn't a test needed here which fails without but passes with the proposed change? - PR: https://git.openjdk.org/jdk/pull/10525
Re: RFR: 8289552: Make intrinsic conversions between bit representations of half precision values and floats [v8]
On Fri, 30 Sep 2022 17:24:31 GMT, Vladimir Kozlov wrote: >> @vnkozlov I spoke too soon. All the GHA tests passed in the dummy draft PR I >> created using Smita's patch: >> https://github.com/openjdk/jdk/pull/10500 >> Please take a look. No build failures reported and all tier1 tests passed. > >> @sviswa7 The failure is due to >> [JDK-8293618](https://bugs.openjdk.org/browse/JDK-8293618), @smita-kamath >> please merge with master. Thanks. > > Yes, I tested with latest JDK sources which includes JDK-8293618. @vnkozlov, I have implemented all of the reviewers comments. Could you kindly test this patch? Thanks a lot for your help. - PR: https://git.openjdk.org/jdk/pull/9781
Re: RFR: JDK-8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance
On Mon, 10 Oct 2022 18:18:55 GMT, Bill Huang wrote:
> The jarsigner and keytool are localized into English, German, Japanese and
> Simplified Chinese. This task is to modify the existing i18n tests to
> validate i18n compliance in these tools.
>
> In addition, this task also contains changes for manual test enhancement and
> simplification which originated from
> [JDK-8292663](https://bugs.openjdk.org/browse/JDK-8292663.
Looks good overall. Some minor suggestions.
test/jdk/sun/security/tools/keytool/i18n.java line 62:
> 60: * @library /test/lib
> 61: * @run main/manual/othervm i18n zh CN
> 62: */
Do you need to triplicate these `@test` tags? Would 3-lines of `@run` suffice?
Also setting the locale by `-Duser.language/country` and `getProperty` them in
the main would be preferable to passing them as the test case arguments.
test/jdk/sun/security/tools/keytool/i18n.java line 250:
> 248: private Thread currentThread = null;
> 249:
> 250: public static class DialogBuilder {
This seems to be a boilerplate for displaying the panel. Could this be
separated from the test and converted into some library?
test/jdk/sun/security/tools/keytool/i18n.java line 334:
> 332: } else if (args.length == 2) {
> 333: Locale.setDefault(Locale.of(args[0], args[1]));
> 334: }
Can be eliminated with the suggestion above.
test/jdk/sun/security/tools/keytool/i18n.java line 335:
> 333: Locale.setDefault(Locale.of(args[0], args[1]));
> 334: }
> 335: final String LANG = Locale.getDefault().getDisplayLanguage();
Instead of `getDisplayLanguage()`, it should issue `getDisplayName()`, as for
`zh-CN` case, it simply displays `Chinese` in the current impl. It's ambiguous
whether it is simplified or traditional.
Also, `LANG` should be lowercase, as it is not a constant.
-
PR: https://git.openjdk.org/jdk/pull/10635
Re: RFR: 8295017: Remove Windows specific workaround in JLI_Snprintf [v3]
On Mon, 10 Oct 2022 11:59:55 GMT, Julian Waters wrote: >> The C99 snprintf is available with Visual Studio 2015 and above, alongside >> Windows 10 and the UCRT, and is no longer identical to the outdated Windows >> _snprintf. Since support for the Visual C++ 2017 compiler was removed a >> while ago, we can now safely remove the compatibility workaround on Windows >> and have JLI_Snprintf simply delegate to snprintf. > > Julian Waters 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 five additional > commits since the last revision: > > - Comment documenting change isn't required > - Merge branch 'openjdk:master' into patch-1 > - Comment formatting > - Remove Windows specific JLI_Snprintf implementation > - Remove Windows JLI_Snprintf definition Looks good. - PR: https://git.openjdk.org/jdk/pull/10625
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 10 Oct 2022 07:19:30 GMT, Jaikiran Pai wrote:
>> Sean Coffey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Check for 0 security events
>
> src/jdk.jfr/share/classes/jdk/jfr/events/InitialSecurityPropertyEvent.java
> line 29:
>
>> 27:
>> 28: import jdk.jfr.*;
>> 29: import jdk.jfr.internal.MirrorEvent;
>
> Hello Sean, this `MirrorEvent` appears to be an unused import. Furthermore,
> as far as I know, in `core-libs` the `*` wildcard imports aren't typically
> used. I don't know if it's OK to use it here in the `jdk.jfr` module.
Thanks Jai - I removed the unused import and reverted to non-wildcard import use
> src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 318:
>
>> 316: InitialSecurityPropertyEvent e = new
>> InitialSecurityPropertyEvent();
>> 317: e.key = (String) entry.getKey();
>> 318: e.value = (String) entry.getValue();
>
> To avoid any (odd/unexpected) `ClassCastException` here, perhaps this loop
> can be changed to something like:
>
> for (Set name : p.stringPropertyNames()) {
> InitialSecurityPropertyEvent e = new InitialSecurityPropertyEvent();
> e.key = name;
> e.value = p.getProperty(name);
>
> Since this code anyway wants to deal with string key/values, this wouldn't
> introduce any functional change and yet at the same time prevent any
> unexpected/theoretical `ClassCastException`s
nice - wasn't aware of that method in Properties. Code updated.
-
PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 10 Oct 2022 14:19:18 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/java/security/ProtectionDomain.java line 76:
>>
>>> 74: static class JavaSecurityAccessImpl implements JavaSecurityAccess {
>>> 75: /* cache a copy for recording purposes */
>>> 76: static Properties initialSecurityProperties;
>>
>> This doesn't look very clean. Could the Security class hold the initial
>> security properties and provide an accessor method that
>> JavaSecurityAccess:getIinitialProperties could use?
>
> Agree, and alternatively, it seems cleaner to add a new SharedSecrets class
> for `java.security.Security` and remove the dependency on PD.
modified code to have Security class hold the initial properties and provided
an accessor method
-
PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: 8292177: InitialSecurityProperty JFR event [v4]
> New JFR event to record state of initial security properties. > > Debug output is also now added for these properties via > -Djava.security.debug=properties Sean Coffey has updated the pull request incrementally with one additional commit since the last revision: Address Oct 10 review comments - Changes: - all: https://git.openjdk.org/jdk/pull/10394/files - new: https://git.openjdk.org/jdk/pull/10394/files/f8faecf4..323938d9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10394=03 - incr: https://webrevs.openjdk.org/?repo=jdk=10394=02-03 Stats: 22 lines in 6 files changed: 9 ins; 3 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/10394.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10394/head:pull/10394 PR: https://git.openjdk.org/jdk/pull/10394
RFR: JDK-8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance
The jarsigner and keytool are localized into English, German, Japanese and Simplified Chinese. This task is to modify the existing i18n tests to validate i18n compliance in these tools. In addition, this task also contains changes for manual test enhancement and simplification which originated from [JDK-8292663](https://bugs.openjdk.org/browse/JDK-8292663. - Commit messages: - Update keytool i18n test. Changes: https://git.openjdk.org/jdk/pull/10635/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10635=00 Issue: https://bugs.openjdk.org/browse/JDK-8294994 Stats: 407 lines in 2 files changed: 395 ins; 2 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/10635.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10635/head:pull/10635 PR: https://git.openjdk.org/jdk/pull/10635
Integrated: 8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited
On Tue, 27 Sep 2022 11:43:08 GMT, Pavel Rappo wrote:
> This adds exception documentation to JDK methods that would otherwise lose
> that documentation once JDK-8287796 is integrated. While adding this
> exception documentation now does not change [^1] the JDK API Documentation,
> it will automatically counterbalance the effect that JDK-8287796 will have.
>
> JDK-8287796 seeks to remove an old, undocumented, and esoteric javadoc
> feature that cannot be suppressed [^2]. The feature is so little known about
> that IDEs either do not implement it correctly or do not implement it at all,
> thus rendering documentation differently from how the javadoc tool renders it.
>
> That feature was introduced in JDK-4947455 and manifests as follows. If a
> method inherits documentation for an exception, along with that documentation
> the method automatically inherits documentation for all exceptions that are
> subclasses of that former exception and are documented in an overridden
> method.
>
> To illustrate that behavior, consider the following example. A method
> `Subtype.m` inherits documentation for `SuperException`, while the overridden
> method `Supertype.m` documents `SuperException`, `SubExceptionA` and
> `SubExceptionB`, where the latter two exceptions are subclasses of the former
> exception:
>
> public class Subtype extends Supertype {
>
> @Override
> public void m() throws SuperException {
> ...
>
> public class Supertype {
>
> /**
> * @throws SuperException general problem
> * @throws SubExceptionA specific problem A
> * @throws SubExceptionB specific problem B
> */
> public void m() throws SuperException {
> ...
>
> public class SuperException extends Exception {
> ...
>
> public class SubExceptionA extends SuperException {
> ...
>
> public class SubExceptionB extends SuperException {
> ...
>
> For the above example, the API Documentation for `Subtype.m` will contain
> documentation for all three exceptions; the page fragment for `Subtype.m` in
> "Method Details" will look like this:
>
> public void m()
>throws SuperException
>
> Overrides:
> m in class Supertype
> Throws:
> SuperException - general problem
> SubExceptionA - specific problem A
> SubExceptionB - specific problem B
>
> It works for checked and unchecked exceptions, for methods in classes and
> interfaces.
>
>
> If it's the first time you hear about that behavior and this change affects
> your area, it's a good opportunity to reflect on whether the exception
> documentation this change adds is really needed or you were simply unaware of
> the fact that it was automatically added before. If exception documentation
> is not needed, please comment on this PR. Otherwise, consider approving it.
>
> Keep in mind that if some exception documentation is not needed, **leaving it
> out** from this PR might require a CSR.
>
>
> [^1]: Aside from insignificant changes on class-use and index pages. There's
> one relatively significant change. This change is in jdk.jshell, where some
> methods disappeared from "Methods declared in ..." section and other
> irregularities. We are aware of this and have filed JDK-8291803 to fix the
> root cause.
> [^2]: In reality, the feature can be suppressed, but it looks like a bug
> rather than intentional design. If we legitimize the feature and its
> suppression behavior, the model for exception documentation inheritance might
> become much more complicated.
This pull request has now been integrated.
Changeset: eb90c4fc
Author:Pavel Rappo
URL:
https://git.openjdk.org/jdk/commit/eb90c4fc0479379c8c1452afca8f37746c762e18
Stats: 320 lines in 14 files changed: 310 ins; 0 del; 10 mod
8294377: Prepare to stop auto-inheriting documentation for subclasses of
exceptions whose documentation is inherited
Reviewed-by: jjg
-
PR: https://git.openjdk.org/jdk/pull/10449
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v3]
On Mon, 10 Oct 2022 14:28:07 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> This change introduces new system and security properties for specifying >> factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider >> implementations. >> >> These new properties allow more granular control over the set of object >> factories allowed to reconstruct Java objects from LDAP and RMI contexts. >> >> The new factory filters are supplementing the existing >> `jdk.jndi.object.factoriesFilter` global factories filter to determine if a >> specific object factory is permitted to instantiate objects for the given >> protocol. >> >> Links: >> >> - [CSR with details](https://bugs.openjdk.org/browse/JDK-8291556) >> - [JBS issue](https://bugs.openjdk.org/browse/JDK-8290368) >> >> ### List of code changes >> >> - Implementation for two new system and security properties have been added >> to the `com.sun.naming.internal.ObjectFactoriesFilter` class >> - `java.security` and `module-info.java` files have been updated with a >> documentation for the new properties >> - To keep API of `javax.naming.spi.NamingManager` and >> `javax.naming.spi.DirectoryManager` classes unmodified a new internal >> `com.sun.naming.internal.NamingManagerHelper` class has been introduced. All >> `getObjectInstance` calls have been updated to use the new helper class. >> >> NamingManagerHelper changes >> Changes performed to construct the `NamingManagerHelper` class: >> - `DirectoryManager.getObjectInstance` -> >> `NamingManagerHelper.getDirObjectInstance`. Dependant methods were also >> moved to the `NamingManagerHelper` class >> - `NamingManager.getObjectInstance` -> >> `NamingManagerHelper.getObjectInstance`. Methods responsible for >> setting/getting object factory builder were moved to the >> `NamingManagerHelper` class too. >> >> >> ### Test changes >> New tests have been added for checking that new factory filters can be used >> to restrict reconstruction of Java objects from LDAP and RMI contexts: >> - LDAP protocol specific test: >> test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java >> - RMI protocol specific test: >> test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java >> Existing `test/jdk/javax/naming/module/RunBasic.java` test has been updated >> to allow test-specific factories filter used to reconstruct objects from the >> test LDAP server. >> >> ### Testing >> tier1-tier3 and JNDI regression/JCK tests not showing any failures related >> to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Change checkInput to be the global filter centric Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v3]
> ### Summary of the change > This change introduces new system and security properties for specifying > factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider > implementations. > > These new properties allow more granular control over the set of object > factories allowed to reconstruct Java objects from LDAP and RMI contexts. > > The new factory filters are supplementing the existing > `jdk.jndi.object.factoriesFilter` global factories filter to determine if a > specific object factory is permitted to instantiate objects for the given > protocol. > > Links: > > - [CSR with details](https://bugs.openjdk.org/browse/JDK-8291556) > - [JBS issue](https://bugs.openjdk.org/browse/JDK-8290368) > > ### List of code changes > > - Implementation for two new system and security properties have been added > to the `com.sun.naming.internal.ObjectFactoriesFilter` class > - `java.security` and `module-info.java` files have been updated with a > documentation for the new properties > - To keep API of `javax.naming.spi.NamingManager` and > `javax.naming.spi.DirectoryManager` classes unmodified a new internal > `com.sun.naming.internal.NamingManagerHelper` class has been introduced. All > `getObjectInstance` calls have been updated to use the new helper class. > > NamingManagerHelper changes > Changes performed to construct the `NamingManagerHelper` class: > - `DirectoryManager.getObjectInstance` -> > `NamingManagerHelper.getDirObjectInstance`. Dependant methods were also moved > to the `NamingManagerHelper` class > - `NamingManager.getObjectInstance` -> > `NamingManagerHelper.getObjectInstance`. Methods responsible for > setting/getting object factory builder were moved to the > `NamingManagerHelper` class too. > > > ### Test changes > New tests have been added for checking that new factory filters can be used > to restrict reconstruction of Java objects from LDAP and RMI contexts: > - LDAP protocol specific test: > test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java > - RMI protocol specific test: > test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java > Existing `test/jdk/javax/naming/module/RunBasic.java` test has been updated > to allow test-specific factories filter used to reconstruct objects from the > test LDAP server. > > ### Testing > tier1-tier3 and JNDI regression/JCK tests not showing any failures related to > this change. > No failures observed for the modified regression tests. Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: Change checkInput to be the global filter centric - Changes: - all: https://git.openjdk.org/jdk/pull/10578/files - new: https://git.openjdk.org/jdk/pull/10578/files/091677e9..528489b0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10578=02 - incr: https://webrevs.openjdk.org/?repo=jdk=10578=01-02 Stats: 10 lines in 1 file changed: 2 ins; 5 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/10578.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10578/head:pull/10578 PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v2]
On Mon, 10 Oct 2022 13:14:34 GMT, Daniel Fuchs wrote:
>>> If I'm not mistaken there's no point in checking the specific filter if the
>>> global filter state is REJECTED. So instead of switching on the
>>> specificResult below, maybe you should change the logic to switch on the
>>> globalResult instead and only call the specific filter if needed?
>>
>> Yes - there is no point, and that will reduce number of `checkInput` calls
>> on a specific filter, if it is not the global one. That's how it will look
>> like:
>>
>> private static boolean checkInput(ConfiguredFilter filter, FactoryInfo
>> serialClass) {
>> var globalFilter = GLOBAL_FILTER.filter();
>> var specificFilter = filter.filter();
>> Status globalResult = globalFilter.checkInput(serialClass);
>>
>> // Check if a specific filter is the global one
>> if (filter == GLOBAL_FILTER) {
>> return globalResult == Status.ALLOWED;
>> }
>> return switch (globalResult) {
>> case ALLOWED -> specificFilter.checkInput(serialClass) !=
>> Status.REJECTED;
>> case REJECTED -> false;
>> case UNDECIDED -> specificFilter.checkInput(serialClass) ==
>> Status.ALLOWED;
>> };
>> }
>>
>>
>> The `if (filter == GLOBAL_FILTER) {` check can be also removed but without
>> it the `checkInput` will be called twice on global filter for the case where
>> `specific == global`, ie call from the `checkGlobalFilter`.
>
> The code above LGTM. An alternative could be:
>
>
> private static boolean checkInput(ConfiguredFilter filter, FactoryInfo
> serialClass) {
> var globalFilter = GLOBAL_FILTER.filter();
> var specificFilter = filter.filter();
> Status globalResult = globalFilter.checkInput(serialClass);
>
> return switch (globalResult) {
> case ALLOWED -> filter == GLOBAL ||
> specificFilter.checkInput(serialClass) != Status.REJECTED;
> case REJECTED -> false;
> case UNDECIDED -> filter != GLOBAL &&
> specificFilter.checkInput(serialClass) == Status.ALLOWED;
> };
> }
>
>
> but I believe your proposed code reads better.
Thanks - pushed as 528489b
-
PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 10 Oct 2022 07:31:29 GMT, Alan Bateman wrote:
>> Sean Coffey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Check for 0 security events
>
> src/java.base/share/classes/java/security/ProtectionDomain.java line 76:
>
>> 74: static class JavaSecurityAccessImpl implements JavaSecurityAccess {
>> 75: /* cache a copy for recording purposes */
>> 76: static Properties initialSecurityProperties;
>
> This doesn't look very clean. Could the Security class hold the initial
> security properties and provide an accessor method that
> JavaSecurityAccess:getIinitialProperties could use?
Agree, and alternatively, it seems cleaner to add a new SharedSecrets class for
`java.security.Security` and remove the dependency on PD.
-
PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 10 Oct 2022 11:29:52 GMT, Erik Gahlin wrote:
>> src/jdk.jfr/share/classes/jdk/jfr/events/InitialSecurityPropertyEvent.java
>> line 33:
>>
>>> 31: @Category({"Java Development Kit", "Security"})
>>> 32: @Label("Initial Security Property")
>>> 33: @Name("jdk.InitialSecurityProperty")
>>
>> Should we name this to `jdk.InitialSecurityProperties` and the label to
>> `Initial Security Properties`, to be more accurate?
>
> There is one property per event, so it uses the same naming convention as
> jdk.InitialSystemProperty
You are right indeed - I overlooked the part where this PR loops over these
properties and creates one event per property.
>> src/jdk.jfr/share/classes/jdk/jfr/events/InitialSecurityPropertyEvent.java
>> line 35:
>>
>>> 33: @Name("jdk.InitialSecurityProperty")
>>> 34: @Description("Initial Security Properties")
>>> 35: public final class InitialSecurityPropertyEvent extends
>>> AbstractJDKEvent {
>>
>> The event naming guidelines here
>> https://docs.oracle.com/en/java/javase/17/jfapi/guidelines-naming-and-labeling-events.html
>> recommend leaving out `Event` from the class name. So, maybe we should call
>> this `InitialSecurityProperties`?
>
> The documentation is somewhat misleading. If the event has a name annotation,
> I think it's fine to call the class Event, because name will override the
> class name.
Thank you Erik for that detail. Looks fine to me then.
-
PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v2]
On Mon, 10 Oct 2022 12:07:38 GMT, Aleksei Efimov wrote:
>> src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java
>> line 99:
>>
>>> 97: return globalResult == Status.ALLOWED;
>>> 98: }
>>> 99:
>>
>> If I'm not mistaken there's no point in checking the specific filter if the
>> global filter state is REJECTED. So instead of switching on the
>> specificResult below, maybe you should change the logic to switch on the
>> globalResult instead and only call the specific filter if needed?
>
>> If I'm not mistaken there's no point in checking the specific filter if the
>> global filter state is REJECTED. So instead of switching on the
>> specificResult below, maybe you should change the logic to switch on the
>> globalResult instead and only call the specific filter if needed?
>
> Yes - there is no point, and that will reduce number of `checkInput` calls on
> a specific filter, if it is not the global one. That's how it will look like:
>
> private static boolean checkInput(ConfiguredFilter filter, FactoryInfo
> serialClass) {
> var globalFilter = GLOBAL_FILTER.filter();
> var specificFilter = filter.filter();
> Status globalResult = globalFilter.checkInput(serialClass);
>
> // Check if a specific filter is the global one
> if (filter == GLOBAL_FILTER) {
> return globalResult == Status.ALLOWED;
> }
> return switch (globalResult) {
> case ALLOWED -> specificFilter.checkInput(serialClass) !=
> Status.REJECTED;
> case REJECTED -> false;
> case UNDECIDED -> specificFilter.checkInput(serialClass) ==
> Status.ALLOWED;
> };
> }
>
>
> The `if (filter == GLOBAL_FILTER) {` check can be also removed but without it
> the `checkInput` will be called twice on global filter for the case where
> `specific == global`, ie call from the `checkGlobalFilter`.
The code above LGTM. An alternative could be:
private static boolean checkInput(ConfiguredFilter filter, FactoryInfo
serialClass) {
var globalFilter = GLOBAL_FILTER.filter();
var specificFilter = filter.filter();
Status globalResult = globalFilter.checkInput(serialClass);
return switch (globalResult) {
case ALLOWED -> filter == GLOBAL ||
specificFilter.checkInput(serialClass) != Status.REJECTED;
case REJECTED -> false;
case UNDECIDED -> specificFilter.checkInput(serialClass) ==
Status.ALLOWED;
};
}
but I believe your proposed code reads better.
-
PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v2]
On Mon, 10 Oct 2022 11:16:40 GMT, Daniel Fuchs wrote:
> If I'm not mistaken there's no point in checking the specific filter if the
> global filter state is REJECTED. So instead of switching on the
> specificResult below, maybe you should change the logic to switch on the
> globalResult instead and only call the specific filter if needed?
Yes - there is no point, and that will reduce number of `checkInput` calls on a
specific filter, if it is not the global one. That's how it will look like:
private static boolean checkInput(ConfiguredFilter filter, FactoryInfo
serialClass) {
var globalFilter = GLOBAL_FILTER.filter();
var specificFilter = filter.filter();
Status globalResult = globalFilter.checkInput(serialClass);
// Check if a specific filter is the global one
if (filter == GLOBAL_FILTER) {
return globalResult == Status.ALLOWED;
}
return switch (globalResult) {
case ALLOWED -> specificFilter.checkInput(serialClass) !=
Status.REJECTED;
case REJECTED -> false;
case UNDECIDED -> specificFilter.checkInput(serialClass) ==
Status.ALLOWED;
};
}
The `if (filter == GLOBAL_FILTER) {` check can be also removed but without it
the `checkInput` will be called twice on global filter for the case where
`specific == global`, ie call from the `checkGlobalFilter`.
-
PR: https://git.openjdk.org/jdk/pull/10578
Withdrawn: 8292016: Cleanup legacy error reporting in the JDK outside of HotSpot
On Sun, 14 Aug 2022 16:21:31 GMT, Julian Waters wrote: > A large section of error reporting code in the JDK does not properly handle > WIN32 API errors and instead mixes them with errors originating from C. Since > they can be rather easily replaced and coming up with an elegant solution > proved to be too much of a hassle to be worth it, and some of the concerns > they address no longer are an issue with current versions of the platforms > supported by the JDK, they can be easily removed without much effect. The > remaining utilities that are still needed now instead report directly from > strerror, with a new subsystem for WIN32 errors put in place wherever > required, to minimize confusion when they are used, which was a problem with > earlier solutions to this issue. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/9870
Re: RFR: 8292016: Cleanup legacy error reporting in the JDK outside of HotSpot [v35]
On Mon, 3 Oct 2022 13:37:38 GMT, Julian Waters wrote: >> A large section of error reporting code in the JDK does not properly handle >> WIN32 API errors and instead mixes them with errors originating from C. >> Since they can be rather easily replaced and coming up with an elegant >> solution proved to be too much of a hassle to be worth it, and some of the >> concerns they address no longer are an issue with current versions of the >> platforms supported by the JDK, they can be easily removed without much >> effect. The remaining utilities that are still needed now instead report >> directly from strerror, with a new subsystem for WIN32 errors put in place >> wherever required, to minimize confusion when they are used, which was a >> problem with earlier solutions to this issue. > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Naming Closing - Will narrow the fix to JLI for now in another changeset - PR: https://git.openjdk.org/jdk/pull/9870
Re: RFR: 8295017: Remove Windows specific workaround in JLI_Snprintf [v2]
On Mon, 10 Oct 2022 05:06:00 GMT, David Holmes wrote: >> src/java.base/share/native/libjli/jli_util.h line 91: >> >>> 89: * https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference >>> 90: * /snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >>> 91: */ >> >> I don't think the comment about the *lack* of a workaround is needed, just >> adding clutter. But this isn't code I have much involvement with. Other >> than that, the change looks fine. > > I agree, we don't document the absence of a workaround. Removed the comment as requested - PR: https://git.openjdk.org/jdk/pull/10625
Re: RFR: 8291917: Windows - Improve error messages when the C Runtime Libraries or jvm.dll cannot be loaded [v14]
On Mon, 10 Oct 2022 04:35:12 GMT, David Holmes wrote: >> Julian Waters 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 16 additional >> commits since the last revision: >> >> - Merge branch 'openjdk:master' into patch-4 >> - Merge branch 'openjdk:master' into patch-4 >> - Use - instead of : as a separator >> - Merge branch 'openjdk:master' into patch-4 >> - Make DLL_ERROR4 look a little better without changing what it means >> - Revert changes to JLI_ReportErrorMessageSys >> - Update java_md.c >> - Update java_md.h >> - Merge branch 'openjdk:master' into patch-4 >> - Merge branch 'openjdk:master' into patch-4 >> - ... and 6 more: https://git.openjdk.org/jdk/compare/ad28ef9e...c3113cac > > src/java.base/windows/native/libjli/java_md.h line 48: > >> 46: */ >> 47: >> 48: void reportWithLastWindowsError(const char* message, ...); > > Why does this need to be exported in the header file? Are you expecting other > code to call this? I left that in there since it would possibly be a useful utility to have for other JLI code that might need to work with Windows errors in the future - PR: https://git.openjdk.org/jdk/pull/9749
Re: RFR: 8295017: Remove Windows specific workaround in JLI_Snprintf [v3]
> The C99 snprintf is available with Visual Studio 2015 and above, alongside > Windows 10 and the UCRT, and is no longer identical to the outdated Windows > _snprintf. Since support for the Visual C++ 2017 compiler was removed a while > ago, we can now safely remove the compatibility workaround on Windows and > have JLI_Snprintf simply delegate to snprintf. Julian Waters 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 five additional commits since the last revision: - Comment documenting change isn't required - Merge branch 'openjdk:master' into patch-1 - Comment formatting - Remove Windows specific JLI_Snprintf implementation - Remove Windows JLI_Snprintf definition - Changes: - all: https://git.openjdk.org/jdk/pull/10625/files - new: https://git.openjdk.org/jdk/pull/10625/files/35de1467..9149aae1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10625=02 - incr: https://webrevs.openjdk.org/?repo=jdk=10625=01-02 Stats: 104 lines in 6 files changed: 2 ins; 97 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/10625.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10625/head:pull/10625 PR: https://git.openjdk.org/jdk/pull/10625
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 10 Oct 2022 08:04:28 GMT, Jaikiran Pai wrote:
>> Sean Coffey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Check for 0 security events
>
> src/jdk.jfr/share/classes/jdk/jfr/events/InitialSecurityPropertyEvent.java
> line 33:
>
>> 31: @Category({"Java Development Kit", "Security"})
>> 32: @Label("Initial Security Property")
>> 33: @Name("jdk.InitialSecurityProperty")
>
> Should we name this to `jdk.InitialSecurityProperties` and the label to
> `Initial Security Properties`, to be more accurate?
There is one property per event, so it uses the same naming convention as
jdk.InitialSystemProperty
-
PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v2]
On Sun, 9 Oct 2022 11:52:18 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> This change introduces new system and security properties for specifying >> factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider >> implementations. >> >> These new properties allow more granular control over the set of object >> factories allowed to reconstruct Java objects from LDAP and RMI contexts. >> >> The new factory filters are supplementing the existing >> `jdk.jndi.object.factoriesFilter` global factories filter to determine if a >> specific object factory is permitted to instantiate objects for the given >> protocol. >> >> Links: >> >> - [CSR with details](https://bugs.openjdk.org/browse/JDK-8291556) >> - [JBS issue](https://bugs.openjdk.org/browse/JDK-8290368) >> >> ### List of code changes >> >> - Implementation for two new system and security properties have been added >> to the `com.sun.naming.internal.ObjectFactoriesFilter` class >> - `java.security` and `module-info.java` files have been updated with a >> documentation for the new properties >> - To keep API of `javax.naming.spi.NamingManager` and >> `javax.naming.spi.DirectoryManager` classes unmodified a new internal >> `com.sun.naming.internal.NamingManagerHelper` class has been introduced. All >> `getObjectInstance` calls have been updated to use the new helper class. >> >> NamingManagerHelper changes >> Changes performed to construct the `NamingManagerHelper` class: >> - `DirectoryManager.getObjectInstance` -> >> `NamingManagerHelper.getDirObjectInstance`. Dependant methods were also >> moved to the `NamingManagerHelper` class >> - `NamingManager.getObjectInstance` -> >> `NamingManagerHelper.getObjectInstance`. Methods responsible for >> setting/getting object factory builder were moved to the >> `NamingManagerHelper` class too. >> >> >> ### Test changes >> New tests have been added for checking that new factory filters can be used >> to restrict reconstruction of Java objects from LDAP and RMI contexts: >> - LDAP protocol specific test: >> test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java >> - RMI protocol specific test: >> test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java >> Existing `test/jdk/javax/naming/module/RunBasic.java` test has been updated >> to allow test-specific factories filter used to reconstruct objects from the >> test LDAP server. >> >> ### Testing >> tier1-tier3 and JNDI regression/JCK tests not showing any failures related >> to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov 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 six additional > commits since the last revision: > > - Refactor checkInput, better reporting for invalid filter patterns > - Merge branch 'master' into JDK-8290368_protocol_specific_factory_filters > - Additional comments/formatting cleanup. > - More tests clean-up. Code/doc comments cleanup. > - Cleanup test comments. Add tests to check that LDAP/RMI filters do not > intersect. > - 8290368: Introduce LDAP and RMI protocol-specific object factory filters > to JNDI implementation Changes requested by dfuchs (Reviewer). src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java line 99: > 97: return globalResult == Status.ALLOWED; > 98: } > 99: If I'm not mistaken there's no point in checking the specific filter if the global filter state is REJECTED. So instead of switching on the specificResult below, maybe you should change the logic to switch on the globalResult instead and only call the specific filter if needed? - PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]
On Wed, 18 May 2022 18:14:52 GMT, Severin Gehwolf wrote: >> Please review this change to the cgroup v1 subsystem which makes it more >> resilient on some of the stranger systems. Unfortunately, I wasn't able to >> re-create a similar system as the reporter. The idea of using the longest >> substring match for the cgroupv1 file paths was based on the fact that on >> systemd systems processes run in separate scopes and the maven forked test >> runner might exhibit this property. For that it makes sense to use the >> common ancestor path. Nothing changes in the common cases where the >> `cgroup_path` matches `_root` and when the `_root` is `/` (container the >> former, host system the latter). >> >> In addition, the code paths which are susceptible to throw NPE have been >> hardened to catch those situations. Should it happen in the future it makes >> more sense (to me) to not have accurate container detection in favor of >> continuing to keep running. >> >> Finally, with the added unit-tests a bug was uncovered on the "substring" >> match case of cgroup paths in hotspot. `p` returned from `strstr` can never >> point to `_root` as it's used as the "needle" to find in "haystack" >> `cgroup_path` (not the other way round). >> >> Testing: >> - [x] Added unit tests >> - [x] GHA >> - [x] Container tests on cgroups v1 Linux. Continue to pass > > Severin Gehwolf has updated the pull request incrementally with two > additional commits since the last revision: > > - Refactor hotspot gtest > - Separate into function. Fix comment. test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java line 224: > 222: > "1:name=systemd:/user.slice/user-1000.slice/user@1000.service/apps.slice/apps-org.gnome.Terminal.slice/vte-spawn-3c00b338-5b65-439f-8e97-135e183d135d.scope\n" > + > 223: > "0::/user.slice/user-1000.slice/user@1000.service/apps.slice/apps-org.gnome.Terminal.slice/vte-spawn-3c00b338-5b65-439f-8e97-135e183d135d.scope\n"; > 224: private String cgroupv1SelfPrefixContent = > "9:memory:/user.slice/user-1000.slice/session-3.scope\n"; Let's remove one redundant space Suggestion: private String cgroupv1SelfPrefixContent = "9:memory:/user.slice/user-1000.slice/session-3.scope\n"; - PR: https://git.openjdk.org/jdk/pull/8629
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 10 Oct 2022 08:06:45 GMT, Jaikiran Pai wrote:
>> Sean Coffey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Check for 0 security events
>
> src/jdk.jfr/share/classes/jdk/jfr/events/InitialSecurityPropertyEvent.java
> line 35:
>
>> 33: @Name("jdk.InitialSecurityProperty")
>> 34: @Description("Initial Security Properties")
>> 35: public final class InitialSecurityPropertyEvent extends AbstractJDKEvent
>> {
>
> The event naming guidelines here
> https://docs.oracle.com/en/java/javase/17/jfapi/guidelines-naming-and-labeling-events.html
> recommend leaving out `Event` from the class name. So, maybe we should call
> this `InitialSecurityProperties`?
The documentation is somewhat misleading. If the event has a name annotation, I
think it's fine to call the class Event, because name will override the class
name.
-
PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 3 Oct 2022 10:30:54 GMT, Sean Coffey wrote:
>> New JFR event to record state of initial security properties.
>>
>> Debug output is also now added for these properties via
>> -Djava.security.debug=properties
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Check for 0 security events
test/jdk/java/security/Security/ConfigFileTest.java line 132:
> 130: .shouldNotContain(EXPECTED_DEBUG_OUTPUT)
> 131: .shouldNotContain(UNEXPECTED_DEBUG_OUTPUT);
> 132: } else{
let's add one space after `else`
-
PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 3 Oct 2022 10:30:54 GMT, Sean Coffey wrote:
>> New JFR event to record state of initial security properties.
>>
>> Debug output is also now added for these properties via
>> -Djava.security.debug=properties
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Check for 0 security events
src/jdk.jfr/share/classes/jdk/jfr/events/InitialSecurityPropertyEvent.java line
33:
> 31: @Category({"Java Development Kit", "Security"})
> 32: @Label("Initial Security Property")
> 33: @Name("jdk.InitialSecurityProperty")
Should we name this to `jdk.InitialSecurityProperties` and the label to
`Initial Security Properties`, to be more accurate?
src/jdk.jfr/share/classes/jdk/jfr/events/InitialSecurityPropertyEvent.java line
35:
> 33: @Name("jdk.InitialSecurityProperty")
> 34: @Description("Initial Security Properties")
> 35: public final class InitialSecurityPropertyEvent extends AbstractJDKEvent {
The event naming guidelines here
https://docs.oracle.com/en/java/javase/17/jfapi/guidelines-naming-and-labeling-events.html
recommend leaving out `Event` from the class name. So, maybe we should call
this `InitialSecurityProperties`?
-
PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 3 Oct 2022 10:30:54 GMT, Sean Coffey wrote:
>> New JFR event to record state of initial security properties.
>>
>> Debug output is also now added for these properties via
>> -Djava.security.debug=properties
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Check for 0 security events
src/java.base/share/classes/java/security/ProtectionDomain.java line 76:
> 74: static class JavaSecurityAccessImpl implements JavaSecurityAccess {
> 75: /* cache a copy for recording purposes */
> 76: static Properties initialSecurityProperties;
This doesn't look very clean. Could the Security class hold the initial
security properties and provide an accessor method that
JavaSecurityAccess:getIinitialProperties could use?
-
PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 3 Oct 2022 10:30:54 GMT, Sean Coffey wrote:
>> New JFR event to record state of initial security properties.
>>
>> Debug output is also now added for these properties via
>> -Djava.security.debug=properties
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Check for 0 security events
src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 318:
> 316: InitialSecurityPropertyEvent e = new
> InitialSecurityPropertyEvent();
> 317: e.key = (String) entry.getKey();
> 318: e.value = (String) entry.getValue();
To avoid any (odd/unexpected) `ClassCastException` here, perhaps this loop can
be changed to something like:
for (Set name : p.stringPropertyNames()) {
InitialSecurityPropertyEvent e = new InitialSecurityPropertyEvent();
e.key = name;
e.value = p.getProperty(name);
Since this code anyway wants to deal with string key/values, this wouldn't
introduce any functional change and yet at the same time prevent any
unexpected/theoretical `ClassCastException`s
-
PR: https://git.openjdk.org/jdk/pull/10394
Re: RFR: 8292177: InitialSecurityProperty JFR event [v3]
On Mon, 3 Oct 2022 10:30:54 GMT, Sean Coffey wrote: >> New JFR event to record state of initial security properties. >> >> Debug output is also now added for these properties via >> -Djava.security.debug=properties > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Check for 0 security events src/jdk.jfr/share/classes/jdk/jfr/events/InitialSecurityPropertyEvent.java line 29: > 27: > 28: import jdk.jfr.*; > 29: import jdk.jfr.internal.MirrorEvent; Hello Sean, this `MirrorEvent` appears to be an unused import. Furthermore, as far as I know, in `core-libs` the `*` wildcard imports aren't typically used. I don't know if it's OK to use it here in the `jdk.jfr` module. - PR: https://git.openjdk.org/jdk/pull/10394
