Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this fix to allow a read-only 'src' buffer to be used with > SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher > operation when a read-only buffer is passed. If the 'src' is read-write, > there is no effect on the current operation > > The PR also includes a CSR for an API implementation note to the > SSLEngine.unwrap. The 'src' buffer may be modified during the decryption > operation. 'unwrap()' has had this behavior forever, so there is no > compatibility issue with this note. Using the 'src' buffer for in-place > decryption was a performance decision. > > Tony src/java.base/share/classes/javax/net/ssl/SSLEngine.java line 677: > 675: * @see #unwrap(ByteBuffer, ByteBuffer[], int, int) > 676: * > 677: * @implNote The data in {@code src} may be modified during the > decryption It looks like a note for the API users to me. Is apiNote tag more appropriate here? - PR: https://git.openjdk.java.net/jdk/pull/8462
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: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this fix to allow a read-only 'src' buffer to be used with > SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher > operation when a read-only buffer is passed. If the 'src' is read-write, > there is no effect on the current operation > > The PR also includes a CSR for an API implementation note to the > SSLEngine.unwrap. The 'src' buffer may be modified during the decryption > operation. 'unwrap()' has had this behavior forever, so there is no > compatibility issue with this note. Using the 'src' buffer for in-place > decryption was a performance decision. > > Tony All of the comments observed are minor. Please consider some of the test changes, but otherwise looks good. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this fix to allow a read-only 'src' buffer to be used with > SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher > operation when a read-only buffer is passed. If the 'src' is read-write, > there is no effect on the current operation > > The PR also includes a CSR for an API implementation note to the > SSLEngine.unwrap. The 'src' buffer may be modified during the decryption > operation. 'unwrap()' has had this behavior forever, so there is no > compatibility issue with this note. Using the 'src' buffer for in-place > decryption was a performance decision. > > Tony Finished the SSLEngine/tests, starting on the SSLCipher. Here's the notes so far. test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 1: > 1: /* Wondering why this is in javax/net/ssl/SSLSession instead of sun/security/ssl/SSLCipher. test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 51: > 49: public class ReadOnlyEngine { > 50: > 51: private static String pathToStores = "../etc"; These 6 can be final if you want. test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 96: > 94: status = engine.getHandshakeStatus(); > 95: break; > 96: case FINISHED: Can combine FINISHED/NOT_HANDSHAKING? test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 157: > 155: // Do TLS handshake > 156: do { > 157: statusClient = doHandshake(client, out, in); It's potentially a little inefficient returning after each wrap/unwrap() instead of doing the task right away, but it works. No need to change. Is this style something you copied from another test? The SSLEngineTemplate in the templates directory is what I often use. test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 160: > 158: statusServer = doHandshake(server, in, out); > 159: } while (statusClient != HandshakeStatus.NOT_HANDSHAKING || > 160: statusServer != HandshakeStatus.NOT_HANDSHAKING); Minor indent problem. test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 162: > 160: statusServer != HandshakeStatus.NOT_HANDSHAKING); > 161: > 162: // Read NST What is NST? test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 172: > 170: out.clear(); > 171: String testString = "ASDF"; > 172: in.put(testString.getBytes()).flip(); If you're going to convert back from UTF_8 later, you should probably convert using getBytes(UTF_8) here. test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 188: > 186: in.clear(); > 187: System.out.println("2: Server send: " + testString); > 188: in.put(testString.getBytes()).flip(); Same test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 189: > 187: System.out.println("2: Server send: " + testString); > 188: in.put(testString.getBytes()).flip(); > 189: send(server, in, out); Did you want to try asReadOnlyBuffer() here also? test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 191: > 189: send(server, in, out); > 190: in.clear(); > 191: receive(client, out, in); And here? - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this fix to allow a read-only 'src' buffer to be used with > SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher > operation when a read-only buffer is passed. If the 'src' is read-write, > there is no effect on the current operation > > The PR also includes a CSR for an API implementation note to the > SSLEngine.unwrap. The 'src' buffer may be modified during the decryption > operation. 'unwrap()' has had this behavior forever, so there is no > compatibility issue with this note. Using the 'src' buffer for in-place > decryption was a performance decision. > > Tony I don't yet count as a reviewer, but your changes look good to me. You might want to let IntelliJ check for problems before you integrate. - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8642/files - new: https://git.openjdk.java.net/jdk/pull/8642/files/e72ce85c..7ff806a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8642=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8642=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8642.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642 PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 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). IO, math, and NIO changes look fine. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 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). The update in security area looks good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 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). Looks good. Assuming copyright years will be updated. src/java.base/share/classes/java/time/es line 1: > 1: X Looks like a random file was added by accident? - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
RFR: 8286378: Address possibly lossy conversions in java.base
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). - Commit messages: - 8286378: Address possibly lossy conversions in java.base Changes: https://git.openjdk.java.net/jdk/pull/8642/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8642=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286378 Stats: 57 lines in 33 files changed: 1 ins; 3 del; 53 mod Patch: https://git.openjdk.java.net/jdk/pull/8642.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642 PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults
On Fri, 6 May 2022 17:56:44 GMT, Alan Bateman 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. You are right. The old way maps the null string to true, and the new way maps it to false. I did not notice that. At this point, I see no value in making the "true".equals and "false".equals changes. Too much can break. I'll reverse these changes. - PR: https://git.openjdk.java.net/jdk/pull/8559
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
On Tue, 10 May 2022 13:51:37 GMT, Jaikiran Pai wrote: >> This change proposes to implement the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8209137. >> >> The change introduces a new API to allow applications to build a >> `java.net.http.HTTPClient` configured with a specific local address that >> will be used while creating `Socket`(s) for connections. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix javadoc link in test LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]
On Tue, 10 May 2022 13:35:35 GMT, Daniel Fuchs wrote: >> Jaikiran Pai has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Daniel's review suggestion - add a test to verify the behaviour of the >> localAddress() default method implementation on HttpClient.Builder >> - Daniel's review suggestion - remove reference to "Internet Protocol" in >> javadoc > > test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 268: > >> 266: /** >> 267: * Tests the {@link >> HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method >> 268: * behaviour when that method is called on a builder returned by >> {@link HttpClient#newBuilder()} > > /** > * Tests the {@link > HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method > ``` > This `{@link` looks broken - the `HttpClient,` prefix probably need to be > removed? Indeed. That prefix wasn't intentional. I'm not sure how I ended up with that. I just pushed an update to fix this. Thank you for flagging this. - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]
On Tue, 10 May 2022 12:37:47 GMT, Jaikiran Pai wrote: >> This change proposes to implement the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8209137. >> >> The change introduces a new API to allow applications to build a >> `java.net.http.HTTPClient` configured with a specific local address that >> will be used while creating `Socket`(s) for connections. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Daniel's review suggestion - add a test to verify the behaviour of the > localAddress() default method implementation on HttpClient.Builder > - Daniel's review suggestion - remove reference to "Internet Protocol" in > javadoc I've triggered some internal CI runs against the current state of PR to see all works fine. Now that we have decided on the nature of this API, I'll go ahead and file a CSR for this. - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
> This change proposes to implement the enhancement noted in > https://bugs.openjdk.java.net/browse/JDK-8209137. > > The change introduces a new API to allow applications to build a > `java.net.http.HTTPClient` configured with a specific local address that will > be used while creating `Socket`(s) for connections. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: fix javadoc link in test - Changes: - all: https://git.openjdk.java.net/jdk/pull/6690/files - new: https://git.openjdk.java.net/jdk/pull/6690/files/3a6f2b6b..d9f7b077 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6690=18 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6690=17-18 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6690.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6690/head:pull/6690 PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]
On Tue, 10 May 2022 13:36:18 GMT, Daniel Fuchs wrote: >> Jaikiran Pai has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Daniel's review suggestion - add a test to verify the behaviour of the >> localAddress() default method implementation on HttpClient.Builder >> - Daniel's review suggestion - remove reference to "Internet Protocol" in >> javadoc > > test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 283: > >> 281: * Tests that the default method implementation of >> 282: * {@link >> HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} throws >> 283: * an {@link UnsupportedOperationException} > > Same remark here Otherwise things look good to me - you should update the CSR if it needs to be updated. - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]
On Tue, 10 May 2022 12:37:47 GMT, Jaikiran Pai wrote: >> This change proposes to implement the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8209137. >> >> The change introduces a new API to allow applications to build a >> `java.net.http.HTTPClient` configured with a specific local address that >> will be used while creating `Socket`(s) for connections. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Daniel's review suggestion - add a test to verify the behaviour of the > localAddress() default method implementation on HttpClient.Builder > - Daniel's review suggestion - remove reference to "Internet Protocol" in > javadoc test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 268: > 266: /** > 267: * Tests the {@link > HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method > 268: * behaviour when that method is called on a builder returned by > {@link HttpClient#newBuilder()} /** * Tests the {@link HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method ``` This `{@link` looks broken - the `HttpClient,` prefix probably need to be removed? test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 283: > 281: * Tests that the default method implementation of > 282: * {@link > HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} throws > 283: * an {@link UnsupportedOperationException} Same remark here - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v17]
On Mon, 9 May 2022 13:31:38 GMT, Daniel Fuchs wrote: >> Jaikiran Pai has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 37 commits: >> >> - Merge latest from master branch >> - add a @build to force jtreg to show consistent test results and add the >> relevant permissions for security manager testing >> - Change the new API to accept an InetAddress instead of an >> InetSocketAddress, after inputs from Michael, Daniel and others >> - Merge latest from master >> - Implement HttpServerAdapters in test as suggested by Daniel >> - fix check when security manager is enabled >> - Add a unit test for the new HttpClient.Builder.localAddress method >> - Implement Daniel's suggestion - only support InetSocketAddress with port 0 >> - Merge latest from master branch >> - Merge latest from master branch >> - ... and 27 more: >> https://git.openjdk.java.net/jdk/compare/b490a58e...d4a19dea > > src/java.net.http/share/classes/java/net/http/HttpClient.java line 398: > >> 396: * >> 397: * @implSpec If the {@link #localAddress(InetAddress) local >> address} is a non-null >> 398: * Internet Protocol address and a security manager is >> installed, then > > Here and in the @throws bellow we could now remove mention of `Internet > Protocol`. > For instance, here we could say > > ... is a non-null address and a security ... Done. Updated this and the other javadoc comment to follow this suggestion. > test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 274: > >> 272: // setting null should work fine >> 273: builder.localAddress(null); >> 274: builder.localAddress(InetAddress.getLoopbackAddress()); > > We probably also need a MockHttpClientBuilder to test the behaviour of the > default implementation (check that it throws UOE). I've now updated this PR to add a new test method which tests the default method implementation of `localAddress`. - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]
> This change proposes to implement the enhancement noted in > https://bugs.openjdk.java.net/browse/JDK-8209137. > > The change introduces a new API to allow applications to build a > `java.net.http.HTTPClient` configured with a specific local address that will > be used while creating `Socket`(s) for connections. Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision: - Daniel's review suggestion - add a test to verify the behaviour of the localAddress() default method implementation on HttpClient.Builder - Daniel's review suggestion - remove reference to "Internet Protocol" in javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/6690/files - new: https://git.openjdk.java.net/jdk/pull/6690/files/d4a19dea..3a6f2b6b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6690=17 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6690=16-17 Stats: 74 lines in 2 files changed: 70 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6690.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6690/head:pull/6690 PR: https://git.openjdk.java.net/jdk/pull/6690