Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat [v4]
On Tue, 14 Jun 2022 12:18:52 GMT, Matthias Baesken wrote: >> When trying to construct an LdapURL object with a bad input string (in this >> example the _ in ad_jbs is causing issues), and not using >> the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we >> run into the exception below : >> >> import com.sun.jndi.ldap.LdapURL; >> >> String url = "ldap://ad_jbs.ttt.net:389/xyz;; // bad input string containing >> _ >> LdapURL ldapUrl = new LdapURL(url); >> >> >> java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest >> Exception in thread "main" javax.naming.NamingException: Cannot parse url: >> ldap://ad_jbs.ttt.net:389/xyz [Root exception is >> java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389] >> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115) >> at LdapParseUrlTest.main(LdapParseUrlTest.java:9) >> Caused by: java.net.MalformedURLException: unsupported authority: >> ad_jbs.ttt.net:389 >> at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367) >> at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230) >> at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174) >> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105) >> >> I would like to add the host and port info to the exception (in the example >> it is host:port of URI:null:-1] ) so that it is directly visible that the >> input caused the construction of a URI >> with "special"/problematic host and port values. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > avoid very long line Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat [v3]
On Tue, 14 Jun 2022 11:36:36 GMT, Matthias Baesken wrote:
>> When trying to construct an LdapURL object with a bad input string (in this
>> example the _ in ad_jbs is causing issues), and not using
>> the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we
>> run into the exception below :
>>
>> import com.sun.jndi.ldap.LdapURL;
>>
>> String url = "ldap://ad_jbs.ttt.net:389/xyz;; // bad input string containing
>> _
>> LdapURL ldapUrl = new LdapURL(url);
>>
>>
>> java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest
>> Exception in thread "main" javax.naming.NamingException: Cannot parse url:
>> ldap://ad_jbs.ttt.net:389/xyz [Root exception is
>> java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389]
>> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115)
>> at LdapParseUrlTest.main(LdapParseUrlTest.java:9)
>> Caused by: java.net.MalformedURLException: unsupported authority:
>> ad_jbs.ttt.net:389
>> at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367)
>> at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230)
>> at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174)
>> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105)
>>
>> I would like to add the host and port info to the exception (in the example
>> it is host:port of URI:null:-1] ) so that it is directly visible that the
>> input caused the construction of a URI
>> with "special"/problematic host and port values.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix copy paste error
src/java.naming/share/classes/com/sun/jndi/toolkit/url/Uri.java line 368:
> 366: // throw if we have user info or regname
> 367: throw new MalformedURLException("Authority
> component is not server-based, or contains user info. Unsupported authority:
> " + auth);
> 368: }
This looks okay but you may have to split up the line to avoid adding a 150+
char line (most of the file seems to keep the lines under 100 or so).
-
PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat
On Fri, 10 Jun 2022 13:41:48 GMT, Matthias Baesken wrote:
> Hi Alan , sure we could use something like the already existing hostInfo of
> property jdk.includeInException private static final boolean
> enhancedExceptionText = SecurityProperties.includedInExceptions("hostInfo");
> and make the enhancement optional/switchable this way. On the other hand we
> already print the url (_**Cannot parse url: ldap://ad_jbs.ttt.net:389/xyz**_
> ) in the existing exception text so I wonder what additional problem the
> added info would bring? That's why I did not use the property so far. But if
> you think there could be special cases were it would be problematic to have
> the enhancement, I'll add the usage of the property.
This is a security sensitive area and not possible to discuss all issues in JBS
or in this PR. If this code is changed then it will require someone from
security-dev to review.
-
PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat
On Fri, 10 Jun 2022 12:16:17 GMT, Matthias Baesken wrote: > When trying to construct an LdapURL object with a bad input string (in this > example the _ in ad_jbs is causing issues), and not using > the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we run > into the exception below : > > import com.sun.jndi.ldap.LdapURL; > > String url = "ldap://ad_jbs.ttt.net:389/xyz;; // bad input string containing _ > LdapURL ldapUrl = new LdapURL(url); > > > java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest > Exception in thread "main" javax.naming.NamingException: Cannot parse url: > ldap://ad_jbs.ttt.net:389/xyz [Root exception is > java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389] > at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115) > at LdapParseUrlTest.main(LdapParseUrlTest.java:9) > Caused by: java.net.MalformedURLException: unsupported authority: > ad_jbs.ttt.net:389 > at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367) > at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230) > at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174) > at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105) > > I would like to add the host and port info to the exception (in the example > it is host:port of URI:null:-1] ) so that it is directly visible that the > input caused the construction of a URI > with "special"/problematic host and port values. We have to be cautious about leaking security sensitive configuration in exception messages. Can you look at the security property jdk.includeInException (conf/security/java.security) and usages in the JDK for ideas on how this might be implemented as opt-in? - PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Fri, 13 May 2022 04:41:03 GMT, ExE Boss wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated copyrights >> Fixed cast style to add a space after cast, (where consistent with file >> style) >> Improved code per review comments in PollSelectors. > > src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128: > >> 126: // timed poll interrupted so need to adjust timeout >> 127: long adjust = System.nanoTime() - startTime; >> 128: to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust); > > This will now always assign a negative number to `to`. > > > > `=-` is not a compound assignment, it’s negation followed by a normal > assignment. Well spotted, I don't think that change was intentionally. - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Wed, 11 May 2022 16:30:41 GMT, Roger Riggs wrote: >> PR#8599 8244681: proposes to add compiler warnings for possible lossy >> conversions >> From the CSR: >> >> "If the type of the right-hand operand of a compound assignment is not >> assignment compatible with the type of the variable, a cast is implied and >> possible lossy conversion may silently occur. While similar situations are >> resolved as compilation errors for primitive assignments, there are no >> similar rules defined for compound assignments." >> >> This PR anticipates the warnings and adds explicit casts to replace the >> implicit casts. >> In most cases, the cast matches the type of the right-hand side to type of >> the compound assignment. >> Since these casts are truncating the value, there might be a different >> refactoring that avoids the cast >> but a direct replacement was chosen here. >> >> (Advise and suggestions will inform similar updates to other OpenJDK >> modules). > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Updated copyrights > Fixed cast style to add a space after cast, (where consistent with file > style) > Improved code per review comments in PollSelectors. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]
On Tue, 10 May 2022 23:01:33 GMT, Roger Riggs wrote:
>> PR#8599 8244681: proposes to add compiler warnings for possible lossy
>> conversions
>> From the CSR:
>>
>> "If the type of the right-hand operand of a compound assignment is not
>> assignment compatible with the type of the variable, a cast is implied and
>> possible lossy conversion may silently occur. While similar situations are
>> resolved as compilation errors for primitive assignments, there are no
>> similar rules defined for compound assignments."
>>
>> This PR anticipates the warnings and adds explicit casts to replace the
>> implicit casts.
>> In most cases, the cast matches the type of the right-hand side to type of
>> the compound assignment.
>> Since these casts are truncating the value, there might be a different
>> refactoring that avoids the cast
>> but a direct replacement was chosen here.
>>
>> (Advise and suggestions will inform similar updates to other OpenJDK
>> modules).
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove stray file
src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128:
> 126: // timed poll interrupted so need to adjust timeout
> 127: long adjust = System.nanoTime() - startTime;
> 128: to -= (int)TimeUnit.MILLISECONDS.convert(adjust,
> TimeUnit.NANOSECONDS);
Can we change this `to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);` while
we here?
src/java.base/share/classes/jdk/internal/org/objectweb/asm/Frame.java line 615:
> 613: if (outputStackTop >= elements) {
> 614: outputStackTop -= (short)elements;
> 615: } else {
I think we usually try to avoid changing the ASM code if possible but maybe you
have to do this because the compiler option is used for compiling java.base?
src/java.base/unix/classes/sun/nio/ch/PollSelectorImpl.java line 123:
> 121: // timed poll interrupted so need to adjust timeout
> 122: long adjust = System.nanoTime() - startTime;
> 123: to -= (int)TimeUnit.MILLISECONDS.convert(adjust,
> TimeUnit.NANOSECONDS);
This one can also be changed to:
`to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);`
-
PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults
On Sun, 8 May 2022 02:22:08 GMT, Phil Race wrote: > I did wonder why it has security-libs as the sub-category and if the intent > was not what we see here. I suspect the JBS issue was initially created to to look at the usages of getProperty in the security code but it has been extended. The issue is that it changes the meaning of the empty value case in at least two places so each one will require careful attention. - PR: https://git.openjdk.java.net/jdk/pull/8559
Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults
On Thu, 5 May 2022 16:49:07 GMT, Mark Powers wrote:
> JDK-6725221 Standardize obtaining boolean properties with defaults
src/java.base/share/classes/java/lang/reflect/AccessibleObject.java line 777:
> 775: if (!printStackPropertiesSet && VM.initLevel() >= 1) {
> 776: printStackWhenAccessFails = GetBooleanAction.
> 777:
> privilegedGetProperty("sun.reflect.debugModuleAccessChecks");
Running with `-Dsun.reflect.debugModuleAccessChecks` should set
printStackWhenAccessFails to true, not false.
-
PR: https://git.openjdk.java.net/jdk/pull/8559
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v4]
On Thu, 28 Apr 2022 16:58:40 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 01:34:19 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. src/java.base/share/classes/java/nio/file/SecureDirectoryStream.java line 55: > 53: * against the original path of the directory (irrespective of if > the > 54: * directory is moved since it was opened). > 55: * @param the type of path It may not be a path. The type parameter is specified in the super interfaces, can you copy that down instead? src/java.base/share/classes/java/nio/file/WatchEvent.java line 51: > 49: /** > 50: * An event kind, for the purposes of identification. > 51: * @param the type of the context value This is okay but the it differs slightly to the type parameters specified further up. I think the issue here is that it was just wasn't copied down to WatchEvent.Kind. - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: A possible JEP to replace SecurityManager after JEP 411
On 26/04/2022 10:06, Peter Firmstone wrote: : What about ensuring that all network access occurs through a single location that we can instrument? Network, file, and process launch are potentially interesting but instrumenting them to run arbitrary code may be problematic (for the same reasons that custom security managers can be problematic). -Alan
Re: A possible JEP to replace SecurityManager after JEP 411
On 25/04/2022 13:53, David Lloyd wrote: Nothing in the proposal causes splitting or delegation of responsibilities. It is _only_ about preserving security checkpoints in the JDK, which *add* a layer of checks to what the container might already provide. Nothing is being subtracted, and thus I find it hard to accept that preserving these checks somehow reduces security overall. "preserving security checkpoints in the JDK" suggests just leaving the calls do AccessController.doPrivileged and SecurityManager.checkPermission in place. That amounts to putting a tax on every feature, every JEP, and all ongoing maintenance of the platform. If there is refactoring or a change that forgets to insert a checkPermission somewhere then we have a security issue and everything that goes along with that. No thanks! I think Martin is right that hooking authorization libraries into low level libraries isn't the right way to do this. Aside from the complexity methods I would add that threads pools or any hand-off between threads will typically break when the context is not carried. One other point about authorization libraries wanting to hook into low level code is that it's a minefield of potential issues with recursive initialization, stack overflows and some really hard to diagnose issues. JDK-8155659 [1] is one report that comes to mind. -Alan [1] https://bugs.openjdk.java.net/browse/JDK-8155659
Re: Reproducer for JDK-8221218
On 25/04/2022 15:45, Flavia Rainone wrote: Hi everyone, I work with the XNIO ( https://github.com/xnio/xnio/ ) project, led by David Lloyd in CC. I'm not sure if this is the best way to get in touch, but I could not find out how to create an account for OpenJDK Jira to add a comment there. We have a reproducer for JDK-8221218, and it appears it is not solved yet (tested with jdk 11.0.12). It is not intermittent, and it happens when we try to do an SSLEngine.unwrap with a zero remaining() ByteBuffer (in older versions, the engine would just return a BUFFER_UNDERFLOW result). JDK-8221218 says it has been fixed in JDK 17. Have you tested with JDK 17 or JDK 18 ? -Alan
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net
On Mon, 25 Apr 2022 17:40:13 GMT, Mark Powers wrote: > https://bugs.openjdk.java.net/browse/JDK-8285504 > > JDK-8273046 is the umbrella bug for this bug. The changes were too large for > a single code review, so it was decided to split into smaller chunks. This is > one such chunk: > > open/src/java.base/share/classes/java/net src/java.base/share/classes/javax/net/ssl/SSLSessionContext.java line 110: > 108: */ > 109: void setSessionTimeout(int seconds) > 110: throws IllegalArgumentException; IllegalArgumentException is a runtime exception, it's unusual to have "throws IllegalArgumentException". It would not impact compatibility to drop it. The important thing is that the javadoc has the `@throws` describing when it is thrown. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: 8285380: Fix typos in security
On Thu, 21 Apr 2022 14:11:08 GMT, Alan Bateman wrote: >> I ran `codespell` on modules owned by the security team (`java.security.jgss >> java.security.sasl java.smartcardio java.xml.crypto jdk.crypto.cryptoki >> jdk.crypto.ec jdk.crypto.mscapi jdk.security.auth jdk.security.jgss`), and >> accepted those changes where it indeed discovered real typos. >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > The folks on security-dev will know for sure but I assume that the changes to > the imported Apache Santuario code should be dropped as it will make upgrades > more complicated. > @AlanBateman So there is even more 3rd party code in there? :-( I tried to > ignore fixes for all files that I could identify as 3rd party. It's actually > a bit annoying that we have imported source code thrown around like this in > the source tree, so there is no clear boundary between code we own and code > we import from someone else... security-dev can say for sure but the only 3rd party code I see in this change is in the src/java.xml.crypto/share/classes/com/sun/org/apache tree (the package name gives a hint has it was it was re-packaged). - PR: https://git.openjdk.java.net/jdk/pull/8340
Re: RFR: 8285380: Fix typos in security
On Thu, 21 Apr 2022 13:51:27 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the security team (`java.security.jgss > java.security.sasl java.smartcardio java.xml.crypto jdk.crypto.cryptoki > jdk.crypto.ec jdk.crypto.mscapi jdk.security.auth jdk.security.jgss`), and > accepted those changes where it indeed discovered real typos. > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. The folks on security-dev will know for sure but I assume that the changes to the imported Apache Santuario code should be dropped as it will make upgrades more complicated. - PR: https://git.openjdk.java.net/jdk/pull/8340
Re: RFR: 8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297
On Mon, 18 Apr 2022 15:13:14 GMT, Anthony Scarpino wrote: > x86-64 only uses this intrinsic, aarch64 does not support this intrinsic and > uses the java code. It looks like aarch64 has an implementation of generate_galoisCounterMode_AESCrypt, isn't that the code that implGCMCrypt0 runs? - PR: https://git.openjdk.java.net/jdk/pull/8280
Re: RFR: 8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297
On Mon, 18 Apr 2022 05:06:26 GMT, Smita Kamath wrote: > When input length provided to the intrinsic is 8192, only 7680 bytes are > processed as the intrinsic operates on multiples of 768 bytes. > In implGCMCrypt(ByteBuffer src, ByteBuffer dst) method, > dst.put(bout, 0, PARALLEL_LEN) statement caused the ciphertext mismatch as > PARALLEL_LEN was set to 8192. > Since the intrinsic only processed 7680 bytes, the rest output was incorrect. It's good that this issue has been found. There seems to be an intrinsic for aarch64 with a vectorized GCM implementation, I guess is must also work in multiples of 768 bytes, so is this change okay there too? - PR: https://git.openjdk.java.net/jdk/pull/8280
Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
On 17/04/2022 17:24, liach wrote:
Convert dynamic proxies to hidden classes. Modifies the serialization of proxies
(requires change in "Java Object Serialization Specification"). Makes the
proxies hidden in stack traces. Removes duplicate logic in proxy building.
The main compatibility changes and their rationales are:
1. Modification to the serialization specification: In the "An instance of the class
is allocated... The contents restored appropriately" section, I propose explicitly
state that handling of proxies are unspecified as to allow implementation freedom, though
I've seen deliberate attempts for proxies to implement interfaces with `readResolve` in
order to control their serialization behavior.
- This is for the existing generated constructor accessor is
bytecode-based, which cannot refer to hidden classes.
- An alternative is to preserve the behavior, where the serialization
constructor calls `invokespecial` on the closest serializable superclass'
no-arg constructor, like in #1830 by @DasBrain.
- My rationale against preservation is such super calls are unsafe and
should be discouraged in the long term. Calling the existing constructor with a
dummy argument, as in my implementation, would be more safe.
2. The disappearance of proxies in stack traces.
- Same behavior exists in lambda expressions: For instance, in `((Runnable) ()
-> { throw new Error(); }).run();`, the `run` method, implemented by the
lambda, will not appear in the stack trace, and isn't too problematic.
It's great that you have time to experiment in this area but just to set
expectations that it will likely require significant discussion and
maybe prototyping of alternatives. It suspect the Reviewer effort will
end up being many times the effort required to do the actual work
because of the compatibility and security issues that will need to be
worked through.
I think you need to add non-discoverability to the list of compatibility
issues. Do we know for sure that there aren't frameworks and libraries
that use Class.forName on proxy classes? We've had issues in the past
with changes in this area.
It's too early to say but it might be that the compatibility risks may
nudge this one into creating a new API.
-Alan.
Re: RFR: 8284893: Fix typos in java.base
On Thu, 14 Apr 2022 19:07:09 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. I skimmed through the changes to src/java.base and they look okay except for the changes to 3rd party code that I think should be dropped from this patch. src/java.base/share/native/libzip/zlib/inftrees.h line 65: > 63:1444, which is the sum of 852 for literal/length codes and 592 for > distance > 64:codes. These values were found by exhaustive searches using the > program > 65:examples/enough.c found in the zlib distribution. The arguments to > that This is 3rd party code so should be dropped from the patch as we want as few changes to this code has possible. src/java.base/windows/native/libnio/ch/wepoll.c line 894: > 892: * error code when the once-callback returns FALSE. We return -1 > here to > 893: * indicate that global initialization failed; the failing init > function is > 894: * responsible for setting `errno` and calling `SetLastError()`. */ This is also 3rd party code so should be dropped from this patch. - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Fri, 8 Apr 2022 22:55:07 GMT, Bradford Wetmore wrote: > @AlanBateman, @dfuch, @bchristi-git, any great ideas here? As you have found, if an open Socket is unreferenced then a cleaner can close the underlying socket (and release the file descriptor). The Cleaner is setup by the SocketImpl implementation. What is the existing behavior with the BaseSSLSocketImpl finalizer? Does it just close the socket or does it to the close_notify before closing the socket. If it does, and you want to keep that behavior, then you are probably into significant refactoring to avoid a reference to the BaseSSLSocketImpl. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: protecting security-sensitive operations on multi-tenant servers
On 27/03/2022 14:45, Rick Hillegas wrote: From the silence, I assume that there isn't any advice I can give Derby users. At this time the Security Manager is the only mechanism for protecting an application against these threats. Users should ignore the deprecation diagnostics and set -Djava.security.manager=allow. I think it's more that the SM was never the right solution for this type of isolation. Also some of the "operations" that you list, creating class loaders, de-registering JDBC drivers, ... suggest there may be potentially malicious code in these environments too. Do you know if these are legacy deployments or Derby users that haven't explored OS containers to isolate applications on the same hardware? -Alan
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 Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1611
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 Thanks for dropping the charset and locale data from the proposal. The updated proposal (b1d1e4d8) looks okay but I can't tell if you are planning to integrate this or wait until there is discussion on the locations proposed in the Informational JEP that you've just submitted. Maybe you plan to just integrate and then adjust/move again if needed? I suspect that JEP will need to includes a "specs" directory. It's okay to jdwp.spec into the java.se "data" directory for now I think "specs" would be a bette place for it. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On 16/03/2022 08:44, Magnus Ihse Bursie wrote: : If you have such strong opinions on the data files shared between java.base and jdk.charsets/jdk.localedata, I propose we leave them in make/data for the time being, clean up the associated mess, and then work out where they actually should be. Does that sound okay to you? The concern, as before, is that it puts data files into src/java.base that are used by the build to generate classes/resources for the service provider modules. We also have the complication that the charsets to include in java.base varies by platform so the module/package for each charset is decided at build time. It's always been low-priority to re-visit that and not clear if we could even get to an agreement easily because there are IBM platforms that want EBCDIC and other double byte charsets whereas other platforms don't want these in java.base. So yes, if you can drop the move of the charset data and CLDR data from the patch then it will make it easier to discuss. You asked about the JDWP spec. This is the protocol spec that is used to generate the spec in HTML, and source files for the debugger front-end and backend (jdk.jdi and jdk.jdwp.agent modules). The "specs" directory might be right. There is another source file (jdwp-spec.md) that isn't in the src tree right now and they probably go together. -Alan
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
Magnus,
This proposal does not address the previous concerns. As before, you are
proposing to put the data files used to generate the classes for
jdk.localedata and jdk.charsets into src/java.base and I don't think we
should do that. I really think this PR needs to be withdraw n or closed
until there is at least some agreement on placement. I know you want to
get the files moved out of the make tree but there are many issues to
work through before that can happen.
-Alan
On 15/03/2022 23:59, Magnus Ihse Bursie wrote:
On Tue, 15 Mar 2022 23:50:20 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 with a new target base due to a
merge or a rebase. The pull request now contains 12 commits:
- Merge branch 'master' into shuffle-data-reborn
- Fix merge
- Merge tag 'jdk-19+13' into shuffle-data-reborn
Added tag jdk-19+13 for changeset 5df2a057
- Move characterdata templates to share/classes/java/lang.
- Update comment refering to "make" dir
- Move new symbols to jdk.compiler
- Merge branch 'master' into shuffle-data
- Move macosxicons from share to macosx
- Move to share/data, and move jdwp.spec to java.se
- Update references in test
- ... and 2 more: https://git.openjdk.java.net/jdk/compare/83d77186...598f740f
I have carefully reviewed all PR comments, and the changes I made in response
to them. I believe I have resolved all requests from reviewers. What remained
to do was to create an informational JEP about the new source structure, and
file some follow-up issues.
I have now created and submitted a new informational JEP ("JDK Source
Structure"), available at https://bugs.openjdk.java.net/browse/JDK-8283227. When
creating this JEP, it felt increasingly silly to just copy and extend the part about
src/$MODULE from JEP 201, so I extended it to cover a relevant overview of the entire JDK
source base structure. I actually think this JEP has a good merit on its own,
notwithstanding it being a reviewer requirement for this PR.
I have also filed follow up issues for the non-standard jdk.hotspot.agent `doc`
and `test` directories (https://bugs.openjdk.java.net/browse/JDK-8283197 and
https://bugs.openjdk.java.net/browse/JDK-8283198, respectively).
I have filed a follow up issue for continued efforts to clean up
charsetmapping, https://bugs.openjdk.java.net/browse/JDK-8283228.
There were two open questions:
* should jdwp.spec belong to specs directory instead of data
* should bin/idea.sh be changed to exclude data
but they sounded so exploratory that I decided not to open JBS issues for them.
@wangweij @naotoj @prrace @erikj79 @jonathan-gibbons You have all approved
this PR at an older revision. Can you please reconfirm that your approval
stands for the latest revision? (Sorry for the mass ping)
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: JDK-8282686: Add constructors taking a cause to SocketException [v3]
On Mon, 7 Mar 2022 17:55:45 GMT, Joe Darcy wrote:
>> Please review this small API enhancement to add the usual constructors
>> taking a cause to SocketException and then update uses of initiCause on
>> creating SocketException to instead pass the cause via the constructor.
>>
>> Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282688
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Improve test.
Hmm, this seems to have been integrated without any Reviewer on the final
commit. How did that happen?
test/jdk/java/net/SocketException/TestSocketExceptionCtor.java line 32:
> 30: import java.util.Objects;
> 31:
> 32: public class TestSocketExceptionCtor {
I don't think this is a good name for the test because it tests mores than the
constructors. So I think drop the suffix from the name.
-
PR: https://git.openjdk.java.net/jdk/pull/7705
Re: RFR: JDK-8282686: Add constructors taking a cause to SocketException
On Fri, 4 Mar 2022 21:17:01 GMT, Joe Darcy wrote: > Please review this small API enhancement to add the usual constructors taking > a cause to SocketException and then update uses of initiCause on creating > SocketException to instead pass the cause via the constructor. > > Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282688 This looks good, I just wonder if we should add a test for the new constructors. One of them will be tested by the usages in the JDK, the cause-only constructor may not be exercised by any code. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7705
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v8]
On Tue, 22 Feb 2022 22:05:32 GMT, Lance Andersen wrote: >> Hi all, >> >> Please review the attached patch to address >> >> - That JarFile::getInputStream did not check for a null ZipEntry passed as a >> parameter >> - Have Zip/JarFile::getInputStream throw a ZipException in the event that an >> unexpected exception occurs >> >> Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar >> test runs >> >> Best >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Modified and clarified test comments Thanks for the update to the test to provide instructions on how to re-create the JAR and sign it. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v4]
On Fri, 18 Feb 2022 19:22:27 GMT, Lance Andersen wrote: > Ok, thank you for the feedback. I just pushed a change with additional > comments on the jar creation which hopefully will address your input above. It's a bit better but I think it needs a clear step-by-step instructions in a comment before declaration of VALID_ENTRY_NAME to show how the JAR file is created, signed (move the instructions that have been placed on the TestNG setup method), and then converted to the byte array. Further maintainers will thank you. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v4]
On Fri, 18 Feb 2022 17:15:17 GMT, Lance Andersen wrote: > If you feel there is still something lacking for documentation, I can > certainly make another pass clarify/add it, but I tried to cover the steps > (but I also understand what might be obvious to me might not be as obvious as > I thought). Yes, I think clear instructions are important to have for tests like this. It doesn't need much but what you know is scattered across a method and a comment on a byte array, just not clear/easy. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v4]
On Fri, 18 Feb 2022 16:25:30 GMT, Lance Andersen wrote: > > The updates changes to ZipFile/JarFile look okay. I don't have time to > > study the test too closely right now but it will need to include > > instructions on how to re-create the signed JAR that is stored in the byte > > array. > > Those instructions are already in the comments for the constant > `SIGNED_VALID_ENTRY_NAME` That's the keytool command to sign the JAR. What I meant is the complete steps to create the JAR file, sign it, and then create the byte array. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v4]
On Thu, 17 Feb 2022 19:00:47 GMT, Lance Andersen wrote: >> Hi all, >> >> Please review the attached patch to address >> >> - That JarFile::getInputStream did not check for a null ZipEntry passed as a >> parameter >> - Have Zip/JarFile::getInputStream throw a ZipException in the event that an >> unexpected exception occurs >> >> Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar >> test runs >> >> Best >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Return null when ZipEntry::getName is null The updates changes to ZipFile/JarFile look okay. I don't have time to study the test too closely right now but it will need to include instructions on how to re-create the signed JAR that is stored in the byte array. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v3]
On Thu, 10 Feb 2022 21:35:56 GMT, Lance Andersen wrote:
>> Hi all,
>>
>> Please review the attached patch to address
>>
>> - That JarFile::getInputStream did not check for a null ZipEntry passed as a
>> parameter
>> - Have Zip/JarFile::getInputStream throw a ZipException in the event that an
>> unexpected exception occurs
>>
>> Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar
>> test runs
>>
>> Best
>> Lance
>
> Lance Andersen has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Return a null InputStream when the ZipEntry is not found in the Jar
> - Address formatting and message feedback
src/java.base/share/classes/java/util/jar/JarFile.java line 881:
> 879: ze = getJarEntry(entryName);
> 880: } else {
> 881: throw new ZipException("ZipEntry::getName returned null");
Throwing ZipException when ZipEntry::getName returns null is still surprising
but not terrible. The spec for getInputStream specifies ZipException for when
a zip file format occurs so we might have to extend that to add "or the zip
entry name is null".
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Tue, 8 Feb 2022 18:11:38 GMT, Lance Andersen wrote: > I personally think it is best to continue throw the NPE as that provides > symmetry with ZipFile::getInputStream, aligns with the current javadoc where > a null parameter will throw an NPE unless specified elsewhere, there are > existing tests which check for an NPE if JarFile::getInpuStream(null) is > called. I think the scenario that we are discussing is where the parameter is not null. It's the case where getInputStream(ZipEntry) is called with a ZipEntry that reports its name as null. I don't think it is covered by existing javadoc. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Tue, 8 Feb 2022 15:27:46 GMT, Sean Mullan wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Reduce Exception checking to JarFile::verifiableEntry
>
> src/java.base/share/classes/java/util/jar/JarFile.java line 871:
>
>> 869: }
>> 870: // ZipEntry::getName should not return null
>> 871: if(ze.getName() != null) {
>
> Nit, add space after "if"
if ZipEntry is extended and getName() overridden then you can't trust the name.
So I think you'll have extract the name rather than calling ZipEntry::getName
twice. I'm almost tempted to have getInputStream(ZipEntry) be re-specified to
throw IAE if the zip entry is null.
> src/java.base/share/classes/java/util/jar/JarFile.java line 877:
>
>> 875: }
>> 876: // ZipEntry returned from JarFile::getJarEntry should not be
>> null
>> 877: if(ze == null) {
>
> Nit, add space after "if"
ze can't be null here.
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName()
On Fri, 4 Feb 2022 12:42:39 GMT, Lance Andersen wrote:
> Hi all,
>
> Please review the attached patch to address
>
> - That JarFile::getInputStream did not check for a null ZipEntry passed as a
> parameter
> - Have Zip/JarFile::getInputStream throw a ZipException in the event that an
> unexpected exception occurs
>
> Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar
> test runs
>
> Best
> Lance
src/java.base/share/classes/java/util/jar/JarFile.java line 840:
> 838: throws IOException
> 839: {
> 840: Objects.requireNonNull(ze, "ze");
Is the NPE specified?
src/java.base/share/classes/java/util/jar/JarFile.java line 866:
> 864: } catch (Exception e2) {
> 865: // Any other Exception should be a ZipException
> 866: throw (ZipException) new ZipException("Zip file format
> error").initCause(e2);
If there is ZIP format error then I would expect ZipException or the more
general IOException is already thrown. So I think this is catching other cases,
maybe broken manifests or signed JAR files, in which case a JarException may be
better.
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: JDK-8281082: Improve javadoc references to JOSS [v2]
On Tue, 1 Feb 2022 22:33:46 GMT, Joe Darcy wrote:
>> The references to JOSS, the Java Object Serialization Specification, are not
>> done consistently in the API javadoc. This should be improved.
>>
>> I'll update copyright years before pushing.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update copyrights.
src/java.base/share/classes/java/io/ObjectOutputStream.java line 659:
> 657: * customize the way in which class descriptors are written to the
> 658: * serialization stream. The corresponding method in
> ObjectInputStream,
> 659: * {@link ObjectInputStream#readClassDescriptor
> readClassDescriptor}, should then be overridden to
This makes the line lengths in this paragraph a bit inconsistent, maybe it can
be re-formatted so that this one line doesn't stick out so much.
-
PR: https://git.openjdk.java.net/jdk/pull/7315
Re: [jdk18] RFR: 8279529: ProblemList java/nio/channels/DatagramChannel/ManySourcesAndTargets.java on macosx-aarch64
On Wed, 5 Jan 2022 17:22:54 GMT, Daniel D. Daugherty wrote: > A couple of trivial ProblemListings: > > JDK-8279529 ProblemList > java/nio/channels/DatagramChannel/ManySourcesAndTargets.java on macosx-aarch64 > JDK-8279532 ProblemList > sun/security/ssl/SSLSessionImpl/NoInvalidateSocketException.java This looks okay although I think we're working around a macOS or configuration issue rather than a JDK or test issue. - PR: https://git.openjdk.java.net/jdk18/pull/83
Re: [jdk18] RFR: 8279222: Incorrect legacyMap.get in java.security.Provider after JDK-8276660 [v3]
On Tue, 4 Jan 2022 06:59:58 GMT, Aleksey Shipilev wrote: >> SonarCloud reports: >> A "Map" cannot contain a "String" in a "ServiceKey" >> type. >> >> >> // clean up old alias if present >> Service prevAliasService = legacyMap.get(aliasAlg); >> >> >> Should be `aliasKey`, like other accesses to `legacyMap`. This code is >> introduced by >> [JDK-8276660](https://bugs.openjdk.java.net/browse/JDK-8276660), so it >> affects JDK 18. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug `jdk_security` > > Aleksey Shipilev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Whitespace > - Metadata updates > - Merge branch 'master' into JDK-8279222-jsp-get > - Test addition by Valerie > - Merge branch 'master' into JDK-8279222-jsp-get > - Fix Thanks for coordinating on this to ensure the change has a test. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk18/pull/70
Re: [jdk18] RFR: 8279222: Incorrect legacyMap.get in java.security.Provider after JDK-8276660
On Thu, 23 Dec 2021 13:33:26 GMT, Aleksey Shipilev wrote: > SonarCloud reports: > A "Map" cannot contain a "String" in a "ServiceKey" > type. > > > // clean up old alias if present > Service prevAliasService = legacyMap.get(aliasAlg); > > > Should be `aliasKey`, like other accesses to `legacyMap`. This code is > introduced by > [JDK-8276660](https://bugs.openjdk.java.net/browse/JDK-8276660), so it > affects JDK 18. > > Additional testing: > - [x] Linux x86_64 fastdebug `jdk_security` src/java.base/share/classes/java/security/Provider.java line 1152: > 1150: case ADD: > 1151: // clean up old alias if present > 1152: Service prevAliasService = legacyMap.get(aliasKey); The change looks okay but I'm surprised this wasn't caught by tests. @valeriepeng - would it be feasible to have a test that removes an old alias so that this code is exercised by tests? - PR: https://git.openjdk.java.net/jdk18/pull/70
Re: [jdk18] RFR: JDK-8278967 rmiregistry fails to start because SecurityManager is disabled [v2]
On Wed, 22 Dec 2021 01:18:58 GMT, Stuart Marks wrote: >> Enable the security manager in rmiregistry's launcher arguments. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Change java.security.manager to "allow"; filter warning lines from > VersionCheck This version looks okay, avoids having an attempt to set the SM in createRegistry always be skipped. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk18/pull/45
Re: [jdk18] RFR: JDK-8278967 rmiregistry fails to start because SecurityManager is disabled
On Fri, 17 Dec 2021 20:01:27 GMT, Stuart Marks wrote: > Enable the security manager in rmiregistry's launcher arguments. As things stand, `rmiregsitry -J-Djava.security.manager` and `rmiregistry -J-Djava.security.manager=allow` are equivalent because rmiregistry sets the default SM. Some difference may be observed if someone is running rmiregistry with a custom system class loader set, or custom file system provider, or running it with a JVM TI agent that is looking at events between VMStart and VMInit but these seem somewhat obscure scenarios for a command line program If rmiregstry worked with ToolProvider then I think there would be more to discuss. If the final patch is to have the launcher set the default security manager then we may want to change RegistryImpl.createRegistry to fail if not set. The warning that is emitted for both cases is expected. JEP 411 is very clear that it there is no mechanism to suppress it. We may need to add a release note to document that rmiregistry will emit a warning a startup, that will at least be something to point at in the event that anyone asks or reports an issue. - PR: https://git.openjdk.java.net/jdk18/pull/45
Re: RFR: 8274809: Update java.base classes to use try-with-resources [v4]
On Wed, 6 Oct 2021 20:37:29 GMT, Andrey Turbanov wrote:
>> 8274809: Update java.base classes to use try-with-resources
>
> Andrey Turbanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8274809: Update java.base classes to use try-with-resources
> fix review comments
Marked as reviewed by alanb (Reviewer).
src/java.base/share/classes/sun/net/NetProperties.java line 73:
> 71: try (FileInputStream in = new FileInputStream(fname);
> 72: BufferedInputStream bin = new BufferedInputStream(in))
> 73: {
Style-wide I'd probably put the "{" at the end of L72.
-
PR: https://git.openjdk.java.net/jdk/pull/5818
Re: RFR: 8277932: Subject:callAs() not throwing NPE when action is null
On Mon, 6 Dec 2021 22:22:14 GMT, Weijun Wang wrote: > Add null check. I must have thought the NPE will be thrown anyway but the > `catch Exception` block swallows it. > > I added a noreg-trivial label. If you think differently can add one. Is there a test for this? (I see noreg-trivial is added but a test should be easy to add). - PR: https://git.openjdk.java.net/jdk/pull/6728
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Looks good.
-
Marked as reviewed by alanb (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v13]
On Tue, 2 Nov 2021 19:35:29 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Address impl review comments
> - Address API review comments
src/java.base/share/classes/java/lang/Module.java line 114:
> 112:
> 113: // true, if this module allows restricted native access; @Stable
> makes sure that modules that allow native
> 114: // access capture this property as a constant.
Do you mind fixing this comment to avoid the really long line, it sticks out
compare to everything else around it.
src/java.base/share/classes/sun/nio/ch/IOUtil.java line 478:
> 476: private static final JavaNioAccess NIO_ACCESS =
> SharedSecrets.getJavaNioAccess();
> 477:
> 478: static Runnable acquireScope(ByteBuffer bb, boolean async) {
At some point (not this PR) we should move the "async" out of this file, IOUtil
was for synchronous I/O.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 17:13:47 GMT, Roger Riggs wrote: > In comments, I think the 'synchronized static 'reads better, the conventional > order is for the code. I think Roger is right and maybe the change to the javadoc could be dropped from this patch. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: previously prevented exploit now possible with JDK 18
On 28/10/2021 20:14, Rick Hillegas wrote: As a canary in the mineshaft, I built and tested Apache Derby with the recent build 18-ea+20-1248 of Open JDK 18. I tripped across the following issue when running Derby's regression tests. The problem is explained in more detail at https://issues.apache.org/jira/browse/DERBY-7126, where a simple repro (DERBY_7126_A) can be found. The problem is almost surely the result of work done on https://bugs.openjdk.java.net/browse/JDK-8269039 (Disable SHA-1 Signed JARs). Under previous versions of the JDK, the JVM would raise an error if you tried to load a class from a jar file which had been signed with SHA-1 but later hacked by inserting malware via "jar -uf". This was the error: SHA1 digest error for $corruptedJarFileName However, under JDK 18 the hacked class loads, no error is raised, and the malware can now be executed. I was surprised that a previously prevented exploit now works. I think it would be better if the JVM still refused to load the hacked class even though SHA-1 has been deprecated. As I understand it, if the JAR file was signed with SHA-1 then it is now treated as unsigned. Are you saying that unsigned JARs are trusted in the environment? -Alan
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v8]
On Tue, 26 Oct 2021 12:52:58 GMT, Aleksei Efimov wrote: >> This change implements a new service provider interface for host name and >> address resolution, so that java.net.InetAddress API can make use of >> resolvers other than the platform's built-in resolver. >> >> The following API classes are added to `java.net.spi` package to facilitate >> this: >> - `InetAddressResolverProvider` - abstract class defining a service, and >> is, essentially, a factory for `InetAddressResolver` resolvers. >> - `InetAddressResolverProvider.Configuration ` - an interface describing the >> platform's built-in configuration for resolution operations that could be >> used to bootstrap a resolver construction, or to implement partial >> delegation of lookup operations. >> - `InetAddressResolver` - an interface that defines methods for the >> fundamental forward and reverse lookup operations. >> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the >> characteristics of one forward lookup operation. >> >> More details in [JEP-418](https://openjdk.java.net/jeps/418). >> >> Testing: new and existing `tier1:tier3` tests > > Aleksei Efimov has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 14 additional > commits since the last revision: > > - Changes to address review comments > - Update tests to address SM deprecation > - Merge branch 'master' into JDK-8244202_JEP418_impl > - More javadoc updates to API classes > - Review updates + move resolver docs to the provider class (CSR update to > follow) > - Change InetAddressResolver method names > - Remove no longer used import from IPSupport > - Rename IPSupport.hasAddress and update it to use SocketChannel.open > - Address review comments: javadoc + code cleanup > - Address resolver bootstraping issue > - ... and 4 more: > https://git.openjdk.java.net/jdk/compare/c2db4e04...1378686b Thanks for the updates to the API docs, you've addressed all my comments. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v7]
On Fri, 22 Oct 2021 14:27:41 GMT, Aleksei Efimov wrote:
>> This change implements a new service provider interface for host name and
>> address resolution, so that java.net.InetAddress API can make use of
>> resolvers other than the platform's built-in resolver.
>>
>> The following API classes are added to `java.net.spi` package to facilitate
>> this:
>> - `InetAddressResolverProvider` - abstract class defining a service, and
>> is, essentially, a factory for `InetAddressResolver` resolvers.
>> - `InetAddressResolverProvider.Configuration ` - an interface describing the
>> platform's built-in configuration for resolution operations that could be
>> used to bootstrap a resolver construction, or to implement partial
>> delegation of lookup operations.
>> - `InetAddressResolver` - an interface that defines methods for the
>> fundamental forward and reverse lookup operations.
>> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the
>> characteristics of one forward lookup operation.
>>
>> More details in [JEP-418](https://openjdk.java.net/jeps/418).
>>
>> Testing: new and existing `tier1:tier3` tests
>
> Aleksei Efimov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> More javadoc updates to API classes
I've done another pass over the API. Overall it's looking very good and my
comments are just to improve the javadoc in a few places to make it clearer.
src/java.base/share/classes/java/net/InetAddress.java line 169:
> 167: * Access Protocol (LDAP).
> 168: * The particular naming services that the built-in resolver uses by
> default
> 169: * depend on the configuration of the local machine.
depend -> depends
src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 38:
> 36: * deployed as a href="InetAddressResolverProvider.html#system-wide-resolver">
> 37: * system-wide resolver. {@link InetAddress} delegates all lookup
> requests to
> 38: * the deployed system-wide resolver instance.
I think the class description would be a bit clearer if we drop sentence 2
because it overlaps with paragraph 2. I think we can adjust sentence 3 to
"InetAddress delegates all lookup operations to the system-wide resolver" and
it will all flow nicely.
src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 88:
> 86: * to a valid IP address length
> 87: */
> 88: String lookupByAddress(byte[] addr) throws UnknownHostException;
I assume this throws NPE if addr is null.
src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 137:
> 135: /**
> 136: * This factory method creates {@link LookupPolicy LookupPolicy}
> instance with a provided
> 137: * {@code characteristics} value.
This should be "This factory method creates a LookupPolicy ...".
src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java line
45:
> 43: * system-wide resolver instance, which is used by
> 44: * href="{@docRoot}/java.base/java/net/InetAddress.html#host-name-resolution">
> 45: * InetAddress.
I think we should prune some some of the text from the class description to
avoid repetition. Here's a suggestion:
1. Add the following immediately after the sentence "A given innovation ..."
"It is set after the VM is fully initialized and when an invocation of a method
in InetAddress class triggers the first lookup operation.
2. Replace the text in L47-65 with:
"A resolver provider is located and loaded by InetAddress to create the
systwm-wide resolver as follows:
3. Replace the remaining three usages of "install" with "set".
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v5]
On Wed, 20 Oct 2021 18:25:24 GMT, Alan Bateman wrote:
>> Aleksei Efimov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Change InetAddressResolver method names
>
> src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java
> line 52:
>
>> 50: /**
>> 51: * Initialise and return the {@link InetAddressResolver} provided by
>> 52: * this provider. This method is called by {@link InetAddress} when
>
> "the InetAddressResolver" suggests there is just one instance. That is
> probably the case but I don't think you want to be restricted to that.
Initialise -> Initialize to be consistent with other usages that use US
spelling.
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v5]
On Wed, 20 Oct 2021 11:52:38 GMT, Aleksei Efimov wrote: >> This change implements a new service provider interface for host name and >> address resolution, so that java.net.InetAddress API can make use of >> resolvers other than the platform's built-in resolver. >> >> The following API classes are added to `java.net.spi` package to facilitate >> this: >> - `InetAddressResolverProvider` - abstract class defining a service, and >> is, essentially, a factory for `InetAddressResolver` resolvers. >> - `InetAddressResolverProvider.Configuration ` - an interface describing the >> platform's built-in configuration for resolution operations that could be >> used to bootstrap a resolver construction, or to implement partial >> delegation of lookup operations. >> - `InetAddressResolver` - an interface that defines methods for the >> fundamental forward and reverse lookup operations. >> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the >> characteristics of one forward lookup operation. >> >> More details in [JEP-418](https://openjdk.java.net/jeps/418). >> >> Testing: new and existing `tier1:tier3` tests > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Change InetAddressResolver method names src/java.base/share/classes/java/net/InetAddress.java line 244: > 242: * @implNote > 243: * For any lookup operation that might occur before the VM is fully > booted the built-in > 244: * resolver will be used. I think we will need decide if InetAddress class description is the right place for this or whether some/most of it should move to InetAddressResolverProvider. It might be that we update the existing "Host Name Resolution" section with some basic/readable text to make the reader aware that there is a provider mechanism with a link to InetAddressResolverProvider with the details. - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v5]
On Wed, 20 Oct 2021 11:52:38 GMT, Aleksei Efimov wrote:
>> This change implements a new service provider interface for host name and
>> address resolution, so that java.net.InetAddress API can make use of
>> resolvers other than the platform's built-in resolver.
>>
>> The following API classes are added to `java.net.spi` package to facilitate
>> this:
>> - `InetAddressResolverProvider` - abstract class defining a service, and
>> is, essentially, a factory for `InetAddressResolver` resolvers.
>> - `InetAddressResolverProvider.Configuration ` - an interface describing the
>> platform's built-in configuration for resolution operations that could be
>> used to bootstrap a resolver construction, or to implement partial
>> delegation of lookup operations.
>> - `InetAddressResolver` - an interface that defines methods for the
>> fundamental forward and reverse lookup operations.
>> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the
>> characteristics of one forward lookup operation.
>>
>> More details in [JEP-418](https://openjdk.java.net/jeps/418).
>>
>> Testing: new and existing `tier1:tier3` tests
>
> Aleksei Efimov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Change InetAddressResolver method names
src/java.base/share/classes/java/net/InetAddress.java line 340:
> 338:
> 339: /* Used to store the system-wide resolver */
> 340: private static volatile InetAddressResolver resolver;
Can this be @Stable?
src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 34:
> 32:
> 33: /**
> 34: * This interface defines operations for looking-up host names and IP
> addresses.
I think change "looking-up" to "looking up".
src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 102:
> 100:
> 101: /**
> 102: * Specifies if IPv4 addresses need to be queried during lookup.
It might be better to change this to "Characteristic value signifying ..."
rather than "Specifies".
src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 186:
> 184:
> 185: /**
> 186: * Returns an integer value which specifies lookup operation
> characteristics.
I think I'd change the first line here to something like "Returns a set of
characteristics of this lookup policy".
src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java line
35:
> 33: /**
> 34: * A resolver provider class is a factory for custom implementations of
> {@linkplain
> 35: * InetAddressResolver resolvers} which define operations for looking-up
> (resolving) host names
This reads "custom implementations of resolvers". It might be better to start
with "Service-provider class for InetAddress resolvers".
src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java line
52:
> 50: /**
> 51: * Initialise and return the {@link InetAddressResolver} provided by
> 52: * this provider. This method is called by {@link InetAddress} when
"the InetAddressResolver" suggests there is just one instance. That is probably
the case but I don't think you want to be restricted to that.
src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java line
88:
> 86: * {@code
> RuntimePermission("inetAddressResolverProvider")}.
> 87: * @implNote It is recommended that an {@code
> InetAddressResolverProvider} service
> 88: * implementation does not perform any heavy initialization in its
"heavy initialization" is probably not the best phrase here. Maybe it should
say that the initialisation should be as simple as possible to avoid recursive
initialisation issues.
src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java line
110:
> 108: /**
> 109: * A {@code Configuration} interface is supplied to the
> 110: * {@link InetAddressResolverProvider#get(Configuration)} method
> when installing a
I think this should be "Configuration object" rather than "interface" here.
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Sun, 17 Oct 2021 15:55:59 GMT, Aleksei Efimov wrote: > * `InetAddressResolverProvider.Configuration` interface API might evolve, > e.g. there might be a need to pass additional configuration items. > * local hostname is a dynamic information, therefore it cannot be passed as > string parameter to `InetAddressResolverProvider::get`. Right, I think we've ended up with a good model here that supports the common case, stacking, and can be extended in the future to add new configuration if required. Also if good use-cases come up when Configuration can be changed to be non-sealed. - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 15 Sep 2021 01:45:49 GMT, wxiang wrote: > Yes, I will take care of it. Need I create a new PR? Up to you, it's okay to continue with this one if you want, we would just need to change the description here and in JBS. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:30:34 GMT, wxiang wrote: >> wxiang has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Some minor changes >> >> Include: >> 1. remove public for INDEX_NAME in JarFile >> 2. recover the definition for INDEX_NAME in JarIndex >> 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar > > Create CSR: https://bugs.openjdk.java.net/browse/JDK-8273473 @shiyuexw I discussed this issue with other maintainers in this area. There was agreement on dropping the support from the URLClassLoader implementation but it was suggested that it should be disabled for a release or two before the code is removed. A system property can be used to re-enable. I've updated the CSR to reflect this proposal. Are you okay to continue with this new proposal? It would mean that we wouldn't remove the tests but instead change them to run with the system property. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:30:34 GMT, wxiang wrote: > Create CSR: https://bugs.openjdk.java.net/browse/JDK-8273473 Thanks. I've updated the CSR to make it clearer what this change is about. There is still some TDB for the JAR file spec. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath
On Tue, 7 Sep 2021 03:34:27 GMT, wxiang
wrote:
> There is a bug for URLClassPath.findResources with JarIndex.
> With some discussions about the bug, the current priority is to remove the
> JAR index support in URLClassPath,
> and don’t need to do anything to the jar tool in the short term, except just
> to move JarIndex to the jdk.jartool module.
>
> The PR includes:
> 1. remove the JarIndex support in URLClassPath
> 2. move JarIndex into jdk.jartool module.
Thanks for taking this on. I've done a pass over the changes and it looks
complete, just a few issues. Next step will be to create a CSR for this change.
src/java.base/share/classes/java/util/jar/JarFile.java line 220:
> 218: * The index file name.
> 219: */
> 220: public static final String INDEX_NAME = "META-INF/INDEX.LIST";
Adding this as a public field means it becomes part of the API, so it shouldn't
be public here.
src/java.base/share/classes/java/util/jar/JarVerifier.java line 147:
> 145:
> 146: if (uname.equals(JarFile.MANIFEST_NAME) ||
> 147: uname.equals(JarFile.INDEX_NAME)) {
It would be useful if someone from security-libs could comment on this. The
interaction between signed JAR and JAR index isn't very clear. The change you
have is safe but it might be that we can drop the checking for INDEX.LIST here.
test/jdk/sun/misc/JarIndex/JarIndexMergeTest.java line 44:
> 42: import sun.tools.jar.JarIndex;
> 43:
> 44: public class JarIndexMergeTest {
Is this the one remaining test in sun/misc/JarIndex? I think it can be moved to
test/jdk/java/util/jar. I think we should change the package name in the
summary comment too.
-
PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging
On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov wrote: > The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" > via an AppContext to support "applet logging isolation". The AppContext class > became useless since the plugin and webstart are no longer supported and > removed in jdk11. > > This is the request to delete the usage of AppContext in the LogManager and > related tests. > > Tested by tier1/tier2/tier3 and jdk_desktop tests. No objection to removing this legacy/unused code but I think it would be useful to useful to have the JBS issue or the PR summary provide a bit more context. As I see it, this is just one piece of the overall cleanup and I assume there are more substantive changes to the java.desktop module coming, is that right? - PR: https://git.openjdk.java.net/jdk/pull/5326
Re: System.exit(...) without SecurityManager - implications for Apache Ant project
On 23/08/2021 07:46, Jaikiran Pai wrote: : So if any of the upcoming Java 18 EA builds introduce the "disallow" by default, then if Ant has to test against those EA builds (not just for SecurityManager but any other changes), then Ant project will have to start setting the "java.security.manager" system property to "allow". That's not going to be straightforward, since it would be dealing with this in potentially multiple launch scripts or maybe even dynamic launch commands issued through the code (I haven't checked where/how many). I don't yet know if this property value can be changed at runtime/dynamically. If that is allowed, it may make things a bit easier since that would allow us to set that property value at the same place where we set the custom security manager in the Ant code, but I would still like to avoid it if possible. So given all this, can the new API(s) design/implementation for preventing System.exit(...) calls without a SecurityManager be prioritized and perhaps be scheduled for release in Java 18 itself, which is where the "disallow" will come in as default? To summarize - Ant would need a way to intercept/prevent JVM exits via System.exit(...) or Runtime.getRuntime().exit(...) calls *and* a way to get hold of the exit code that was used to trigger such calls (Ant had to wrap the SecurityException into a custom exception type just to keep track of that exit code value). Once the JDK is changed to disallow setting a SM dynamically then it will require the command line option to keep existing code that calls System.setSM working. This change should have little-to-no impact on libraries as it would be rare for a library to set a SM. It will have impact on applications and tools that do set it dynamically as they will need the CLI option to allow a SM be set. The System.exit use-case is well studied. It has come up several times over the years in the context of tools that want to prevent accidental calls to System.exit by plugins/other programs that are run "in process". Your description of the fork mode where it runs the task "in process" seems to match this. I assume one option is to just catch UOE or not attempt to set a SM on >= 18, it just means that tools that do call System.exit will cause ant to shutdown with maybe a shutdown hook used to log a message. It's too early to point to a specific API/proposal at this time. There were early exploratory sketches a few years ago but they didn't go very far. The main thing is that we don't want to introduce APIs for intercepting specific operations in isolation, e.g. we don't want one API to intercept System.exit and a completely different API for intercepting attempts to make socket connections for example. So TBD, and I'm sure there will be a lot of discussion on the mailing lists. -Alan
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On 23/08/2021 05:45, David Holmes wrote: : @wangweij there are many tests that can call setSecurityManager() and will presumably need to be fixed before this switch can be applied. And all testing will need to be updated to require jtreg 6.1 (which no longer uses the SM) once it is released. Most of the preparation work for the tests was done via JDK-8267184 in JDK 17. JDK-8270380 isn't going to be integrated until after the upgrade the jtreg 6.1 (which I think is one part of Jai's mail). -Alan.
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.
src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java line 342:
> 340:
> 341: try {
> 342: for (String line : Files.readAllLines(statusPath, UTF_8)) {
The 1-arg readAllLines is specified to use UTF-8 so you can drop the second
parameter here if you want.
-
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 11:23:54 GMT, Weijun Wang wrote:
>> 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".
I think I agree with Lance that "Throws UOE" would be clearer in this table.
-
PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8272626: Avoid C-style array declarations in java.*
On Wed, 18 Aug 2021 10:07:35 GMT, Claes Redestad wrote: > C-style array declarations generate noisy warnings in IDEs, et.c. This patch > cleans up all java.* packages. > > (Copyrights intentionally not updated due the triviality of most changes) Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5161
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> 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.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
Marked as reviewed by alanb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 18:41:04 GMT, Claes Redestad wrote: > Yes, while I don't know exactly which changes resolved JDK-6764325, it's > clear from the microbenchmarks added for #2102 that it's no longer an issue - > at least not in the mainline. Thanks for checking and for closing that issue. - PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> 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.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
It would be useful to get up to date performance data on using Charset vs.
charset name. At least historically, the charset name versions were faster (see
[JDK-6764325](https://bugs.openjdk.java.net/browse/JDK-6764325)).
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: JEP 411, removal of finalizers, a path forward.
On 01/08/2021 15:28, Uwe Schindler wrote: : What I figured out: You intend to remove SecurityManager because it does not fit your latest ideas how Java threads should behave. I know the main problem is not "SecurityManager is too complex / too slow / wrongly used /..." -- the main problem of some OpenJDK people around the Loom project is that it won't work correctly with those new type of threads. You are now always arguing against use cases of SecurityManager for the purpose of secuirty because you just want to hide that the new "light" threads (aka fibers) in project Loom are incompatible to the stack-based access control provided by AccessController and SecurityManager. So the only canard is Project Loom - sorry! This isn't right, I don't know where you got that. The only connection to threads is the unspecified capturing of an access control context at Thread create time. Loom has been betting that it will be irrelevant and eventually removed so doesn't capture it. For the interim it just specifies that virtual threads have "no permissions". : - Allow to hook into the I/O system. Unfortunately the java.nio FileSystem API is not enough: it does not cover java.io.File (why is this the case?) nor does the FileSystem API allows to hook in everywhere. We figured out that for example the new Panama interface to get a MemorySegment from a file path is not calling any API in the Filesystem abstraction! There are bootstrapping and compatibility issues, this isn't the right place to go into all that. We have seen this in Java 9 already: Suddenly the module system was weakened shortly before release: because there was no replacement for sun.misc.Unsafe. This isn't right either. Critical internal APIs, including sun.misc.Unsafe, were never encapsulated. So no change to the accessibility of Unsafe when relaxed strong encapsulated was introduced in Java 9. -Alan
Re: JEP 411, removal of finalizers, a path forward.
On 31/07/2021 04:04, Peter Firmstone wrote: Allan has advised when finalizers are removed, it will be practical to use Agents to instrument public API to implement an authorization layer, this is try, so can it be coordinated with JEP 411 et al? Our exchange was about instrumenting constructors that specify SM permission checks and where the classes that define these constructors have been hardened to thwart finalizer attacks. It wasn't a comment on the bigger question on how practical it is to use instrumented the entire JDK. Once you get further on then I assume a big challenge will be with APIs that separate the interface and implementation (think factory methods, APIs that define service provider interfaces ...). Here I expect you will want to instrument the implementation classes. Going deeper, you may find places where the SM check isn't on method entry but instead after defensive copying of mutable parameters or after acquiring a lock that prevents mutation while do a security sensitive operations. So non-trivial but a fun approach to explore. If you have the cycles then pick a version and try it. That will give you a sense on how much effort may be required to keep up and be confident that every interesting code path is covered. -Alan
Re: JEP 411 Headaches: Instrumenting private methods in the JDK for authorization checkpoints.
On 23/07/2021 23:33, Peter Firmstone wrote: I think it's worth noting that there isn't a way to securely run code with malicious intent now, so I'm surprised that at this late stage you were still providing support for sand boxing (whack a mole). It's just for us many assumptions have been made on a Java platform with SM, using POLP (not sandboxing) as this was one of the foundational principles of secure coding guidelines (just like following concurrency best practice, were were following security best practice). Sandboxing is an all or nothing approach, if you had a trusted applet that was signed, it had AllPermission, if you had an unsigned applet, then it had no permissions. Sandboxing was one of the use cases for SM, when combined with ClassLoader visibility, but we never realized that OpenJDK developers meant sandboxing == authorization access controls. When you remove that pillar, everything it's supporting collapses, not just sand boxing, so when you say you are removing support for sandboxing, we say, good idea, but we didn't realize you were saying you were removing support for all authorization access controls. Reduced and revised authorization and access control would have been acceptable, as tightening reflection visibility using a different form of access control removes the need for authorization based reflection access checks, but also removing atomic construction guarantee's just seems like were doing this at a rapid pace without the community understanding what you have in mind, and this may have more uses than just stopping finalizer attacks. I'm not 100% sure what you mean by "atomic construction guarantee" here. This JEP does not propose to change anything with finalization or do anything with the registration of finalizers after Object. runs. Our exchange in the previous mails was about classes (using ClassLoader as the example) that specify a SM permission check in their constructors, something that is strongly discouraged as the checks are easy to bypass. The idiom that we use in the JDK to prevent bypassing these SM permission checks with a finalizer attack is to check in a static method that returns a dummy parameter for the invokespecial. My point in the previous mail is that when the SM permission checks eventually go away then many of the uses of this idiom can do away too. That said, there is strong desire to eventually remove finalization too. Finalization was deprecated several years ago and the Java platform defines APIs that provide much more flexible and efficient ways do run cleanup actions when an object becomes unreachable. So another multi-year/multi-release effort to remove a problematic feature, just nothing to do with this JEP. As regards POLP, the focus of SM architecture when it was enhanced in Java 1.2. The JEP attempts to explain why this has been a failure. AccessController is voodoo that most developers have never encountered so anyone trying to run with SM ends up running counter to the principle of least privilege by granting all permissions. -Alan.
Re: RFR: 8268873: Unnecessary Vector usage in java.base
On Fri, 18 Jun 2021 18:28:26 GMT, Sean Mullan wrote: >> Usage of thread-safe collection `Vector` is unnecessary. It's recommended to >> use `ArrayList` if a thread-safe implementation is not needed. In >> post-BiasedLocking times, this is gets worse, as every access is >> synchronized. >> I checked only places where `Vector` was used as local variable. > > The change to `PKCS7::verify(byte[])` looks fine. @seanjmullan Are you planning to sponsor this? - PR: https://git.openjdk.java.net/jdk/pull/4482
Re: JEP 411 Headaches: Instrumenting private methods in the JDK for authorization checkpoints.
On 23/07/2021 11:48, Peter Firmstone wrote: Perhaps the solution is to replace the entire class, instead of instrumenting one method? Compile a patched copy of the JVM, with modified class files, then replace the existing classes in the JVM with the modified classes? Kinda like maintaining a fork, but using Agents to instrument the original JVM with classes from the fork? I sure wish there was a better option, if anyone knows one, I'm all ears. JEP 411 puts the JDK on the road to dropping support for sandboxing. This means there won't be a built-in means to securely run code that has malicious intent. It means that many of the concerns for finalizer attacks go away too. In the case of the ClassLoader example in your first mail then it may be that the private static method that you want to instrument will be removed. If removed, then it should make the instrumentation a bit easier so that you can instrument the protected constructors to invokestatic your equivalent of a permission check before the invokespecial. So I think this specific case is surmountable but in general I don't think it will be tenable to patch hundreds of classes and be confident that you've got everything, esp. with a moving code base and new features. I can't tell if your "authorization layer" is for use when running with code that has malicious intent. If it is, then I don't think it will be tenable to duplicate all the deeply invasive permission checks that exist today and keep it up to date as new features and changes. When agents were muted in the early discussion on JEP 411 then the context was file and network access where several people were interested in having a means to veto access. Expanding this to have a SM equivalent be able to veto every reflective access, prevent trusted method chain and other attacks, amounts to keeping the SM forever. As regards the comments about agents having the power to instrument methods that aren't accessible to user code then that is normal. Java agents are for tools to do powerful things, they aren't intended for libraries for applications to use directly. This is why agents are opt-in on the command line. Agent maintainers weld great power and must take care to never leak the Instrumentation object to applications or libraries. -Alan
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller [v2]
On Wed, 30 Jun 2021 15:45:25 GMT, Weijun Wang wrote: >> 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 Marked as reviewed by alanb (Reviewer). - 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 30/06/2021 08:19, Bernd Eckenfels wrote: Hello, sorry for being unpopular, but I just hate it to waste developer resources, I realy think this deprecation message should be re-considered, it broke a lot of things, the amount of work to implement a caching solution feels like a waste of time and on top of it, there is no clear replacement strategy published yet. I would restrict deprication (for removal if you really insist) to javadoc and not poison stdout/stderr. The purpose of the warning message is to make it very clear that applications that call System.setSecurityManager in a running VM will fail in the future. It is not hugely different to the "Illegal reflective access" warning that were emitted in JDK 9 to JDK 15. Maybe you could clarify what you mean by "it broke a lot of things". If you have something that is sensitive to warnings going to stderr then I would have expected the "Illegal reflective access" warnings to have caused a lot more issues. -Alan.
Re: Initial feedback from Apache Ant users on recent security manager warning messages
On 30/06/2021 05:51, Jaikiran Pai wrote: In the case we are dealing with, the class is always "org.apache.tools.ant.types.Permissions". It will always be loaded by one single classloader (so classloaded just once). However, multiple different instances of this class will get created during the lifetime of the build and each such instance of org.apache.tools.ant.types.Permissions can/will invoke this setSecurityManager method. If I read this correctly, the caller of setSecurityManager is code in org.apache.tools.ant.types.Permission. There may be many instances of Permissions but from the perspective of setSecurityManager then it's all the same caller Class. -Alan
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 21:18:35 GMT, Weijun Wang wrote: >> 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 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. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: Initial feedback from Apache Ant users on recent security manager warning messages
On 29/06/2021 08:04, Jaikiran Pai wrote: Thank you Alan. I see that https://bugs.openjdk.java.net/browse/JDK-8269543 has been created for this. I'll keep a watch on that one. Yes, that was the original intention but had to dropped because it wasn't thread safe. BTW: I'm puzzled as to why Ant does this. I re-read your mail yesterday a few times and it seems a real outlier to run an Ant script like this. Maybe there are use-cases beyond building where this may be have been useful in the past? (I understand you may not have all the history/rational for this). -Alan
Re: Initial feedback from Apache Ant users on recent security manager warning messages
On 28/06/2021 18:16, Jaikiran Pai wrote: On a slightly related note, I was wondering why we decided to go with what appears to be a bit more aggressive approach to these warning messages as compared to what was done with the illegal reflective access warnings? I would have thought that the illegal reflective access changes would be much more involved if not the same level as moving away from setSecurityManager calls. The typical SM setup will be to set it once, the Ant "same JVM" scenario where it sets and then resets it may be unusual. In any case, the original implementation patch did have caching to avoid duplicates. It wasn't quite right and had to be pulled, it may be time to re-visit that to avoid too much noise for code that sets it many times. -Alan.
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 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. - PR: https://git.openjdk.java.net/jdk17/pull/152
Re: Authorization layer API and low level access checks.
On 23/06/2021 04:02, Peter Firmstone wrote: Note: I'm not sure how to replace an inherited AccessControlContext (with a new implementation based on StackWalker functionality) at thread creation time, as it must be created when threads are created, possibly by using ThreadFactory everywhere, but this doesn't cover all threads. How to cater for virtual threads? I don't think the inherited AccessControlContext is widely known or even clearly specified. In any case, virtual threads do not want to be burdened with this field. For now they are specified to not support any permissions. The FJ common pool is another example, the threads don't have any permissions either (see FJP class description has more on that). -Alan
Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]
On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey wrote: >> Sufficient permissions missing if this code was ever to run with >> SecurityManager. >> >> Cleanest approach appears to be use of InnocuousThread to create the >> cleaner/poller threads. >> Test case coverage extended to cover the SecurityManager scenario. >> >> Reviewer request: @valeriepeng > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Move TokenPoller to Runnable src/java.base/share/classes/module-info.java line 202: > 200: jdk.charsets, > 201: jdk.compiler, > 202: jdk.crypto.cryptoki, At some point I'd like to see this re-visited so we can avoid exporting this package to modules defined to the platform class loader. - PR: https://git.openjdk.java.net/jdk17/pull/117
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v5]
On 18/06/2021 03:51, Jaikiran Pai wrote: : Given all that, do you think that we should be printing the jar file paths in this WARNING message by default? I read the linked CSR, but I didn't see why the location of the jar or the name of the jar would be useful in this warning message. As long as the caller class (and perhaps the caller method) is printed, I think that should be enough of a summary on what's triggering this warning. It's not hugely different to the "Illegal reflective access" warnings except I wouldn't expect many libraries to call setSecurityManager in a running system. Instead, it tends to be the main application that installs the SM at startup. However if code in a library were to call it then it useful to identify where that code is. -Alan.
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v4]
On Thu, 17 Jun 2021 14:55:02 GMT, Weijun Wang wrote: >> More loudly and precise warning messages when a security manager is either >> enabled at startup or installed at runtime. >> >> This is new PR for the `openjdk/jdk17` repo copied from >> https://github.com/openjdk/jdk/pull/4400. A new commit is added. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > add bug id into a test Implementation changes/text is okay. src/java.base/share/classes/java/lang/System.java line 330: > 328: > 329: // Remember original System.err. setSecurityManager() warning goes > here > 330: private static volatile @Stable PrintStream originalErrStream = null; I'd probably use "initial" rather than "original" here but okay. You can drop setting it to null as that is its default value. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/13
Re: blizzard of deprecation warnings related to JEP 411
On 17/06/2021 00:30, Rick Hillegas wrote:
Thanks for that advice, Alan. I have rototilled
@SuppressWarnings("removal") annotations across the Derby codebase and
thrown more memory at javadoc so that it won't crash on JDK 11. When I
run Derby's test suites, I see a blizzard of the following diagnostics:
WARNING: java.lang.System::setSecurityManager is deprecated and will
be removed in a future release.
Unfortunately, Derby still has more than 100 canon-based tests which
were developed before assertion-based testing became the norm. These
tests are run both with and without a security manager. In the latter
case, they now fail on JDK 17.
Is there a way to disable this diagnostic?
Even better: Could the diagnostic be removed in the next Open JDK
build? It could be re-introduced when Open JDK provides a replacement
for the deprecated functionality. Right now the diagnostic does not
seem to provide any actionable information.
I assume the OOME with javadoc isn't anything to do with this JEP or the
@SupressWarnings annotations, right?
There isn't any way to suppress the warning. This warning is sending a
clear message that that setSecurityManager will be removed in the
future. The "Illegal reflective access" warnings in JDK 9-15 set the
precedent.
For applications that do set a security manager then it is more likely
that they set it once, at startup, rather than setting it hundreds of
times in a running VM. Does Derby call setSecurityManager itself? At
least in the embedded case then I wouldn't expect it does as it will be
up to the application to do that if it wants. Clearly Derby has tests to
ensure that it can work with a SM so I assume its the tests that are
calling setSecurityManager. I'm not familar with the term "canon-based
tests" but I'm guessing that these are tests that run with and without
SM and are somehow sensitive to the stderr stream, is that right? There
were a small number of these in the JDK test suite too, but not many.
-Alan
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Thu, 17 Jun 2021 05:16:14 GMT, David Holmes wrote: > There are a lot more tests than just tier1. :) I don't expect many, if any, > tests to be looking for a specific IOOBE message, and I can't see an easy way > to find such tests without running them. If core-libs folk are okay with this > update then I guess we can just handle any test failures if they arise. But > I'll run this through our tier 1-3 testing. It would be good to run tier 1-3. Off hand I can't think of any tests that are dependent on the exception message, I think I'm more concerned about changing behavior (once you throw a more specific IOOBE is some of the very core classes then it's hard to change it because libraries get dependent on the more specific exception). - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Wed, 16 Jun 2021 08:08:47 GMT, Yi Yang wrote:
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex
> by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in
> the whole JDK codebase.
I looked through the changes in java.base and only spotted one case where a
different (and more specific) exception is thrown.
The changes to to files in java.util.zip lead to annoying long lines so would
be good to fix all those.
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 471:
> 469: */
> 470: public int offsetByCodePoints(int index, int codePointOffset) {
> 471: checkOffset(index, count);
String.offsetByCodePoints is specified to throw IOOBE. checkOffset may throw
the more specific StringIndexOutOfBoundsException. That's a compatible change
but I worry that we might want to throw the less specific exception in the
further because code. I only mention is because there have been cases in
java.lang where IOOBE was specified and AIOOBE by the implementation and it has
been problematic to touch the implementation as a result.
src/java.base/share/classes/java/util/Base64.java line 934:
> 932: if (closed)
> 933: throw new IOException("Stream is closed");
> 934: Preconditions.checkFromIndexSize(len, off, b.length, (xa,
> xb) -> new ArrayIndexOutOfBoundsException());
You might to split this really long line to avoid inconsistent line length in
this source file.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: blizzard of deprecation warnings related to JEP 411
On 15/06/2021 15:10, Rick Hillegas wrote:
:
When I tried to build Derby with the Rampdown Phase One build of open
JDK 17 (17-ea+26-2439), I saw many warnings related to the deprecation
of Security Manager classes and methods, undoubtedly the consequence
of JEP 411 (https://openjdk.java.net/jeps/411). Derby, like Tomcat,
embraced the Security Manager early on. Permissions checks were
rototilled across the whole code base and our distributions ship with
several template policy files, which we encourage users to customize
for their environments. The "Configuring Java Security" section of our
Security Guide explains how to do this
(https://db.apache.org/derby/docs/10.15/security/index.html).
My build only reported the first 100 warnings. It is likely that there
are many more.
Yes, JEP 411 deprecates a number of APIs for future removal. There
probably isn't much to do right now except to be aware that the APIs are
earmarked for removal in some future release. I've no doubt there will
be another JEP when that time comes. I assume you know about
@SuppressWarnings("removal"), which you can use to suppress the warnings
for now. The JDK usages of these APIs are using SuppressWarnings as the
JDK is compiled with -Xlint set to made warnings fatal.
-Alan
Re: JEP 411: Deprecation with removal would break most existing Java libraries
On 14/06/2021 08:35, Peter Firmstone wrote: I wouldn't want to see SecurityManager and Policy be neutralized, it's better to remove it and fail early so people update their software, there's a risk they may update without realizing it's no longer fully functional. Get rid of the baggage so people can start fresh with better practices. I think the context for the question is libraries that want to be able to compile to an older JDK release and work with a very wide range of JDK releases. Many libraries do not play well with a SM. They don't execute actions that require permission checks in privileged blocks and will often need to be granted AllPermission. This may have knock on impact to the components that call into these libraries, maybe they end up needing to be granted AllPermission too. Add callbacks or hand-off between threads to the picture and it can become farcical. If the library code isn't calling System.getSM or invoking AccessController.doPrivileged then it probably won't care if these APIs are degraded or removed. There are some libraries where the maintainers have put effort into working with a SM. Code in the library may use System.getSM, or doPriv or limited-doPriv. It may document the permissions that it requires and be helpful to someone assembling an application and creating its policy file. This JEP is mildly disruptive in that there will be warnings at compile-time or testing JDK 17+. If some future JDK releases degrades some of these APIs then it may be a bit more disruptive, maybe tests that try to set a SM will fail or need to be skipped. It might be that a library uses an exotic API that doesn't degrade in a sensible way and maybe that will be a bit more disruptive. Further out again, if the APIs are actually removed then it will be disruptive for libraries that want to support the possibility of being deployed with a SM on an older release. That may require some refactoring and the use of a MR-JAR as Remi mentioned. -Alan
Re: JEP 411: Deprecation with removal would break most existing Java libraries
cc'ing security-dev as that is the mailing list to use for this JEP. This JEP is the first of several in a multi-release/multi-year effort. It's way too early to give any guess as to when the APIs will be removed. As the JEP says, future releases may degrade the SM APIs so that System.getSM returns always returns null or AccessController::doPriv just runs the action. This should mean that libraries that are compiling to older releases should continue to compile and run on those releases. When they run on some future release that degrades the implementation then it will be as if there is no SM. So I would say the impact is little to none for libraries for the foreseeable future. -Alan On 13/06/2021 21:28, Rafael Winterhalter wrote: I am currently looking into how I should address JEP 411 in my library Byte Buddy and I find it rather challenging. The problem I am facing is that I know of several users who rely on the security manager in their Java 8/11 applications. I would like to continue to support those users' use cases as long as I support Java versions that contain the security manager, which will be for many years to come. At the same time, I would like to address the announced removal of the API and make sure that Byte Buddy can work without it prior to the deadline when the library in its current state would no longer link. From my understanding of the intention of JEP 411, the API was supposed to be stubbed – similar to Android’s stubbing of the API - rather than being removed. However, with the announced deprecation for removal of AccessController and SecurityManager, I understand that I would need to fully remove the dispatching to work with future Java versions. Furthermore, it is difficult to create a working facade for dispatching to the security manager only if it is available. Methods like AccessController.doPrivileged are caller sensitive and by adding a utility to a library, this utility would leak to any potential user. It would therefore require package-private dispatchers for any relevant package, which would lead to a lot of copy-paste to retain backwards compatibility (given that a library cannot assume to be run as a module). Finally, removing the API would mean that Byte Buddy versions of the last ten years would no longer link in future JDKs. For Byte Buddy where new Java versions often require an update, that might not be a big issue but many other libraries do support the API, I don’t feel it would be a rather severe restriction and cause unnecessary breakage if API is removed, rather than stubbed. I am thinking of libraries like Netty here which are rather omnipresent and would suddenly no longer link, a concept that is unlikely intuitive to a lot of developers. Therefore, my question is: should SecurityManager, AccessController and the Policy APIs really be deprecated for removal? Rather, I think that the APIs should be deprecated, but be retained with stubbed implementations. System.getSecurityMananger would then always return null. System.setSecurityManager on the other hand could be deprecated for removal. This way, existing code could continue to work as if the security manager is not active, which already is the common scenario and would not cause any disruption at the small price of keeping a handful of some stubbed classes. Thanks for advice on how this is intended to be handled by library developers like me. Best regards, Rafael
Re: Low level hooks in JDK for instrumentation of permission checks.
On 10/06/2021 07:40, Peter Firmstone wrote: Just a quick question, would it be possible that some JFR hooks might also be useable for an authorisation layer? JFR events can't be used to intercept/veto operations, assuming that is what you are asking. However, it might be that JFR events are monitored as part of some overall security approach that takes into account events recorded for health, performance, or troubleshooting purposes. -Alan
Re: Low level hooks in JDK for instrumentation of permission checks.
On 10/06/2021 03:49, Peter Firmstone wrote: Hi Sean, Sorry I've confused you. What I should have said is a ProtectionDomain with a null CodeSource. What I mean to ask is, where ProtectionDomain is created with a null CodeSource, in Class::getProtectionDomain() can we have CodeSource's that represents system modules instead of null? A CodeSource with URL's like jrt:/jdk.* or jrt:/java.* for system modules? This is already the case for system modules that are mapped to the platform or application class loaders. I think your question is about modules that are mapped to the boot loader and whether they should get a unique PD that includes a useful code source rather than using a "shared" PD. That would be changing long standing behavior and would require careful analysis to see if anything would break. -Alan
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Tue, 8 Jun 2021 18:22:55 GMT, Weijun Wang 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. >> >> 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. I think it would be simpler to start out without the caller cache. Sorry the sentence got garbled, I was trying to repeat what I said above that WeakHashMap is not synchronized so you would need to add synchronization to use it. - 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 12:22:52 GMT, Weijun Wang 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. WeakHashMap access 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? - 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:30:00 GMT, David Holmes 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. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 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. Changes requested by alanb (Reviewer). 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. 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: JEP 411: Documentation on the new way to establish TLS connections
On 04/06/2021 05:22, Peter Firmstone wrote: Could someone please advise the recommended way now to preserve the Subject in Executors to establish a TLS connection? I think the primitive you are looking for is scope local variables. It's still in the exploration phase but there is a draft JEP [1]. I'm quite sure that if we did this 20 years ago then Subject.doAs and friends would be different. -Alan [1] http://openjdk.java.net/jeps/8263012
Re: Authorization Layer post JEP 411
On 03/06/2021 01:04, Chapman Flack wrote: On 06/02/21 19:41, Peter Firmstone wrote: We need the power of AccessController's stack walk, StackWalker doesn't work with compiled code, only bytecode. Is this correct? I have not tried it, but the apidocs had led me to understand it did not distinguish much between JITed and interpreted code. Even getByteCodeIndex does not mention any limitation when the frame is JITed Java code (though it does say it will return a negative number in the distinct case of an actual native method). There should be no issue here. I suspect Peter meant that the stack walker is about walking Java frames, it's transparent whether there are interpreter frames, compiled frame, or a mix on the stack. -Alan
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v8]
On Tue, 1 Jun 2021 15:21:33 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 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 Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4073
