Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Thu, 10 Mar 2022 08:52:17 GMT, Сергей Цыпанов wrote:
>> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with
>> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when
>> called with vararg of size 0, 1, 2.
>>
>> In general replacement of `Arrays.asList()` with `List.of()` is dubious as
>> the latter is null-hostile, however in some cases we are sure that arguments
>> are non-null. Into this PR I've included the following cases (in addition to
>> those where the argument is proved to be non-null at compile-time):
>> - `MethodHandles.longestParameterList()` never returns null
>> - parameter types are never null
>> - interfaces used for proxy construction and returned from
>> `Class.getInterfaces()` are never null
>> - exceptions types of method signature are never null
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8282662: Revert dubious changes in MethodType
src/java.base/share/classes/sun/security/validator/EndEntityChecker.java line
119:
> 117: // TLS key exchange algorithms requiring keyEncipherment key usage
> 118: private static final Collection KU_SERVER_ENCRYPTION =
> 119: Arrays.asList("RSA");
I understand that you haven't modified the `KU_SERVER_KEY_AGREEMENT` on line
122 because it has 4 elements. However, these 2 nearby lines using different
styles look a little strange to me. Why restrict the number to 0, 1, and 2? If
we have defined methods with up to 9 arguments, does that mean the new type is
suitable for 9 elements?
-
PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v5]
On Fri, 29 Apr 2022 15:51:12 GMT, Weijun Wang wrote:
>> test/lib/jdk/test/lib/util/FileUtils.java line 389:
>>
>>> 387: * @throws IOException
>>> 388: */
>>> 389: public static void patch(Path path, int fromLine, int toLine,
>>> String from, String to) throws IOException {
>>
>> Should this method check whether the `fromLine` and `toLine` are valid
>> values? Things like, negative value or 0 or `toLine` being less than
>> `fromLine`. I'm not familiar with the expectations of test library code -
>> maybe those checks aren't necessary since this is a test util?
>
> `lines.remove()` and `lines.subList()` will throw the correct exception.
> Since you asked, we can add it.
Now that we call `subList` at the beginning, I think there's no need to
explicitly perform a check.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v9]
On Mon, 2 May 2022 13:01:40 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > 8285452: Tests updated Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 14:19:40 GMT, Daniel Fuchs wrote: >> The comparison is intentionally made lax so the caller does not need to >> provide the exact indentation or even new line characters. We think along >> with `fromLine` and `toLine` this is enough to make sure we are not >> modifying the wrong lines. > > Shouldn't the comparison be better implemented by the caller then, who will > know whether spaces are important or not? That's why I had suggested taking a > `Predicate` that could be called with each line removed, and the > caller could interrupt the parsing by returning false when they detect a > mismatch with what they expect. We can provide a more sophisticated `Function` replacer if we want to let user to customize all the details. This time we still only want them to be string literals. I agree we can keep the new lines inside, but trimming on each line and the final block is still useful so caller does not need to care about indentation and empty lines at both ends. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v5]
On Fri, 29 Apr 2022 12:02:59 GMT, Jaikiran Pai wrote:
>> Sibabrata Sahoo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> updated to add space character in begining of multiline string
>
> test/lib/jdk/test/lib/util/FileUtils.java line 389:
>
>> 387: * @throws IOException
>> 388: */
>> 389: public static void patch(Path path, int fromLine, int toLine,
>> String from, String to) throws IOException {
>
> Should this method check whether the `fromLine` and `toLine` are valid
> values? Things like, negative value or 0 or `toLine` being less than
> `fromLine`. I'm not familiar with the expectations of test library code -
> maybe those checks aren't necessary since this is a test util?
`lines.remove()` and `lines.subList()` will throw the correct exception. Since
you asked, we can add it.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:44:26 GMT, Daniel Fuchs wrote:
>> test/lib/jdk/test/lib/util/FileUtils.java line 394:
>>
>>> 392: var removed = "";
>>> 393: for (int i = fromLine; i <= toLine; i++) {
>>> 394: removed += lines.remove(fromLine - 1).trim();
>>
>> shouldn't you insert a "\n" ? otherwise concatenating lines "ab" and "c"
>> will be the same as concatenating lines "a" and "bc".
>
> Also calling trim() assumes that white spaces are not significant. This might
> not be the case in the general case (for instance - think of markdown files
> were leading spaces are significant).
The comparison is intentionally made lax so the caller does not need to provide
the exact indentation or even new line characters. We think along with
`fromLine` and `toLine` this is enough to make sure we are not modifying the
wrong lines.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v4]
On Fri, 29 Apr 2022 10:36:50 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > 8285452: Add a new test library API to replace a file content using > FileUtils.java test/lib/jdk/test/lib/util/FileUtils.java line 396: > 394: removed += lines.remove(fromLine - 1).trim(); > 395: } > 396: var froms = > Arrays.asList(from.split(System.lineSeparator())).stream() How about just using `from.lines()`? - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > Update FileUtils.java I've updated the title of the bug. Siba can update the PR title. You're right that it's not easy to discover such APIs. We put it in `FileUtils` hoping people who does not want to write their own method will search from there first. As for the API itself, we've imagined something like FileUtils.patch(inputFile) .with(1, 10, List.of(lines), List.of(newLines)) .with(100, 100, linesAsAString, newLinesAsAString) .writeTo(outputFile) but the current form is the simplest case and could be kept even if we have a verbose one. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > Update FileUtils.java Currently it's just for one test, but it's the exact kind of method that should go into the test library, i.e. a general purpose file manipulation utility that can be useful by everyone. `patch` overwrites the input file (by default) too. We can always enhance it to support `-o`. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > Update FileUtils.java Or we can provide an example in the method spec. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > Update FileUtils.java We have a test that dynamically downloads a source file, patches it, compiles it, and then runs it. We don't want to include a static copy of the file inside the test. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 16:06:22 GMT, Daniel Fuchs wrote:
>> Sibabrata Sahoo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update FileUtils.java
>
> test/lib/jdk/test/lib/util/FileUtils.java line 402:
>
>> 400: if (!removed.equals(froms)) {
>> 401: throw new IOException("Removed not the same");
>> 402: }
>
> That's a bit strange. I would suggest to return the removed lines instead, or
> to pass a `Consumer` (or even better, a `Predicate` ?) that
> will accept the removed lines. You could continue to remove if the predicate
> returns true and throw if it returns false. It would also enable you to tell
> exactly which line failed the check.
I was just thinking about providing the removed lines and the added lines at
the same time into the method (just like what a patch file looks like). The
exception here can probably be enhanced to compare the content of `removed`
with `from`. Two blocks of code (call and callback) would be needed if a
consumer or predicate is used, and I don't feel it's worth doing. Here I've
already trimmed the lines to make sure whitespaces do not matter.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v7]
On Tue, 22 Mar 2022 21:25:28 GMT, Valerie Peng wrote: >> It's been several years since we increased the default key sizes. Before >> shifting to PQC, NSA replaced its Suite B cryptography recommendations with >> the Commercial National Security Algorithm Suite which suggests: >> >> - SHA-384 for secure hashing >> - AES-256 for symmetric encryption >> - RSA with 3072 bit keys for digital signatures and for key exchange >> - Diffie Hellman (DH) with 3072 bit keys for key exchange >> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures >> (ECDSA) >> >> So, this proposed changes made the suggested key size and algorithm changes. >> The changes are mostly in keytool, jarsigner and their regression tests, so >> @wangweij Could you please take a look? >> >> Thanks! > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Minor code refactoring Everything looks fine to me. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8257733: Move module-specific data from make to respective module [v9]
On Thu, 17 Mar 2022 00:12:38 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typos The security-related change looks fine to me - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v5]
On Tue, 15 Mar 2022 20:44:20 GMT, Valerie Peng wrote:
>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java
>> line 122:
>>
>>> 120: default -> {
>>> 121: throw new ProviderException
>>> 122: ("Unrecognized algorithm for checking key
>>> size");
>>
>> If it's an unknown key algorithm, is it possible we just ignore it and keep
>> using `minKeyLen` and `maxKeyLen`?
>
> Well, instead of ignore unknown key algorithm, perhaps safer to throw
> Exception so it can be caught and handled during develop time.
> P11KeyPairGenerator class is only used for known algorithms which it is
> registered for, so probably ok to go either way. I'd prefer to play it safe
> and force a review of this block of code when new algorithm is added.
OK.
-
PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v5]
On Mon, 14 Mar 2022 20:08:31 GMT, Valerie Peng wrote:
>> It's been several years since we increased the default key sizes. Before
>> shifting to PQC, NSA replaced its Suite B cryptography recommendations with
>> the Commercial National Security Algorithm Suite which suggests:
>>
>> - SHA-384 for secure hashing
>> - AES-256 for symmetric encryption
>> - RSA with 3072 bit keys for digital signatures and for key exchange
>> - Diffie Hellman (DH) with 3072 bit keys for key exchange
>> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures
>> (ECDSA)
>>
>> So, this proposed changes made the suggested key size and algorithm changes.
>> The changes are mostly in keytool, jarsigner and their regression tests, so
>> @wangweij Could you please take a look?
>>
>> Thanks!
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update again and undo DSA changes
Some small comments. Otherwise looks fine.
src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java
line 121:
> 119: v = max;
> 120: }
> 121: } catch (NullPointerException | NoSuchAlgorithmException ne)
> {
There is no need to mention NPE.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java
line 101:
> 99: // set default key sizes and apply our own algorithm-specific
> limits
> 100: // override lower limit to disallow unsecure keys being generated
> 101: // override upper limit to deter DOS attack
No a P11 expert, but I assume `algorithm` here is already guaranteed to be in
uppercase?
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java
line 122:
> 120: default -> {
> 121: throw new ProviderException
> 122: ("Unrecognized algorithm for checking key size");
If it's an unknown key algorithm, is it possible we just ignore it and keep
using `minKeyLen` and `maxKeyLen`?
-
PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8280400: JDK 19 L10n resource files update - msgdrop 10 [v2]
On Thu, 10 Mar 2022 17:55:44 GMT, Alisen Chung wrote: >> msg drop for jdk19, Mar 9, 2022 > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > moved CurrencyNames changes to jdk.localedata The security related changes look fine, except for the year 2021. - PR: https://git.openjdk.java.net/jdk/pull/7765
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v3]
On Wed, 9 Mar 2022 19:15:36 GMT, Valerie Peng wrote: >> It's been several years since we increased the default key sizes. Before >> shifting to PQC, NSA replaced its Suite B cryptography recommendations with >> the Commercial National Security Algorithm Suite which suggests: >> >> - SHA-384 for secure hashing >> - AES-256 for symmetric encryption >> - RSA with 3072 bit keys for digital signatures and for key exchange >> - Diffie Hellman (DH) with 3072 bit keys for key exchange >> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures >> (ECDSA) >> >> So, this proposed changes made the suggested key size and algorithm changes. >> The changes are mostly in keytool, jarsigner and their regression tests, so >> @wangweij Could you please take a look? >> >> Thanks! > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Update JarSigner javadoc to make it consistent with previous update Sorry if my previous comment confused you, the code and javadoc are not consistent now. src/java.base/share/classes/sun/security/util/SignatureUtil.java line 561: > 559: return (isDSA || bitLength >= 624 ? "SHA384" : "SHA256"); > 560: } > 561: } In this method, "SHA-384" for 7680-bit key (7680 > 7680 is false). src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java line 439: > 437: * Specifically, if a DSA or RSA key with a key size no less > than 7680 > 438: * bits, or an EC key with a key size no less than 512 bits, > 439: * SHA-512 will be used as the hash function for the signature. In this javadoc, SHA-512 for 7680-bit key (7680 is no less than 7680). - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v2]
On Wed, 9 Mar 2022 02:02:51 GMT, Valerie Peng wrote: >> It's been several years since we increased the default key sizes. Before >> shifting to PQC, NSA replaced its Suite B cryptography recommendations with >> the Commercial National Security Algorithm Suite which suggests: >> >> - SHA-384 for secure hashing >> - AES-256 for symmetric encryption >> - RSA with 3072 bit keys for digital signatures and for key exchange >> - Diffie Hellman (DH) with 3072 bit keys for key exchange >> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures >> (ECDSA) >> >> So, this proposed changes made the suggested key size and algorithm changes. >> The changes are mostly in keytool, jarsigner and their regression tests, so >> @wangweij Could you please take a look? >> >> Thanks! > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Updated to use SHA-384 as long as the keysize permits. The `JarSigner` API is not updated. - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8281525: Enable Zc:strictStrings flag in Visual Studio build [v2]
On Tue, 22 Feb 2022 16:43:28 GMT, Daniel Jeliński wrote: >> Please review this PR that enables >> [Zc:strictStrings](https://docs.microsoft.com/en-us/cpp/build/reference/zc-strictstrings-disable-string-literal-type-conversion?view=msvc-170) >> compiler flag, which makes assigning a string literal to a non-const >> pointer a compile-time error. >> >> This type of assignment is [disallowed by C++ standard since >> C++11](https://en.cppreference.com/w/cpp/language/string_literal). Writing >> to a string literal through a non-const pointer [produces a run-time >> error](https://docs.microsoft.com/en-us/cpp/cpp/string-and-character-literals-cpp?view=msvc-170#microsoft-specific-1). >> >> The included code changes are trivial; I added `const` keyword to variable >> and parameter declarations where needed, and added explicit casts to >> non-const pointers where adding `const` was not feasible. >> >> I verified that the build passes both with and without `--enable-debug`, >> both with VS2017 and VS2019. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Move strictStrings to toolchain_cflags I'm the original author of `sspi.cpp`. All changes look fine to me. Thanks a lot for the enhancement. - PR: https://git.openjdk.java.net/jdk/pull/7565
RFR: 8281175: Add a -providerPath option to jarsigner
Add the `-providerPath` option to jarsigner to be consistent with keytool. - Commit messages: - 8281175: Add a -providerPath option to jarsigner Changes: https://git.openjdk.java.net/jdk/pull/7338/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7338=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281175 Stats: 39 lines in 3 files changed: 24 ins; 5 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/7338.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7338/head:pull/7338 PR: https://git.openjdk.java.net/jdk/pull/7338
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]
On Wed, 26 Jan 2022 16:25:24 GMT, Daniel Fuchs wrote: >> Michael McMahon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> removed ^M from test > > test/jdk/sun/security/krb5/auto/HttpsCB.java line 120: > >> 118: >> 119: boolean expected1 = Boolean.parseBoolean(args[0]); >> 120: boolean expected2 = Boolean.parseBoolean(args[1]); > > It might be better for future maintainers and readability if these two > variables could have better names, and possibly a comment to explain their > purpose. AFAIU it's the expected result of running with/without CBT - where > `true` means that the operation should succeed and `false` that it's expected > to fail with some exception... Maybe `expectedCbtUrlResult` and `expectedNormalUrlResult`. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]
On Wed, 26 Jan 2022 16:27:29 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> removed ^M from test
>
> test/jdk/sun/security/krb5/auto/HttpsCB.java line 201:
>
>> 199: return reader.readLine().equals(CONTENT);
>> 200: } catch (Exception e) {
>> 201: return false;
>
> Should we log that we have received the excepted exception here? Shouldn't
> the catch clause only list the exceptions that we are expecting?
It's `java.net.SocketException: Unexpected end of file from server`. Does not
include any CBT words so don't know if it's worth parsing.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v7]
On Mon, 24 Jan 2022 22:11:51 GMT, Michael McMahon wrote:
>> Hi,
>>
>> This change adds Channel Binding Token (CBT) support to HTTPS
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO,
>> Kerberos) authentication scheme. When enabled, the implementation
>> preemptively includes a CBT with authentication requests over Kerberos. The
>> feature is enabled as follows:
>>
>> A system property "jdk.spnego.cbt" is defined which can have the values
>> "never" (default), which means the feature is disabled, "always", which
>> means the CBT is included for all https Negotiate authentications, or it can
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of
>> domains/hosts where the feature is enabled, and disabled everywhere else. In
>> the given example, the CBT would be included in authentication requests for
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its
>> sub-domains.
>>
>> A test will be added separately to the implementation.
>>
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>>
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> more updates
Looks good to me. Only several wording and style suggestions.
I know you are asking SQE to create a security infra test, but I'll see if I
can contribute a regression test. Don't wait for me.
src/java.base/share/classes/java/net/doc-files/net-properties.html line 223:
> 221:
> 222: "never". This is also the default value if the property
> is not set. In this case,
> 223: CBT's are never sent.
Typo, "CBTs"?
src/java.base/share/classes/java/net/doc-files/net-properties.html line 224:
> 222: "never". This is also the default value if the property
> is not set. In this case,
> 223: CBT's are never sent.
> 224: "always". CBTs are sent for all Kerberos authentication
> attempts over HTTPS.
Shall we remove "Kerberos"? Or we can use "Kerberos or Negotiate".
src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
line 1:
> 1: /**
This is not a doc comment.
src/java.security.jgss/share/classes/sun/net/www/protocol/http/spnego/NegotiatorImpl.java
line 124:
> 122: try {
> 123: init(hci);
> 124: } catch (GSSException | ChannelBindingException e) {
Two spaces before "e".
-
Marked as reviewed by weijun (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]
On Mon, 24 Jan 2022 15:54:01 GMT, Michael McMahon wrote: >> src/java.base/share/classes/sun/security/util/TlsChannelBinding.java line >> 100: >> >>> (failed to retrieve contents of file, check the PR for context) >> I think this method should stay here. Suppose one day the CBT type is >> configurable for HTTPS we'll have to get it back. Of course we will need to >> update the message to avoid talking about LDAP. > > So, where should the two constant Strings go? It doesn't feel like they > belong in java.base since they are JNDI/SASL related, and we can't have a > method in `java.base` depending on code in other modules? The 2 strings should be on the LDAP side. This method does not really depend on the strings except for mentioning one in the exception message. We can just rewrite it into `"Illegal channel binding type: " + cbType`. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]
On Mon, 24 Jan 2022 13:54:12 GMT, Daniel Fuchs wrote:
>> Michael McMahon 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 eight additional
>> commits since the last revision:
>>
>> - fixed failing test issue and update for latest comments
>> - Merge branch 'master' into spnego
>> - added root cause to NamingException
>> - more tidy-up
>> - removed sasl module dependency and added SaslException cause
>> - changes after first review round
>> - cleanup but still no test. Will be added in closed repo
>> - First version of fix. No test and feature enabled always.
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 260:
>
>> 258: * @throws ChannelBindingException
>> 259: */
>> 260: private static TlsChannelBindingType parseType(String cbType)
>> throws ChannelBindingException {
>
> Maybe this method could throw NamingException directly now? That would avoid
> wrapping CBE into NamingException?
My opinion is this method should be put back.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]
On Mon, 24 Jan 2022 13:36:47 GMT, Michael McMahon wrote: >> Hi, >> >> This change adds Channel Binding Token (CBT) support to HTTPS >> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, >> Kerberos) authentication scheme. When enabled, the implementation >> preemptively includes a CBT with authentication requests over Kerberos. The >> feature is enabled as follows: >> >> A system property "jdk.spnego.cbt" is defined which can have the values >> "never" (default), which means the feature is disabled, "always", which >> means the CBT is included for all https Negotiate authentications, or it can >> take the form "domain:a,b.c,*.d.com" which is a comma separated list of >> domains/hosts where the feature is enabled, and disabled everywhere else. In >> the given example, the CBT would be included in authentication requests for >> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its >> sub-domains. >> >> A test will be added separately to the implementation. >> >> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842 >> >> Thanks, >> Michael > > Michael McMahon 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 eight additional > commits since the last revision: > > - fixed failing test issue and update for latest comments > - Merge branch 'master' into spnego > - added root cause to NamingException > - more tidy-up > - removed sasl module dependency and added SaslException cause > - changes after first review round > - cleanup but still no test. Will be added in closed repo > - First version of fix. No test and feature enabled always. src/java.base/share/classes/sun/security/util/TlsChannelBinding.java line 100: > (failed to retrieve contents of file, check the PR for context) I think this method should stay here. Suppose one day the CBT type is configurable for HTTPS we'll have to get it back. Of course we will need to update the message to avoid talking about LDAP. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v4]
On Fri, 21 Jan 2022 15:40:16 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> more tidy-up
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 144:
>
>> 142: } catch (ChannelBindingException e) {
>> 143: throw new NamingException(e.getMessage());
>> 144: }
>
> Should we call initCause here and above? I see that we do call initCause in
> NegotiatorImpl.java.
> On the one hand it's better for diagnostic. On the other hand it exposes a
> module-internal exception class which is not great. Or maybe we should set
> the cause of the CBE as the cause of NamingException.
As long as the spec has not dictated what the cause should be, I don't care if
the exception type is internal or not.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]
On Fri, 21 Jan 2022 16:02:29 GMT, Michael McMahon wrote: >> Hi, >> >> This change adds Channel Binding Token (CBT) support to HTTPS >> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, >> Kerberos) authentication scheme. When enabled, the implementation >> preemptively includes a CBT with authentication requests over Kerberos. The >> feature is enabled as follows: >> >> A system property "jdk.spnego.cbt" is defined which can have the values >> "never" (default), which means the feature is disabled, "always", which >> means the CBT is included for all https Negotiate authentications, or it can >> take the form "domain:a,b.c,*.d.com" which is a comma separated list of >> domains/hosts where the feature is enabled, and disabled everywhere else. In >> the given example, the CBT would be included in authentication requests for >> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its >> sub-domains. >> >> A test will be added separately to the implementation. >> >> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842 >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > added root cause to NamingException Please move // TLS channel binding type property public static final String CHANNEL_BINDING_TYPE = "com.sun.jndi.ldap.tls.cbtype"; // internal TLS channel binding property public static final String CHANNEL_BINDING = "jdk.internal.sasl.tlschannelbinding"; outside of the `TlsChannelBinding` class. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]
On Fri, 21 Jan 2022 16:02:29 GMT, Michael McMahon wrote:
>> Hi,
>>
>> This change adds Channel Binding Token (CBT) support to HTTPS
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO,
>> Kerberos) authentication scheme. When enabled, the implementation
>> preemptively includes a CBT with authentication requests over Kerberos. The
>> feature is enabled as follows:
>>
>> A system property "jdk.spnego.cbt" is defined which can have the values
>> "never" (default), which means the feature is disabled, "always", which
>> means the CBT is included for all https Negotiate authentications, or it can
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of
>> domains/hosts where the feature is enabled, and disabled everywhere else. In
>> the given example, the CBT would be included in authentication requests for
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its
>> sub-domains.
>>
>> A test will be added separately to the implementation.
>>
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>>
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> added root cause to NamingException
src/java.base/share/classes/java/net/doc-files/net-properties.html line 220:
> 218: This controls the generation and sending of TLS channel binding tokens
> (CBT) when Kerberos
> 219: or the Negotiate authentication scheme using Kerberos are
> employed over HTTPS with
> 220: {@code HttpsURLConnection}. There are three possible
> settings:
You can probably mention here that the 'tls-server-end-point' Channel Binding
Type defined in RFC 5929 is used.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v2]
On Wed, 19 Jan 2022 22:20:47 GMT, Michael McMahon wrote:
>> Hi,
>>
>> This change adds Channel Binding Token (CBT) support to HTTPS
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO,
>> Kerberos) authentication scheme. When enabled, the implementation
>> preemptively includes a CBT with authentication requests over Kerberos. The
>> feature is enabled as follows:
>>
>> A system property "jdk.spnego.cbt" is defined which can have the values
>> "never" (default), which means the feature is disabled, "always", which
>> means the CBT is included for all https Negotiate authentications, or it can
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of
>> domains/hosts where the feature is enabled, and disabled everywhere else. In
>> the given example, the CBT would be included in authentication requests for
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its
>> sub-domains.
>>
>> A test will be added separately to the implementation.
>>
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>>
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> changes after first review round
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 133:
> 131:
> (String)env.get(TlsChannelBinding.CHANNEL_BINDING_TYPE));
> 132: } catch (ChannelBindingException e) {
> 133: throw new SaslException(e.getMessage());
How about setting `e` as cause of new exception? In `TlsChannelBinding.java`
the when the original exception was thrown (the 2nd throws) there was a cause.
src/java.security.jgss/share/classes/module-info.java line 36:
> 34: module java.security.jgss {
> 35: requires java.naming;
> 36: requires java.security.sasl;
Can this be removed now?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Fri, 14 Jan 2022 10:18:50 GMT, Daniel Fuchs wrote:
>> This is what was intended (equivalent)
>>
>> `if (s ==null || (s!="always" && s!="never" && !s.startsWith("domain")))`
>
> Argh - you're right I missed the fact that the 3 expressions where included
> in parenthesis. I read it as
>
> ! (s.equals("always")) || ...
Shall we log a message if the value is not one of the 3 forms?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Fri, 14 Jan 2022 18:40:41 GMT, Michael McMahon wrote: >> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 152: >> >>> 150: * If enabled (for a particular destination) then SPNEGO >>> authentication requests will include >>> 151: * a channel binding token for the destination server. The default >>> behavior and setting for the >>> 152: * property is "never" >> >> Maybe this description should be added to >> `src/java.base//share/classes/java/net/doc-files/net-properties.html` too? > > It's actually a purely system property rather than a Net property at the > moment (same as the other spnego ones). Maybe, I should convert them all to > net properties, so they can be documented/set in that file? This system property should only be used for TLS, and the CBT can be used in both the SPNEGO mechanism and the Kerberos 5 mechanism. Therefore I suggest the name should probably contain "tls" (or maybe "https") and "negotiate". BTW, will you reuse this system property if we decide to support CBT in NTLM as well? - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Fri, 14 Jan 2022 18:42:08 GMT, Michael McMahon wrote:
>> src/java.security.jgss/share/classes/module-info.java line 36:
>>
>>> 34: module java.security.jgss {
>>> 35: requires java.naming;
>>> 36: requires java.security.sasl;
>>
>> Someone from security-dev should probably review this and validate that this
>> is OK. I'm also a bit uncomfortable that we require a class from
>> `com.sun.jndi.ldap.sasl` even though `java.naming` is already required by
>> `java.security.jgss` - so maybe this is OK.
>
> Yes. I would like the security team to validate this.
I suggest moving the `TlsChannelBinding` class into
`java.base/sun.security.util` since it's not only used by LDAP anymore. You
might need to modify the types of exceptions thrown in the class and move the 2
final strings to some other class inside `java.security.sasl`.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs [v2]
On Thu, 13 Jan 2022 21:57:57 GMT, Sean Mullan wrote: >> If a JAR is signed with multiple digest algorithms and one of the digest >> algorithms is disabled, `ManifestEntryVerifier.verify()` was incorrectly >> returning null indicating that the jar entry has no signers. >> >> This fixes the issue such that an entry is considered signed if at least one >> of the digest algorithms is not disabled and the digest match passes. This >> makes the fix consistent with how multiple digest algorithms are handled in >> the Signature File. This also fixes an issue in the >> `ManifestEntryVerifier.getParams()` method in which it was incorrectly >> checking the algorithm constraints against all signers of a JAR when it >> should check them only against the signers of the entry that is being >> verified. >> >> An additional cache has also been added to avoid checking if the digest >> algorithm is disabled more than once for entries signed by the same set of >> signers. > > Sean Mullan has updated the pull request incrementally with one additional > commit since the last revision: > > Change permittedAlgs variable name. > Move put methods out of checkConstraints(). No more comment. Approved. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs [v2]
On Thu, 13 Jan 2022 19:54:44 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java
>> line 211:
>>
>>> 209: }
>>> 210:
>>> 211: CodeSigner[] entrySigners = sigFileSigners.get(name);
>>
>> What if we return here if `entrySigners == null`? It seems the hash
>> comparison will be skipped, but at the end there is no difference in
>> `verifiedSigners`.
>
> The algorithm constraints check will be skipped (because `permittedAlgs` will
> be `null`) but the hash check will not be skipped.
>
> I don't think `null` would be returned in a normal case. The only case I can
> think of is if there was an entry in the Manifest, but no corresponding entry
> in the SF file. I suppose we could still do a constraints check as if there
> were no signers. However, it doesn't seem that useful since technically the
> entry is not protected by the signature and the hash check is still done,
> unless I am missing something.
Or maybe the key/signature algorithm is weak and ignored. I was only thinking
it's not worth calculating the hash anymore. Of course there will be a behavior
change. If there's a hash mismatch, it used to be a security exception, but if
ignored, it's just a plain unsigned entry.
>> src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java
>> line 230:
>>
>>> 228: params = new
>>> JarConstraintsParameters(entrySigners);
>>> 229: }
>>> 230: if (!checkConstraints(digestAlg, permittedAlgs,
>>> params)) {
>>
>> Can we move the `permittedAlgs::put` call from inside the `checkConstraints`
>> method to here? You can even call `computeIfAbsent` to make the intention
>> clearer.
>
> Yes, I can do that. However, I'm a bit wary of using lambdas in this code
> which may get exercised early in app startup. WDYT?
Maybe, I don't know how problematic if lambda is used this early.
Anyway, I still prefer moving the update of `permittedAlgs` here. Updating it
inside the method seems too faraway.
-
PR: https://git.openjdk.java.net/jdk/pull/7056
Re: RFR: 8278851: Correct signer logic for jars signed with multiple digestalgs
On Wed, 12 Jan 2022 21:57:22 GMT, Sean Mullan wrote:
> If a JAR is signed with multiple digest algorithms and one of the digest
> algorithms is disabled, `ManifestEntryVerifier.verify()` was incorrectly
> returning null indicating that the jar entry has no signers.
>
> This fixes the issue such that an entry is considered signed if at least one
> of the digest algorithms is not disabled and the digest match passes. This
> makes the fix consistent with how multiple digest algorithms are handled in
> the Signature File. This also fixes an issue in the
> `ManifestEntryVerifier.getParams()` method in which it was incorrectly
> checking the algorithm constraints against all signers of a JAR when it
> should check them only against the signers of the entry that is being
> verified.
>
> An additional cache has also been added to avoid checking if the digest
> algorithm is disabled more than once for entries signed by the same set of
> signers.
src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line
211:
> 209: }
> 210:
> 211: CodeSigner[] entrySigners = sigFileSigners.get(name);
What if we return here if `entrySigners == null`? It seems the hash comparison
will be skipped, but at the end there is no difference in `verifiedSigners`.
src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line
230:
> 228: params = new
> JarConstraintsParameters(entrySigners);
> 229: }
> 230: if (!checkConstraints(digestAlg, permittedAlgs,
> params)) {
Can we move the `permittedAlgs::put` call from inside the `checkConstraints`
method to here? You can even call `computeIfAbsent` to make the intention
clearer.
src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line
272:
> 270: }
> 271:
> 272: // Gets the permitted algs for the signers of this entry.
This can probably be another `computeIfAbsent`.
-
PR: https://git.openjdk.java.net/jdk/pull/7056
RFR: 8255266: 2021-11-27 public suffix list update v 3c213aa
Update Public Suffix List data to the latest version at https://github.com/publicsuffix/list. - Commit messages: - 8255266: 2021-11-27 public suffix list update v 3c213aa Changes: https://git.openjdk.java.net/jdk/pull/6643/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6643=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255266 Stats: 1254 lines in 5 files changed: 887 ins; 215 del; 152 mod Patch: https://git.openjdk.java.net/jdk/pull/6643.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6643/head:pull/6643 PR: https://git.openjdk.java.net/jdk/pull/6643
Integrated: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > This change was previously announced on the [jdk-dev > alias](https://mail.openjdk.java.net/pipermail/jdk-dev/2021-May/005616.html) > and is documented in [JEP 411](https://openjdk.java.net/jeps/411#Description). > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. This pull request has now been integrated. Changeset: d589b664 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/d589b664cc809aea39ec094e99b1898df1bf3c19 Stats: 30 lines in 6 files changed: 5 ins; 8 del; 17 mod 8270380: Change the default value of the java.security.manager system property to disallow Reviewed-by: lancea, mullan, rriggs - PR: https://git.openjdk.java.net/jdk/pull/5204
Integrated: 8275512: Upgrade required version of jtreg to 6.1
On Tue, 19 Oct 2021 13:51:45 GMT, Weijun Wang wrote: > As a follow up of JEP 411, we will soon disallow security manager by default. > jtreg 6.1 does not set its own security manager if JDK version is >= 18. This pull request has now been integrated. Changeset: c24fb852 Author: Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/c24fb852f20bf0fc2817dfed52ff1609a5bced59 Stats: 8 lines in 7 files changed: 0 ins; 0 del; 8 mod 8275512: Upgrade required version of jtreg to 6.1 Reviewed-by: ihse, iignatyev, joehw, lancea, jjg, mchung - PR: https://git.openjdk.java.net/jdk/pull/6012
Re: RFR: 8275512: Upgrade required version of jtreg to 6.1 [v2]
> As a follow up of JEP 411, we will soon disallow security manager by default. > jtreg 6.1 does not set its own security manager if JDK version is >= 18. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: upgrade the version in GHA config only in patch2: unchanged: - Changes: - all: https://git.openjdk.java.net/jdk/pull/6012/files - new: https://git.openjdk.java.net/jdk/pull/6012/files/b86e799c..f5ffc49b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6012=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6012=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6012.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6012/head:pull/6012 PR: https://git.openjdk.java.net/jdk/pull/6012
RFR: 8275512: Upgrade required version of jtreg to 6.1
As a follow up of JEP 411, we will soon disallow security manager by default. jtreg 6.1 does not set its own security manager if JDK version is >= 18. - Commit messages: - 8275512: Upgrade required version of jtreg to 6.1 Changes: https://git.openjdk.java.net/jdk/pull/6012/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6012=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275512 Stats: 7 lines in 6 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/6012.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6012/head:pull/6012 PR: https://git.openjdk.java.net/jdk/pull/6012
Re: RFR: 8274949: Use String.contains() instead of String.indexOf() in java.base
On Fri, 17 Sep 2021 08:56:47 GMT, Andrey Turbanov wrote: > String.contains was introduced in Java 5. > Some code in java.base still uses old approach with `String.indexOf` to check > if String contains specified substring. > I propose to migrate such usages. Makes code shorter and easier to read. security-related change looks fine. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5559
Re: RFR: 8274333: Redundant null comparison after Pattern.split
On Sun, 26 Sep 2021 15:10:52 GMT, Andrey Turbanov wrote: > In couple of classes, result part of arrays of Pattern.split is compared with > `null`. Pattern.split (and hence String.split) never returns `null` in array > elements. Such comparisons are redundant. Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5708
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Tue, 31 Aug 2021 02:05:06 GMT, Weijun Wang wrote: >> This change modifies the default value of the `java.security.manager` system >> property from "allow" to "disallow". This means unless it's explicitly set >> to "allow", any call to `System.setSecurityManager()` would throw an UOE. >> >> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are >> updated to confirm this behavior change. Two other tests are updated because >> they were added after JDK-8267184 and do not have >> `-Djava.security.manager=allow` on its `@run` line even it they need to >> install a `SecurityManager` at runtime. >> >> Please note that this code change requires jtreg to be upgraded to 6.1, >> where a security manager [will not be >> set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). > > New commit pushed. Sections added. > @wangweij This pull request has been inactive for more than 4 weeks and will > be automatically closed if another 4 weeks passes without any activity. To > avoid this, simply add a new comment to the pull request. Feel free to ask > for assistance if you need help with progressing this pull request towards > integration! Still waiting for jtreg upgraded to 6.1. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8274079: Cleanup unnecessary calls to Throwable.initCause() in java.base module
On Thu, 16 Sep 2021 19:03:26 GMT, Andrey Turbanov wrote: > Pass "cause" exception as constructor parameter is shorter and easier to read. Looks fine. Thanks. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5551
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. > > Please note that this code change requires jtreg to be upgraded to 6.1, where > a security manager [will not be > set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). New commit pushed. Sections added. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow [v2]
> This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. > > Please note that this code change requires jtreg to be upgraded to 6.1, where > a security manager [will not be > set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: sections etc - Changes: - all: https://git.openjdk.java.net/jdk/pull/5204/files - new: https://git.openjdk.java.net/jdk/pull/5204/files/63b1b7c9..08635b91 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5204=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5204=00-01 Stats: 7 lines in 2 files changed: 2 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5204.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5204/head:pull/5204 PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Mon, 23 Aug 2021 03:22:18 GMT, Jaikiran Pai wrote: > Would this then allow the security manager to be used? In other words, can > the value of `java.security.manager` be changed dynamically at runtime or is > it restricted to be set only at launch time (via command line arugment > `-Djava.security.manager`)? Whether to allow a SecurityManager to be installed later is determined at system initialization time, so there is no use to set it to "allow" inside a program. See https://github.com/openjdk/jdk/blob/3a690240336bda8582a15ca52f4dcb78be323dcd/src/java.base/share/classes/java/lang/System.java#L2191 The spec in `SecurityManager.java` uses the words "if the Java virtual machine **is started with** the java.security.manager system property..." to describe this. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. > > Please note that this code change requires jtreg to be upgraded to 6.1, where > a security manager [will not be > set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). This issue (https://bugs.openjdk.java.net/browse/JDK-8270380) is blocked by jtreg 6.1 (https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). Precisely, it should be blocked by a JDK bug that updates the jtreg required version to 6.1. I should have mentioned this in the PR. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8272805: Avoid looking up standard charsets
On Sun, 22 Aug 2021 02:53:44 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> This change includes:
> * demo/utils
> * jdk.xx packages
> * Some places were missed in the previous changes. I have found it by
> tracing the calls to the Charset.forName() by executing tier1,2,3 and desktop
> tests.
>
> Some performance discussion: https://github.com/openjdk/jdk/pull/5063
>
> Code excluded in this fix: the Xerces library(should be fixed upstream),
> J2DBench(should be compatible to 1.4), some code in the network(the change
> there are not straightforward, will do it later).
>
> Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
The security related change looks fine to me.
-
Marked as reviewed by weijun (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5210
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Sat, 21 Aug 2021 16:59:39 GMT, Alan Bateman wrote: >> This is the same as the existing words for "disallow". > > I think I agree with Lance that "Throws UOE" would be clearer in this table. Suggestion accepted. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 23:01:27 GMT, Lance Andersen wrote:
>> This change modifies the default value of the `java.security.manager` system
>> property from "allow" to "disallow". This means unless it's explicitly set
>> to "allow", any call to `System.setSecurityManager()` would throw an UOE.
>>
>> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are
>> updated to confirm this behavior change. Two other tests are updated because
>> they were added after JDK-8267184 and do not have
>> `-Djava.security.manager=allow` on its `@run` line even it they need to
>> install one at runtime.
>
> src/java.base/share/classes/java/lang/SecurityManager.java line 128:
>
>> 126: * null
>> 127: * None
>> 128: * Always throws {@code UnsupportedOperationException}
>
> Not sure "Always" is needed, could just be "Throws UOE"
This is the same as the existing words for "disallow".
-
PR: https://git.openjdk.java.net/jdk/pull/5204
RFR: 8270380: Change the default value of the java.security.manager system property to disallow
This change modifies the default value of the `java.security.manager` system property from "allow" to "disallow". This means unless it's explicitly set to "allow", any call to `System.setSecurityManager()` would throw an UOE. The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are updated to confirm this behavior change. Two other tests are updated because they were added after JDK-8267184 and do not have `-Djava.security.manager=allow` on its `@run` line even it they need to install one at runtime. - Commit messages: - 8270380: Change the default value of the java.security.manager system property to disallow Changes: https://git.openjdk.java.net/jdk/pull/5204/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5204=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270380 Stats: 24 lines in 6 files changed: 3 ins; 8 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/5204.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5204/head:pull/5204 PR: https://git.openjdk.java.net/jdk/pull/5204
Integrated: 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result
On Tue, 3 Aug 2021 19:05:55 GMT, Weijun Wang wrote: > `oddPart` contains a lot of info on the `modInverse` output, sometimes it's > even the same. Clearing it in case the result is sensitive. > > No new regression test since it's difficult to access a temporary local > variable in an internal class. Existing tier1-2 tests passed. This pull request has now been integrated. Changeset: a8408708 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/a8408708b065a877278acc6b007ad6a9baaf2561 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result Reviewed-by: bpb, darcy, valeriep - PR: https://git.openjdk.java.net/jdk/pull/4973
RFR: 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result
oddPart contains a lot of info on the modInverse output, sometimes it's even the same. Clearing it in case the result is sensitive. No new regression test since it's difficult to access a temporary local variable in an internal class. Existing tier1-2 tests passed. - Commit messages: - 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result Changes: https://git.openjdk.java.net/jdk/pull/4973/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4973=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271616 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4973.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4973/head:pull/4973 PR: https://git.openjdk.java.net/jdk/pull/4973
Re: RFR: 8266054: VectorAPI rotate operation optimization [v13]
On Tue, 20 Jul 2021 09:57:07 GMT, Jatin Bhateja wrote: >> Current VectorAPI Java side implementation expresses rotateLeft and >> rotateRight operation using following operations:- >> >> vec1 = lanewise(VectorOperators.LSHL, n) >> vec2 = lanewise(VectorOperators.LSHR, n) >> res = lanewise(VectorOperations.OR, vec1 , vec2) >> >> This patch moves above handling from Java side to C2 compiler which >> facilitates dismantling the rotate operation if target ISA does not support >> a direct rotate instruction. >> >> AVX512 added vector rotate instructions vpro[rl][v][dq] which operate over >> long and integer type vectors. For other cases (i.e. sub-word type vectors >> or for targets which do not support direct rotate operations ) instruction >> sequence comprising of vector SHIFT (LEFT/RIGHT) and vector OR is emitted. >> >> Please find below the performance data for included JMH benchmark. >> Machine: Cascade Lake Server (Intel(R) Xeon(R) Platinum 8280 CPU @ 2.70GHz) >> >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40;> >> >> >> >> >> >> > href="file:///C:/Users/jatinbha/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> >> > href="file:///C:/Users/jatinbha/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> >> >> >> >> >> >> >> >> >> >> Benchmark | (bits) | (shift) | (size) | Baseline Score (ops/ms) | With Opts >> (ops/ms) | Gain >> -- | -- | -- | -- | -- | -- | -- >> RotateBenchmark.testRotateLeftB | 128 | 7 | 256 | 3939.136 | 3836.133 | >> 0.973851372 >> RotateBenchmark.testRotateLeftB | 128 | 7 | 512 | 1984.231 | 1918.27 | >> 0.966757399 >> RotateBenchmark.testRotateLeftB | 128 | 15 | 256 | 3925.165 | 4043.842 | >> 1.030234907 >> RotateBenchmark.testRotateLeftB | 128 | 15 | 512 | 1962.723 | 1936.551 | >> 0.986665464 >> RotateBenchmark.testRotateLeftB | 128 | 31 | 256 | 3945.6 | 3817.883 | >> 0.967630525 >> RotateBenchmark.testRotateLeftB | 128 | 31 | 512 | 1944.458 | 1914.229 | >> 0.984453766 >> RotateBenchmark.testRotateLeftB | 256 | 7 | 256 | 4612.149 | 4514.874 | >> 0.978908964 >> RotateBenchmark.testRotateLeftB | 256 | 7 | 512 | 2296.252 | 2270.237 | >> 0.988670669 >> RotateBenchmark.testRotateLeftB | 256 | 15 | 256 | 4576.628 | 4515.53 | >> 0.986649996 >> RotateBenchmark.testRotateLeftB | 256 | 15 | 512 | 2288.278 | 2270.923 | >> 0.992415694 >> RotateBenchmark.testRotateLeftB | 256 | 31 | 256 | 4624.243 | 4511.46 | >> 0.975610495 >> RotateBenchmark.testRotateLeftB | 256 | 31 | 512 | 2305.459 | 2273.788 | >> 0.986262605 >> RotateBenchmark.testRotateLeftB | 512 | 7 | 256 | 7748.283 | .105 | >> 1.003719792 >> RotateBenchmark.testRotateLeftB | 512 | 7 | 512 | 3906.214 | 3912.647 | >> 1.001646863 >> RotateBenchmark.testRotateLeftB | 512 | 15 | 256 | 7764.653 | 7763.482 | >> 0.999849188 >> RotateBenchmark.testRotateLeftB | 512 | 15 | 512 | 3916.061 | 3919.363 | >> 1.000843194 >> RotateBenchmark.testRotateLeftB | 512 | 31 | 256 | 7779.754 | 7770.239 | >> 0.998776954 >> RotateBenchmark.testRotateLeftB | 512 | 31 | 512 | 3916.471 | 3912.718 | >> 0.999041739 >> RotateBenchmark.testRotateLeftI | 128 | 7 | 256 | 4043.39 | 13461.814 | >> 3.329338501 >> RotateBenchmark.testRotateLeftI | 128 | 7 | 512 | 1996.217 | 6455.425 | >> 3.233829288 >> RotateBenchmark.testRotateLeftI | 128 | 15 | 256 | 4028.614 | 13077.277 | >> 3.246098286 >> RotateBenchmark.testRotateLeftI | 128 | 15 | 512 | 1997.612 | 6452.918 | >> 3.230315997 >> RotateBenchmark.testRotateLeftI | 128 | 31 | 256 | 4123.357 | 13079.045 | >> 3.171940969 >> RotateBenchmark.testRotateLeftI | 128 | 31 | 512 | 2003.356 | 6452.716 | >> 3.22095324 >> RotateBenchmark.testRotateLeftI | 256 | 7 | 256 | 7666.949 | 25658.625 | >> 3.34665393 >> RotateBenchmark.testRotateLeftI | 256 | 7 | 512 | 3855.826 | 12278.106 | >> 3.18429981 >> RotateBenchmark.testRotateLeftI | 256 | 15 | 256 | 7670.901 | 24625.466 | >> 3.210244272 >> RotateBenchmark.testRotateLeftI | 256 | 15 | 512 | 3765.786 | 12272.771 | >> 3.259019764 >> RotateBenchmark.testRotateLeftI | 256 | 31 | 256 | 7660.599 | 25678.864 | >> 3.352069988 >> RotateBenchmark.testRotateLeftI | 256 | 31 | 512 | 3773.401 | 12006.469 | >> 3.181869353 >> RotateBenchmark.testRotateLeftI | 512 | 7 | 256 | 11900.948 | 31242.989 | >> 2.625252123 >> RotateBenchmark.testRotateLeftI | 512 | 7 | 512 | 5830.878 | 15727.149 | >> 2.697217983 >> RotateBenchmark.testRotateLeftI | 512 | 15 | 256 | 12171.847 | 33180.067 | >> 2.72596813 >> RotateBenchmark.testRotateLeftI | 512 | 15 | 512 | 5830.544 | 16740.182 | >> 2.871118372 >> RotateBenchmark.testRotateLeftI | 512 | 31 | 256 | 11909.553 | 31250.882 | >> 2.624018047 >> RotateBenchmark.testRotateLeftI | 512 | 31 | 512 | 5846.747 | 15738.831 | >> 2.691895339 >> RotateBenchmark.testRotateLeftL | 128 | 7 | 256 | 2047.243 | 6888.484 | >> 3.364761291 >> RotateBenchmark.testRotateLeftL | 128 | 7 | 512 | 1005.029 | 3245.931
[jdk17] Integrated: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Mon, 28 Jun 2021 21:09:43 GMT, Weijun Wang wrote: > Add a cache to record which sources have called `System::setSecurityManager` > and only print out warning lines once for each. This pull request has now been integrated. Changeset: c4ea13ed Author:Weijun Wang URL: https://git.openjdk.java.net/jdk17/commit/c4ea13edd036bd6aeb213bb5391dd374d283d382 Stats: 59 lines in 2 files changed: 40 ins; 6 del; 13 mod 8269543: The warning for System::setSecurityManager should only appear once for each caller Reviewed-by: lancea, alanb, dfuchs - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller [v2]
On Wed, 30 Jun 2021 06:32:04 GMT, Alan Bateman wrote: >> I hope this is uncommon but if that class is loaded by a `ClassLoader` again >> and again then it will be different each time. I'll investigate more. > > WeakHashMap, Boolean>, where the key is the caller, should be okay > (assume synchronization of course). Even with applications that do call > setSecurityManager then the map is probably going to be one entry. I wouldn't > expect it is common to create custom class loaders that load code that sets a > security manager, meaning it is more likely that the container sets the SM > rather have each plugin/application/whatever try to set the system-wide SM. Thanks. Switched to Class as cache key. New commit pushed. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller [v2]
> Add a cache to record which sources have called `System::setSecurityManager` > and only print out warning lines once for each. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: update cache key from String to Class - Changes: - all: https://git.openjdk.java.net/jdk17/pull/166/files - new: https://git.openjdk.java.net/jdk17/pull/166/files/a9188921..c158d4bf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=166=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=166=00-01 Stats: 9 lines in 1 file changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk17/pull/166.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/166/head:pull/166 PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 19:57:24 GMT, Roger Riggs wrote: >> If I switch to a "non-weak" set or map, then it seems I can safely use the >> source string as the key. Will using the Class object as a key prevent them >> from unloading? > > Using a synchronized WeakHashMap with the class as the key would not prevent > class unloading. > Using a non-weak set or map to strings would keep the strings around for the > life of the runtime. I hope this is uncommon but if that class is created by a `ClassLoader` again and again then it will be different each time. I'll investigate more. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 19:23:26 GMT, Roger Riggs wrote: >> src/java.base/share/classes/java/lang/System.java line 337: >> >>> 335: = Collections.synchronizedMap(new WeakHashMap<>()); >>> 336: } >>> 337: >> >> I wonder about the use of a WeakHashMap here. That may work well when the >> source is an interned string (a class name) which will be strongly >> referenced elsewhere and may be garbage collected if the class is unloaded, >> but in the case where it contains the name of the source jar then that >> string will only be referenced by the weak hashmap, and therefore it could >> be garbage collected any time - which would cause the mapping to be removed. >> In that case you cannot guarantee that the warning will be emitted only once. > > Using a HashSet could use the callerClass as the key and be a stable > reference for having given the message. > or use a ConcurrentHashMap>, boolean> and avoid any separate > synchronization that would be needed with a HashSet or HashMap. If I switch to a "non-weak" set or map, then it seems I can safely use the source string as the key. Will using the Class object as a key prevent them from unloading? - PR: https://git.openjdk.java.net/jdk17/pull/166
[jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
Add a cache to record which source has called `System::setSecurityManager` and only print out warning lines once for each. - Commit messages: - renaming variables, two callers in test - one warning for each caller Changes: https://git.openjdk.java.net/jdk17/pull/166/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=166=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269543 Stats: 47 lines in 2 files changed: 34 ins; 0 del; 13 mod Patch: https://git.openjdk.java.net/jdk17/pull/166.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/166/head:pull/166 PR: https://git.openjdk.java.net/jdk17/pull/166
Integrated: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
On Mon, 28 Jun 2021 18:03:56 GMT, Weijun Wang wrote: > More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. > > Note: this is copied from https://github.com/openjdk/jdk17/pull/152. This pull request has now been integrated. Changeset: e9b2c058 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/e9b2c058a4ed5de29b991360f78fc1c5263c9268 Stats: 293 lines in 21 files changed: 165 ins; 64 del; 64 mod 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K Reviewed-by: lancea, naoto - PR: https://git.openjdk.java.net/jdk/pull/4615
RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
More refactoring to limit the scope of `@SuppressWarnings` annotations. Sometimes I introduce new methods. Please feel free to suggest method names you like to use. Note: this is copied from https://github.com/openjdk/jdk17/pull/152. - Commit messages: - copy all code change from jdk17 Changes: https://git.openjdk.java.net/jdk/pull/4615/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4615=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269409 Stats: 293 lines in 21 files changed: 165 ins; 64 del; 64 mod Patch: https://git.openjdk.java.net/jdk/pull/4615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4615/head:pull/4615 PR: https://git.openjdk.java.net/jdk/pull/4615
[jdk17] Withdrawn: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
On Fri, 25 Jun 2021 20:04:37 GMT, Weijun Wang wrote: > More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]
On Fri, 25 Jun 2021 23:40:27 GMT, Weijun Wang wrote: >> More refactoring to limit the scope of `@SuppressWarnings` annotations. >> >> Sometimes I introduce new methods. Please feel free to suggest method names >> you like to use. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > one more I'm going to move this to jdk18. - PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]
On Mon, 28 Jun 2021 12:20:38 GMT, Daniel Fuchs wrote:
>> This cast is only to tell the compiler which overloaded method to call, and
>> I don't think there will be a real cast at runtime. It might look a little
>> ugly but extracting it into a variable declaration/definition plus a new
>> `initStatic` method seems not worth doing, IMHO.
>
> Why not simply declare a local variable in the static initializer below?
>
>
> private static final long CURRENT_PID;
> private static final boolean ALLOW_ATTACH_SELF;
> static {
> PrivilegedAction pa = ProcessHandle::current;
> @SuppressWarnings("removal")
> long pid = AccessController.doPrivileged(pa).pid();
> CURRENT_PID = pid;
> String s = VM.getSavedProperty("jdk.attach.allowAttachSelf");
> ALLOW_ATTACH_SELF = "".equals(s) || Boolean.parseBoolean(s);
> }
I've just pushed a commit with a different fix:
private static final long CURRENT_PID = pid();
@SuppressWarnings("removal")
private static long pid() {
PrivilegedAction pa = () -> ProcessHandle.current();
return AccessController.doPrivileged(pa).pid();
}
-
PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v3]
> More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: one more refinement - Changes: - all: https://git.openjdk.java.net/jdk17/pull/152/files - new: https://git.openjdk.java.net/jdk17/pull/152/files/774eb9da..2e4a8ba7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=152=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=152=01-02 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk17/pull/152.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152 PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]
On Sat, 26 Jun 2021 16:53:30 GMT, Alan Bateman wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> one more > > src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java line > 53: > >> 51: private static final long CURRENT_PID = >> AccessController.doPrivileged( >> 52: (PrivilegedAction) >> ProcessHandle::current).pid(); >> 53: > > The original code separated out the declaration of the PrivilegedAction to > avoid this cast. If you move the code from the original static initializer > into a static method that it called from initializer then it might provide > you with a cleaner way to refactor this. There are several other places in > this patch that could do with similar cleanup. This cast is only to tell the compiler which overloaded method to call, and I don't think there will be a real cast at runtime. It might look a little ugly but extracting it into a variable declaration/definition plus a new `initStatic` method seems not worth doing, IMHO. - PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]
> More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: one more - Changes: - all: https://git.openjdk.java.net/jdk17/pull/152/files - new: https://git.openjdk.java.net/jdk17/pull/152/files/d8b384df..774eb9da Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=152=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=152=00-01 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk17/pull/152.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152 PR: https://git.openjdk.java.net/jdk17/pull/152
[jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
More refactoring to limit the scope of `@SuppressWarnings` annotations. Sometimes I introduce new methods. Please feel free to suggest method names you like to use. - Commit messages: - 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K Changes: https://git.openjdk.java.net/jdk17/pull/152/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=152=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269409 Stats: 289 lines in 21 files changed: 161 ins; 64 del; 64 mod Patch: https://git.openjdk.java.net/jdk17/pull/152.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152 PR: https://git.openjdk.java.net/jdk17/pull/152
Withdrawn: 8268349: Provide more detail in JEP 411 warning messages
On Mon, 7 Jun 2021 20:42:53 GMT, Weijun Wang wrote: > More loudly and precise warning messages when a security manager is either > enabled at startup or installed at runtime. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Tue, 8 Jun 2021 17:15:29 GMT, Alan Bateman wrote: >> I thought about that but not sure of performance impact. Is the worst >> problem that more than one warnings will be printed for a single caller? >> It's not really harmless. >> >> As for the frame, if the warning message only contain the caller class name >> and its code source, why is it worth using a key of multiple frames? The >> message will look the same. > >> I thought about that but not sure of performance impact. Is the worst >> problem that more than one warnings will be printed for a single caller? >> It's not really harmless. >> >> As for the frame, if the warning message only contain the caller class name >> and its code source, why is it worth using a key of multiple frames? The >> message will look the same. > > WeakHashMap access needs synchronization. Whether we need to cache to avoid > excessive warnings isn't clear. If the SM is enabled once and never > disabled/re-enabled then caching isn't interesting. On the other hand if > there are programs that are enabling/disabling to execute subsets of code > then maybe it is. Maybe we should just drop this and see if there is any > feedback on the repeated warning? Not sure what you meant by "WeakHashMap access synchronization", it's just a noun without any other parts. Do you think synchronization is necessary? For the cache, I'm OK to drop it at the moment. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Tue, 8 Jun 2021 06:11:17 GMT, Alan Bateman wrote: >> More loudly and precise warning messages when a security manager is either >> enabled at startup or installed at runtime. > > src/java.base/share/classes/java/lang/System.java line 331: > >> 329: >> 330: // Remember original System.err. setSecurityManager() warning goes >> here >> 331: private static PrintStream oldErrStream = null; > > I assume this should needs to be volatile and @Stable. I think we need a > better name for it too. Will add the modifiers. How about "originalErr"? > src/java.base/share/classes/java/lang/System.java line 336: > >> 334: // Remember callers of setSecurityManager() here so that warning >> 335: // is only printed once for each different caller >> 336: final static Map callersOfSSM = new >> WeakHashMap<>(); > > You can't use a WeakHashMap without synchronization but a big question here > is whether a single caller frame is sufficient. If I were doing this then I > think I would capture the hash of a number of stack frames to create a better > filter. I thought about that but not sure of performance impact. Is the worst problem that more than one warnings will be printed for a single caller? It's not really harmless. As for the frame, if the warning message only contain the caller class name and its code source, why is it worth using a key of multiple frames? The message will look the same. > src/java.base/share/classes/java/lang/System.java line 2219: > >> 2217: WARNING: java.lang.SecurityManager is >> deprecated and will be removed in a future release >> 2218: WARNING: -Djava.security.manager=%s >> will have no effect when java.lang.SecurityManager is removed >> 2219: """, smProp); > > Raw strings may be useful here but means the lines length are inconsistent > and makes it too hard to look at side by side diffs now. I understand what you mean when I switch to Split View. While I can extract the lines to a method, I somehow think it's not worth doing because for each type of warning the method is only called once. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Tue, 8 Jun 2021 06:41:11 GMT, Alan Bateman wrote: > > You might want to make your "WARNING" consistent with the VM's "Warning" so > > that OutputAnalyzer's logic to ignore warnings will automatically ignore > > these too. > > The uppercase "WARNING" is intentional here, it was the same with illegal > reflective access warnings. I'm sure Max has or will run all tests to see if > there are any issues. Will definitely run all from tier1-tier9. I ran them multiple times while implementing JEP 411. I've seen warnings with "VM" word in the prefix and test methods that filter them out, but feel the warnings here are not related to VM. The new warnings do have impacts on some tests and I'll be very carefully not break them. - PR: https://git.openjdk.java.net/jdk/pull/4400
RFR: 8268349: Provide more detail in JEP 411 warning messages
More loudly and precise warning messages when either a security manager is enabled at startup or installed at runtime. - Commit messages: - 8268349: Provide more detail in JEP 411 warning messages Changes: https://git.openjdk.java.net/jdk/pull/4400/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4400=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268349 Stats: 115 lines in 5 files changed: 73 ins; 21 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/4400.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4400/head:pull/4400 PR: https://git.openjdk.java.net/jdk/pull/4400
Integrated: 8267521: Post JEP 411 refactoring: maximum covering > 50K
On Fri, 21 May 2021 01:52:27 GMT, Weijun Wang wrote:
> The code change refactors classes that have a `SuppressWarnings("removal")`
> annotation that covers more than 50KB of code. The big annotation is often
> quite faraway from the actual deprecated API usage it is suppressing, and
> with the annotation covering too big a portion it's easy to call other
> deprecated methods without being noticed.
>
> The code change shows some common solutions to avoid such too wide
> annotations:
>
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local
> variable and add annotation on the declaration, and then assign that value to
> the original l-value if not void. The local variable will be called `tmp` if
> later reassigned or `dummy` if it will be discarded.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
>
> I'll add a copyright year update commit before integration.
This pull request has now been integrated.
Changeset: 508cec75
Author:Weijun Wang
URL:
https://git.openjdk.java.net/jdk/commit/508cec7535cd0ad015d566389bc9e5f53ce4103b
Stats: 245 lines in 18 files changed: 140 ins; 39 del; 66 mod
8267521: Post JEP 411 refactoring: maximum covering > 50K
Reviewed-by: dfuchs, prr
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v4]
> The code change refactors classes that have a `SuppressWarnings("removal")`
> annotation that covers more than 50KB of code. The big annotation is often
> quite faraway from the actual deprecated API usage it is suppressing, and
> with the annotation covering too big a portion it's easy to call other
> deprecated methods without being noticed.
>
> The code change shows some common solutions to avoid such too wide
> annotations:
>
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local
> variable and add annotation on the declaration, and then assign that value to
> the original l-value if not void. The local variable will be called `tmp` if
> later reassigned or `dummy` if it will be discarded.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
>
> I'll add a copyright year update commit before integration.
Weijun Wang has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains one commit:
8267521: Post JEP 411 refactoring: maximum covering > 50K
-
Changes: https://git.openjdk.java.net/jdk/pull/4138/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4138=03
Stats: 245 lines in 18 files changed: 140 ins; 39 del; 66 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
Integrated: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. This pull request has now been integrated. Changeset: 6765f902 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/6765f902505fbdd02f25b599f942437cd805cad1 Stats: 2755 lines in 826 files changed: 1997 ins; 20 del; 738 mod 8266459: Implement JEP 411: Deprecate the Security Manager for Removal Co-authored-by: Sean Mullan Co-authored-by: Lance Andersen Co-authored-by: Weijun Wang Reviewed-by: erikj, darcy, chegar, naoto, joehw, alanb, mchung, kcr, prr, lancea - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v9]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits: - copyright years - merge from master, resolve one conflict - Merge branch 'master' - merge from master - rename setSecurityManagerDirect to implSetSecurityManager - default behavior reverted to allow - move one annotation to new method - merge from master, two files removed, one needs merge - keep only one systemProperty tag - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java - ... and 4 more: https://git.openjdk.java.net/jdk/compare/19450b99...331389b5 - Changes: https://git.openjdk.java.net/jdk/pull/4073/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4073=08 Stats: 2755 lines in 826 files changed: 1997 ins; 20 del; 738 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v8]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - merge from master - rename setSecurityManagerDirect to implSetSecurityManager - default behavior reverted to allow - move one annotation to new method - merge from master, two files removed, one needs merge - keep only one systemProperty tag - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java - feedback from Sean, Phil and Alan - add supresswarnings annotations automatically - manual change before automatic annotating - ... and 1 more: https://git.openjdk.java.net/jdk/compare/74b70a56...ea2c4b48 - Changes: https://git.openjdk.java.net/jdk/pull/4073/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4073=07 Stats: 2132 lines in 826 files changed: 1997 ins; 20 del; 115 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v7]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: rename setSecurityManagerDirect to implSetSecurityManager - Changes: - all: https://git.openjdk.java.net/jdk/pull/4073/files - new: https://git.openjdk.java.net/jdk/pull/4073/files/8fd09c39..926e4b9a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4073=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4073=05-06 Stats: 5 lines in 1 file changed: 0 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v6]
On Mon, 31 May 2021 15:02:57 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > default behavior reverted to allow New commit pushed. The default behavior is now reverted to be equivalent to `-Djava.security.manager=allow`. No warning will be shown when the system property is set to "allow", "disallow", or not set. A new test is added to check these warning messages. Some tests are updated to match the new behavior. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v6]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: default behavior reverted to allow - Changes: - all: https://git.openjdk.java.net/jdk/pull/4073/files - new: https://git.openjdk.java.net/jdk/pull/4073/files/01dc4c0d..8fd09c39 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4073=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4073=04-05 Stats: 183 lines in 6 files changed: 127 ins; 23 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 28 May 2021 03:12:35 GMT, Phil Race wrote: >> There *is* a tiny refactoring here: a new variable `s2` is introduced so the >> 2nd `doPrivileged` call on line 636 is now also in a declaration statement >> (for `s2`) and therefore annotatable. Without this I cannot add the >> annotation on line 635. > > Ok. But I will quote you > "This happens when a deprecated method is called inside a static block. The > annotation can only be added to a declaration and here it must be the whole > class" > > So the way you explained this before made it sound like any time there was > any SM API usage in a static block, the entire class needed to be annotated. > > Why has the explanation changed ? I should have been more precise. An annotation can only be added on a declaration, whether it's a variable, a method, or a class. Static block is not a declaration and the only one covers it is the class. But then if it's on a local variable declaration inside a static block, we certainly can annotate on that variable. - PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Thu, 27 May 2021 17:42:56 GMT, Phil Race wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update FtpClient.java
>
> src/java.desktop/share/classes/java/awt/Component.java line 630:
>
>> 628: }
>> 629:
>> 630: @SuppressWarnings("removal")
>
> I'm confused. I thought the reason this wasn't done in the JEP implementation
> PR is because of refactoring
> that was needed because of the usage in this static block and you could not
> apply the annotation here.
> Yet it seems you are doing exactly what was supposed to be impossible with no
> refactoring.
> Can you explain ?
There *is* a tiny refactoring here: a new variable `s2` is introduced so the
2nd `doPrivileged` call on line 636 is now also in a declaration statement (for
`s2`) and therefore annotatable. Without this I cannot add the annotation on
line 635.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v5]
On Thu, 27 May 2021 20:16:25 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - move one annotation to new method > - merge from master, two files removed, one needs merge > - keep only one systemProperty tag > - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java > - feedback from Sean, Phil and Alan > - add supresswarnings annotations automatically > - manual change before automatic annotating > - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal Two files were removed by JEP 403 and JEP 407, respectively. One method in `XMLSchemaFactory.java` got [its own](https://github.com/openjdk/jdk/commit/8c4719a58834dddcea39d69b199abf1aabf780e2#diff-593a224979eaff03e2a3df1863fcaf865364a31a2212cc0d1fe67a8458057857R429) `@SuppressWarnings` and have to be merged with the one here. Another file `ResourceBundle.java` had a portion of a method extracted into a [new method](https://github.com/openjdk/jdk/commit/a4c46e1e4f4f2f05c8002b2af683a390fc46b424#diff-59caf1a68085064b4b3eb4f6e33e440bb85ea93719f34660970e2d4eaf8ce469R3175) and the annotation must be moved there. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v5]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - move one annotation to new method - merge from master, two files removed, one needs merge - keep only one systemProperty tag - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java - feedback from Sean, Phil and Alan - add supresswarnings annotations automatically - manual change before automatic annotating - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal - Changes: https://git.openjdk.java.net/jdk/pull/4073/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4073=04 Stats: 2022 lines in 825 files changed: 1884 ins; 10 del; 128 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Mon, 24 May 2021 09:00:07 GMT, Daniel Fuchs wrote: > Thanks for taking in my suggestions for FtpClient. I have also reviewed the > changes to java.util.logging and JMX. I wish there was a way to handle > doPrivileged returning void more nicely. Maybe component maintainers will be > able to deal with them on a case-by-case basis later on. Assigning to a useless "dummy" variable looks ugly. Extracting the call to a method might make it faraway from its caller. In L114 of `FtpClient.java` I managed to invent a return value and I thought it was perfect. But you said it's "a bit strange". :-( - PR: https://git.openjdk.java.net/jdk/pull/4138
Integrated: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang wrote:
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
This pull request has now been integrated.
Changeset: 640a2afd
Author:Weijun Wang
URL:
https://git.openjdk.java.net/jdk/commit/640a2afda36857410b7abf398af81e35430a62e7
Stats: 1028 lines in 852 files changed: 252 ins; 9 del; 767 mod
8267184: Add -Djava.security.manager=allow to tests calling
System.setSecurityManager
Co-authored-by: Lance Andersen
Co-authored-by: Weijun Wang
Reviewed-by: dholmes, alanb, dfuchs, mchung, mullan, prr
-
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v4]
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
Weijun Wang 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 20 additional commits since the
last revision:
- Merge branch 'master' into 8267184
- feedback from Phil
reverted:
- adjust order of VM options
- test for awt
- test for hotspot-gc
- test for compiler
- test for net
- test for core-libs
- test for beans
- test for xml
- ... and 10 more: https://git.openjdk.java.net/jdk/compare/37f74de7...412264a0
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4071/files
- new: https://git.openjdk.java.net/jdk/pull/4071/files/9a3ec578..412264a0
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4071=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4071=02-03
Stats: 12227 lines in 453 files changed: 6978 ins; 3721 del; 1528 mod
Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]
On Sun, 23 May 2021 16:35:43 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/java/lang/SecurityManager.java line 104:
>>
>>> 102: * method will throw an {@code UnsupportedOperationException}). If the
>>> 103: * {@systemProperty java.security.manager} system property is set to
>>> the
>>> 104: * special token "{@code allow}", then a security manager will not be
>>> set at
>>
>> Can/should the `{@systemProperty ...}` tag be used more than once for a
>> given system property? I thought it should be used only once, at the place
>> where the system property is defined. Maybe @jonathan-gibbons can offer some
>> more guidance on this.
>
> Good point. I would remove the extra @systemProperty tags on lines 103, 106,
> and 113. Also, in `System.setSecurityManager` there are 3 @systemProperty
> java.security.manager tags, so we should remove those too.
New commit pushed. There is only one `@systemProperty` tag now.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: keep only one systemProperty tag - Changes: - all: https://git.openjdk.java.net/jdk/pull/4073/files - new: https://git.openjdk.java.net/jdk/pull/4073/files/c4221b5f..1f6ff6c4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4073=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4073=02-03 Stats: 8 lines in 2 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Fri, 21 May 2021 20:43:05 GMT, Phil Race wrote:
> I haven't seen any response to this comment I made a couple of days ago and I
> suspect it got missed
>
> > Weijun Wang has updated the pull request incrementally with one additional
> > commit since the last revision:
> > fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
>
> test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
>
> > 57: ProcessCommunicator
> > 58: .executeChildProcess(Consumer.class, new
> > String[0]);
> > 59: if (!"Hello".equals(processResults.getStdOut())) {
>
> Who or what prompted this change ?
I replied right in the thread but unfortunately GitHub does not display it at
the end of page.
This is because the process is now launched with
`-Djava.security.manager=allow` (because of another change in
https://github.com/openjdk/jdk/pull/4071), and a new warning is displayed in
stderr. Therefore I switched to stdout.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
> The code change refactors classes that have a `SuppressWarnings("removal")`
> annotation that covers more than 50KB of code. The big annotation is often
> quite faraway from the actual deprecated API usage it is suppressing, and
> with the annotation covering too big a portion it's easy to call other
> deprecated methods without being noticed.
>
> The code change shows some common solutions to avoid such too wide
> annotations:
>
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local
> variable and add annotation on the declaration, and then assign that value to
> the original l-value.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
update FtpClient.java
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4138/files
- new: https://git.openjdk.java.net/jdk/pull/4138/files/d460efb8..60d97a4c
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4138=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4138=01-02
Stats: 23 lines in 1 file changed: 0 ins; 12 del; 11 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
On Fri, 21 May 2021 15:37:48 GMT, Daniel Fuchs wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:
>
>> 548: * @throws IOException if the connection was unsuccessful.
>> 549: */
>> 550: @SuppressWarnings("removal")
>
> Could the scope of the annotation be further reduced by applying it to the
> two places where `doPrivileged` is called in this method?
I'll probably need to assign the doPriv result on L631 to a tmp variable and
then assign it to `s`. Are you OK with this ugliness? Update: Ah, I see you
already have similar suggestion in the next comment.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
On Fri, 21 May 2021 15:35:05 GMT, Daniel Fuchs wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:
>
>> 118: vals[1] =
>> Integer.getInteger("sun.net.client.defaultConnectTimeout",
>> 300_000).intValue();
>> 119: return System.getProperty("file.encoding",
>> "ISO8859_1");
>> 120: }
>
> It is a bit strange that "file.encoding" seem to get a special treatment -
> but I guess that's OK.
You might say we thus avoid wasting the return value, as much as possible.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v3]
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
feedback from Phil
reverted:
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4071/files
- new: https://git.openjdk.java.net/jdk/pull/4071/files/bfa955ad..9a3ec578
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4071=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4071=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
On Fri, 21 May 2021 18:26:48 GMT, Phil Race wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> adjust order of VM options > > test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3: > >> 1: /* >> 2: * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights >> reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > > Probably the (c) update was meant to be on the .sh file that is actually > modified. Oops, I'll back it out. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4071
