Re: RFR: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest [v3]
On Tue, 27 Aug 2024 08:38:23 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to fix the issue >> noted in https://bugs.openjdk.org/browse/JDK-8338445? >> >> The JDK internal class `jdk.internal.loader.URLClassPath` is used by >> classloader implementations in the JDK (for example by the >> `java.net.URLClassLoader` and the >> `jdk.internal.loader.ClassLoaders$AppClassLoader`). The implementation in >> `URLClassPath` constructs internal `Loader`s for each URL that is in the >> classpath of that instance. `Loader` implementations hold on to underlying >> resources from which the classpath resources are served by the >> `URLClassPath`. >> >> When constructing a `Loader`, the `URLClassPath` allows the loader >> implementation to parse a local classpath for that specific `Loader`. For >> example, the `JarLoader` (an internal class of the JDK) will parse >> `Class-Path` attribute in the jar file's manifest to construct additional >> URLs that will be included in the classpath through which resources will be >> served by `URLClassPath`. While parsing the local classpath, if the `Loader` >> instance runs into any `IOException` or a `SecurityException`, the >> `URLClassPath` will ignore that specific instance of the `Loader` and will >> not hold on to it as a classpath element. >> >> As noted earlier, `Loader` instances may hold onto underlying resources. >> When the `URLClassPath` ignores such `Loader`(s) and doesn't call `close()` >> on them, then that results in potential resource leaks. The linked JBS issue >> demonstrates a case where the `JarFile` held by the `JarLoader` doesn't get >> closed (until GC garbage collects the `JarLoader` and thus the `JarFile`), >> when the `URLClassPath` ignores that `JarLoader` due to an `IOException` >> when the `JarLoader` is parsing the `Class-Path` value from the jar's >> manifest. >> >> The commit in this PR addresses the issue by calling `close()` on such >> `Loader`s that get rejected by the `URLClassPath` when either an >> `IOException` or a `SecurityException` gets thrown when constructing the >> `Loader` or parsing the local classpath. >> >> A new jtreg test has been introduced which consistently reproduces this >> issue (on Windows) and verifies the fix. Additionally tier1, tier2 and tier3 >> testing has completed with this change without any failures. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Christian's suggestion - string ValueSource(s) instead of MethodSource Update looks reasonable - Marked as reviewed by michaelm (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20691#pullrequestreview-2265658939
Re: RFR: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest [v2]
On Mon, 26 Aug 2024 03:47:49 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to fix the issue >> noted in https://bugs.openjdk.org/browse/JDK-8338445? >> >> The JDK internal class `jdk.internal.loader.URLClassPath` is used by >> classloader implementations in the JDK (for example by the >> `java.net.URLClassLoader` and the >> `jdk.internal.loader.ClassLoaders$AppClassLoader`). The implementation in >> `URLClassPath` constructs internal `Loader`s for each URL that is in the >> classpath of that instance. `Loader` implementations hold on to underlying >> resources from which the classpath resources are served by the >> `URLClassPath`. >> >> When constructing a `Loader`, the `URLClassPath` allows the loader >> implementation to parse a local classpath for that specific `Loader`. For >> example, the `JarLoader` (an internal class of the JDK) will parse >> `Class-Path` attribute in the jar file's manifest to construct additional >> URLs that will be included in the classpath through which resources will be >> served by `URLClassPath`. While parsing the local classpath, if the `Loader` >> instance runs into any `IOException` or a `SecurityException`, the >> `URLClassPath` will ignore that specific instance of the `Loader` and will >> not hold on to it as a classpath element. >> >> As noted earlier, `Loader` instances may hold onto underlying resources. >> When the `URLClassPath` ignores such `Loader`(s) and doesn't call `close()` >> on them, then that results in potential resource leaks. The linked JBS issue >> demonstrates a case where the `JarFile` held by the `JarLoader` doesn't get >> closed (until GC garbage collects the `JarLoader` and thus the `JarFile`), >> when the `URLClassPath` ignores that `JarLoader` due to an `IOException` >> when the `JarLoader` is parsing the `Class-Path` value from the jar's >> manifest. >> >> The commit in this PR addresses the issue by calling `close()` on such >> `Loader`s that get rejected by the `URLClassPath` when either an >> `IOException` or a `SecurityException` gets thrown when constructing the >> `Loader` or parsing the local classpath. >> >> A new jtreg test has been introduced which consistently reproduces this >> issue (on Windows) and verifies the fix. Additionally tier1, tier2 and tier3 >> testing has completed with this change without any failures. > > Jaikiran Pai has updated the pull request incrementally with three additional > commits since the last revision: > > - revert to old code comment > - use "JAR file" consistently in test's code comments > - rename closeLoaderIgnoreEx to closeQuietly LGTM - Marked as reviewed by michaelm (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20691#pullrequestreview-2261044090
Re: RFR: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest [v2]
On Mon, 26 Aug 2024 15:36:25 GMT, Jaikiran Pai wrote: >> Is the if statement at line 470 redundant then? Presumably the code can >> reach here. > > Hello Michael, >> Is the if statement at line 470 redundant then? > > `Loader.getClassPath()` can return null (and it does for some implementations > of Loader). So the null check would still be needed. What if `getLoader(url)` throws an exception so `Loader.getClassPath()` is not called? Okay, I see Daniel's point, that in that case, the code is not reached. - PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1731446869
Re: RFR: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest [v2]
On Mon, 26 Aug 2024 15:27:39 GMT, Daniel Fuchs wrote:
>> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 470:
>>
>>> 468: continue;
>>> 469: }
>>> 470: if (loaderClassPathURLs != null) {
>>
>> I'm missing something here. Why does the compiler not complain that
>> `loaderClassPathURLs` might not be initialized, when it's accessed here?
>
> Because we would not reach here - since it's either assigned at line 447 or
> we continue at line 457, 468, or exit throwing an uncaught exception?
Is the if statement at line 470 redundant then? Presumably the code can reach
here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1731430476
Re: RFR: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest [v2]
On Mon, 26 Aug 2024 03:47:49 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to fix the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8338445?
>>
>> The JDK internal class `jdk.internal.loader.URLClassPath` is used by
>> classloader implementations in the JDK (for example by the
>> `java.net.URLClassLoader` and the
>> `jdk.internal.loader.ClassLoaders$AppClassLoader`). The implementation in
>> `URLClassPath` constructs internal `Loader`s for each URL that is in the
>> classpath of that instance. `Loader` implementations hold on to underlying
>> resources from which the classpath resources are served by the
>> `URLClassPath`.
>>
>> When constructing a `Loader`, the `URLClassPath` allows the loader
>> implementation to parse a local classpath for that specific `Loader`. For
>> example, the `JarLoader` (an internal class of the JDK) will parse
>> `Class-Path` attribute in the jar file's manifest to construct additional
>> URLs that will be included in the classpath through which resources will be
>> served by `URLClassPath`. While parsing the local classpath, if the `Loader`
>> instance runs into any `IOException` or a `SecurityException`, the
>> `URLClassPath` will ignore that specific instance of the `Loader` and will
>> not hold on to it as a classpath element.
>>
>> As noted earlier, `Loader` instances may hold onto underlying resources.
>> When the `URLClassPath` ignores such `Loader`(s) and doesn't call `close()`
>> on them, then that results in potential resource leaks. The linked JBS issue
>> demonstrates a case where the `JarFile` held by the `JarLoader` doesn't get
>> closed (until GC garbage collects the `JarLoader` and thus the `JarFile`),
>> when the `URLClassPath` ignores that `JarLoader` due to an `IOException`
>> when the `JarLoader` is parsing the `Class-Path` value from the jar's
>> manifest.
>>
>> The commit in this PR addresses the issue by calling `close()` on such
>> `Loader`s that get rejected by the `URLClassPath` when either an
>> `IOException` or a `SecurityException` gets thrown when constructing the
>> `Loader` or parsing the local classpath.
>>
>> A new jtreg test has been introduced which consistently reproduces this
>> issue (on Windows) and verifies the fix. Additionally tier1, tier2 and tier3
>> testing has completed with this change without any failures.
>
> Jaikiran Pai has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - revert to old code comment
> - use "JAR file" consistently in test's code comments
> - rename closeLoaderIgnoreEx to closeQuietly
src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 470:
> 468: continue;
> 469: }
> 470: if (loaderClassPathURLs != null) {
I'm missing something here. Why does the compiler not complain that
`loaderClassPathURLs` might not be initialized, when it's accessed here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1731407899
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v3]
On Thu, 21 Mar 2024 17:13:46 GMT, Bill Huang wrote: >> This task addresses an essential aspect of our testing infrastructure: the >> proper handling and cleanup of temporary files and socket files created >> during test execution. The motivation behind these changes is to prevent the >> accumulation of unnecessary files in the default temporary directory, which >> can affect the system's storage and potentially influence subsequent test >> runs. >> >> Our review identified that several tests create temporary files or socket >> files without ensuring their removal post-execution. >> - Direct calls to java.io.File.createTempFile and >> java.nio.file.Files.createTempFile without adequate cleanup. >> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not >> explicitly removing socket files post-use. > > Bill Huang has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed potential NPE issues. unixdomain NIO test changes look fine to me - Marked as reviewed by michaelm (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18352#pullrequestreview-1960889711
Re: CFV: New Core Libraries Group Member: Per-Ake Minborg
Vote: Yes On 19/03/2024 16:19, Daniel Fuchs wrote: Hi, I hereby nominate Per-Ake Minborg (pminborg) [1] to Membership in the Core Libraries Group [4]. Per-Ake is an OpenJDK Reviewer, a committer in the Leyden and Panama projects, and a member of Oracle’s Java Core Libraries team. Per-Ake has been actively participating in the development of the JDK and Panama projects for several years, and is one of the co-author of the Implementation of Foreign Function and Memory API (Third Preview) [2]. His contributions include more than 80 commits in the JDK [3] Votes are due by 16:00 UTC on April 2, 2024. Only current Members of the Core Libraries Group [4] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [5]. best regards, -- daniel [1] https://openjdk.org/census#pminborg [2] https://github.com/openjdk/jdk/commit/cbccc4c8172797ea2f1b7c301d00add3f517546d [3] https://github.com/search?q=repo%3Aopenjdk%2Fjdk+author%3Aminborg&type=commits [4] https://openjdk.org/census#core-libs [5] https://openjdk.org/groups/#member-vote
Re: CFV: New Core Libraries Group Member: Viktor Klang
Vote: Yes On 20/02/2024 17:51, Naoto Sato wrote: Vote: yes Naoto On 2/19/24 3:40 PM, Stuart Marks wrote: I hereby nominate Viktor Klang [6] to Membership in the Core Libraries Group [4]. Viktor has been a member of the Java team in Oracle since 2022, and he is currently a Committer on the JDK project. Viktor has led JEP 461 [1], an enhancement to the Streams API to support new intermediate stream operations. Viktor has also contributed several other changes to the JDK [2][3], focusing primarily on the java.util.concurrent package. Prior to his work on OpenJDK, Viktor's past work includes Akka, Reactive Streams, and many other contributions to the Scala world. Votes are due by 23:59 UTC on March 4, 2024. Only current Members of the Core Libraries Group [4] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [5]. s'marks [1] https://openjdk.org/jeps/461 [2] https://github.com/openjdk/jdk/commits?author=vklang%40openjdk.org [3] https://github.com/openjdk/jdk/commits?author=viktorklang-ora [4] https://openjdk.org/census#core-libs [5] https://openjdk.org/groups/#member-vote [6] https://openjdk.org/census#vklang
Re: CFV: New Core Libraries Group Member: Raffaello Giulietti
Vote: Yes On 13/02/2024 20:25, Brian Burkhalter wrote: I hereby nominate Raffaello Giulietti to Membership in the Core Libraries Group. Raffaello has been working in the Core Library team at Oracle since April, 2022. He has authored more than 50 contributions to OpenJDK in a number of areas including numerics, formatting, regular expressions, and random number generation. Votes are due by Wednesday, February 28, 2023. Only current Members of the Core Libraries Group [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list For Lazy Consensus voting instructions, see [2]. Brian Burkhalter [1]https://openjdk.org/census#core-libs [2]https://openjdk.org/groups/#member-vote
Re: HttpURLConnection cache issues leading to crashes in JGSS w/ native GSS introduced by 8303809
Hi, Thanks for bringing this to our attention. You are right that this is a misuse of the authentication cache in the case of Kerberos (Negotiate) authentication. Though that is not the case for other auth schemes, because normally what gets cached are credentials, rather than security tokens. It makes no sense to cache GSS contexts either, outside the scope of any individual request (being authenticated through multiple request/responses). We don't need to cache it in this situation as it is already kept as a local variable in the HttpURLConnection impl class. So, my first impression is that the fix here needs to disable the cache permanently for the Negotiate scheme, which is basically what the system property workaround is doing. But, we need to just be sure about that before we publish a PR. Thanks, Michael. On 19/10/2023 23:09, Nico Williams wrote: # Crashes We recently upgrade to OpenJDK 17.0.8.1 and started observing crashes resulting from double-frees via `gss_delete_sec_context()`. Adding `-Djdk.spnego.cache=false` to our java invocations stops the crashes. We believe this is due to a race condition that has long been in `HttpURLConnection` that was mostly harmless before but which with the addition of this commit: 16843770b5a 8303809: Dispose context in SPNEGO NegotiatorImpl became a use-after-free / double-free bug, leading to crashes. This happens with stock 17.0.8.1+1 from Adoptium. Attached is a reproducer, Test.java, which you can run like this: $ # credendials are kinitu...@jgssbug.twosigma.com, password password $ # $ "$JAVA_HOME/bin/java" \ -Dsun.security.jgss.native=true \ -Dsun.security.jgss.lib=/usr/lib/libgssapi_krb5.so \ Test.javahttps://kerberos.club/tmp/jgssbug.txt It doesn't crash every time, but it crashes often. A typical Java stack trace upon crashing looks like: j sun.security.jgss.wrapper.GSSLibStub.deleteContext(J)J+0java.security.jgss@17.0.8.1 j sun.security.jgss.wrapper.NativeGSSContext.dispose()V+76java.security.jgss@17.0.8.1 j sun.security.jgss.GSSContextImpl.dispose()V+16java.security.jgss@17.0.8.1 j sun.net.www.protocol.http.spnego.NegotiatorImpl.disposeContext()V+11java.security.jgss@17.0.8.1 J 4456 c1 sun.net.www.protocol.http.NegotiateAuthentication.disposeContext()Vjava.base@17.0.8.1 (24 bytes) @ 0x7f909dc33834 [0x7f909dc337c0+0x000 00074] j sun.net.www.protocol.http.HttpURLConnection.getInputStream0()Ljava/io/InputStream;+1923java.base@17.0.8.1 j sun.net.www.protocol.http.HttpURLConnection.getInputStream()Ljava/io/InputStream;+62java.base@17.0.8.1 j sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream()Ljava/io/InputStream;+4java.base@17.0.8.1 j Test.lambda$main$0()V+16 j Test$$Lambda$207+0x7f9020144208.run()V+0 j java.lang.Thread.run()V+11java.base@17.0.8.1 v ~StubRoutines::call_stub On the C side the crash typically happens in the allocator, typically in `free()`: #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x7fb6e1185535 in __GI_abort () at abort.c:79 #2 0x7fb6e11dc648 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7fb6e12e62a0 "%s\n") at ../sysdeps/posix/libc_fatal.c:181 #3 0x7fb6e11e2d6a in malloc_printerr (str=str@entry=0x7fb6e12e444e "free(): invalid pointer") at malloc.c:5359 #4 0x7fb6e11e459c in _int_free (av=, p=, have_lock=) at malloc.c:4180 #5 0x7fb582ba5b6d in krb5_free_address (context=context@entry=0x7fb5d80cbf60, val=0x7fb5d880) at kfree.c:62 #6 0x7fb582b93d9e in krb5_auth_con_free (context=context@entry=0x7fb5d80cbf60, auth_context=0x7fb5d80778a0) at auth_con.c:60 #7 0x7fb6440a3eec in krb5_gss_delete_sec_context (minor_status=0x7fb586868504, context_handle=0x7fb5d80e22d0, output_token=) at delete_sec_context.c:77 #8 0x7fb6440964b0 in gssint_delete_internal_sec_context (minor_status=0x7fb586868504, mech_type=, internal_ctx=internal_ctx@entry=0x7fb5d80e22d0, output_token=0x0) at g_glue.c:606 #9 0x7fb6440946dd in gss_delete_sec_context (minor_status=, context_handle=0x7fb5d80d2be8, output_token=) at g_delete_sec_context.c:91 #10 0x7fb6440b8dfe in spnego_gss_delete_sec_context (minor_status=, context_handle=0x7fb5d807bd70, output_token=) at spnego_mech.c:2161 #11 0x7fb6440964b0 in gssint_delete_internal_sec_context (minor_status=0x7fb586868504, mech_type=, internal_ctx=internal_ctx@entry=0x7fb5d807bd70, output_token=0x0) at g_glue.c:606 #12 0x7fb6440946dd in gss_delete_sec_context (minor_status=, context_handle=0x7fb586868508, output_token=) at g_delete_sec_context.c:91 #13 0x7fb6dc0559ff in Java_sun_security_jgss_wrapper_GSSLibStub_deleteContext () from /home/nico/tmp/jdk-17.0.8.1+1/lib/libj2gss.so #14 0x7fb6c89a653a in ?? () #15 0x in ?? () We've seen other variations, but always they can be traced to `HttpURLConnection`.
Re: RFR: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array
On Wed, 24 May 2023 09:24:46 GMT, Rudi Horn wrote: > This change prevents the contents of the internal string array from being > copied back when releasing it. There was an unrelated change to these functions recently. You might want to rebase this PR with the latest version. - PR Comment: https://git.openjdk.org/jdk/pull/14117#issuecomment-1564712601
Integrated: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters
On Mon, 22 May 2023 13:19:04 GMT, Michael McMahon wrote: > This PR creates a new version of the JNI utility function > JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which > performs additional validation of the returned string, namely that it does > not contain any embedded NULL characters. If any such characters are found > the function returns NULL with an IAE pending. The change also switches usage > in the networking native code to use the new function. > > This cautious approach was taken rather than changing the behavior of the > existing function as each native code area needs to review the effect of > making the switch. Otherwise, surprising behavior changes might occur (eg > undocumented IAE being thrown to user code instead of some other exception). This pull request has now been integrated. Changeset: e7edf8d1 Author:Michael McMahon URL: https://git.openjdk.org/jdk/commit/e7edf8d1458ff0d66aedbb0086050c36864702f6 Stats: 188 lines in 9 files changed: 163 ins; 1 del; 24 mod 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters Reviewed-by: dfuchs, naoto - PR: https://git.openjdk.org/jdk/pull/14083
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v4]
On Wed, 24 May 2023 20:46:23 GMT, Michael McMahon wrote: >> This PR creates a new version of the JNI utility function >> JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which >> performs additional validation of the returned string, namely that it does >> not contain any embedded NULL characters. If any such characters are found >> the function returns NULL with an IAE pending. The change also switches >> usage in the networking native code to use the new function. >> >> This cautious approach was taken rather than changing the behavior of the >> existing function as each native code area needs to review the effect of >> making the switch. Otherwise, surprising behavior changes might occur (eg >> undocumented IAE being thrown to user code instead of some other exception). > > Michael McMahon 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 nullStrings > - error message and test update > - Merge branch 'master' into nullStrings > - test comment update > - test update > - Merge branch 'master' into nullStrings > - exception message update > - test update > - remve whitespace > - update > - ... and 2 more: https://git.openjdk.org/jdk/compare/207fbcb0...35df1a67 I'll integrate this today unless there are further comments. - PR Comment: https://git.openjdk.org/jdk/pull/14083#issuecomment-1562998991
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v3]
On Wed, 24 May 2023 14:30:01 GMT, Alan Bateman wrote: > I skimmed through the changes and mostly look okay. There's a few usage of > "NULL" in exception messages that I assume should be "NUL character". > Probably need to bump the copyright date on several files. You might want to > find a better name for the test as NullChar isn't too descriptive, the test > is really doing a lookup of a hostname that contains a NUL character. I > assume there will be follow up issues to change other parts of the system to > use the strict functions. Thanks, I've updated per these comments. - PR Comment: https://git.openjdk.org/jdk/pull/14083#issuecomment-1561899890
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v4]
> This PR creates a new version of the JNI utility function > JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which > performs additional validation of the returned string, namely that it does > not contain any embedded NULL characters. If any such characters are found > the function returns NULL with an IAE pending. The change also switches usage > in the networking native code to use the new function. > > This cautious approach was taken rather than changing the behavior of the > existing function as each native code area needs to review the effect of > making the switch. Otherwise, surprising behavior changes might occur (eg > undocumented IAE being thrown to user code instead of some other exception). Michael McMahon 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 nullStrings - error message and test update - Merge branch 'master' into nullStrings - test comment update - test update - Merge branch 'master' into nullStrings - exception message update - test update - remve whitespace - update - ... and 2 more: https://git.openjdk.org/jdk/compare/207fbcb0...35df1a67 - Changes: https://git.openjdk.org/jdk/pull/14083/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14083&range=03 Stats: 188 lines in 9 files changed: 163 ins; 1 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/14083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14083/head:pull/14083 PR: https://git.openjdk.org/jdk/pull/14083
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v3]
> This PR creates a new version of the JNI utility function > JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which > performs additional validation of the returned string, namely that it does > not contain any embedded NULL characters. If any such characters are found > the function returns NULL with an IAE pending. The change also switches usage > in the networking native code to use the new function. > > This cautious approach was taken rather than changing the behavior of the > existing function as each native code area needs to review the effect of > making the switch. Otherwise, surprising behavior changes might occur (eg > undocumented IAE being thrown to user code instead of some other exception). Michael McMahon has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits: - Merge branch 'master' into nullStrings - test comment update - test update - Merge branch 'master' into nullStrings - exception message update - test update - remve whitespace - update - Merge branch 'master' into nullStrings - first impl - Changes: https://git.openjdk.org/jdk/pull/14083/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14083&range=02 Stats: 183 lines in 9 files changed: 163 ins; 1 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/14083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14083/head:pull/14083 PR: https://git.openjdk.org/jdk/pull/14083
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v2]
> This PR creates a new version of the JNI utility function > JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which > performs additional validation of the returned string, namely that it does > not contain any embedded NULL characters. If any such characters are found > the function returns NULL with an IAE pending. The change also switches usage > in the networking native code to use the new function. > > This cautious approach was taken rather than changing the behavior of the > existing function as each native code area needs to review the effect of > making the switch. Otherwise, surprising behavior changes might occur (eg > undocumented IAE being thrown to user code instead of some other exception). Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: test comment update - Changes: - all: https://git.openjdk.org/jdk/pull/14083/files - new: https://git.openjdk.org/jdk/pull/14083/files/8cf24635..0acc456a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14083&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14083&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14083/head:pull/14083 PR: https://git.openjdk.org/jdk/pull/14083
RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters
This PR creates a new version of the JNI utility function JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which performs additional validation of the returned string, namely that it does not contain any embedded NULL characters. If any such characters are found the function returns NULL with an IAE pending. The change also switches usage in the networking native code to use the new function. This cautious approach was taken rather than changing the behavior of the existing function as each native code area needs to review the effect of making the switch. Otherwise, surprising behavior changes might occur (eg undocumented IAE being thrown to user code instead of some other exception). - Commit messages: - test update - Merge branch 'master' into nullStrings - exception message update - test update - remve whitespace - update - Merge branch 'master' into nullStrings - first impl Changes: https://git.openjdk.org/jdk/pull/14083/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14083&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300038 Stats: 183 lines in 9 files changed: 163 ins; 1 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/14083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14083/head:pull/14083 PR: https://git.openjdk.org/jdk/pull/14083
Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
On Wed, 17 May 2023 13:53:55 GMT, Darragh Clarke wrote: >> Updated instances of `toLowerCase` and `toUpperCase` in several net and io >> files to specify `Locale.ROOT` to ensure that case conversion issues don't >> occur, >> >> I didn't add any new tests but ran tier 1-3 with no issues > > Darragh Clarke has updated the pull request incrementally with two additional > commits since the last revision: > > - Update > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > >Co-authored-by: Daniel Jelinski > - removed StreamTokenizer changes, will make seperate ticket for those Marked as reviewed by michaelm (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14006#pullrequestreview-1434277855
Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
On Wed, 17 May 2023 13:53:55 GMT, Darragh Clarke wrote: >> Updated instances of `toLowerCase` and `toUpperCase` in several net and io >> files to specify `Locale.ROOT` to ensure that case conversion issues don't >> occur, >> >> I didn't add any new tests but ran tier 1-3 with no issues > > Darragh Clarke has updated the pull request incrementally with two additional > commits since the last revision: > > - Update > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > >Co-authored-by: Daniel Jelinski > - removed StreamTokenizer changes, will make seperate ticket for those Seems like a useful change and I can see how issues could arise if strings were stored somewhere after being upper/lower cased and then reused in a different locale. Is it correct to say that the assumption is these strings are all supposed to be US ASCII (eg protocol defined identifiers, or hostnames etc) rather than user generated text strings? That seems to be the case as far as I can see. - PR Comment: https://git.openjdk.org/jdk/pull/14006#issuecomment-1554431858
Re: RFR: 8297976: Remove sun.net.ProgressMonitor and related classes [v4]
On Tue, 6 Dec 2022 13:42:04 GMT, Daniel Jeliński wrote: >> Please review this patch that removes progress monitoring classes used by >> UrlConnection. >> Since Java 9 these classes are not used in the JDK, and are not exported >> from java.base. If anyone was still using them, reimplementing them in user >> code should be pretty straightforward. >> >> This PR also fixes the issue where MeteredStream finalizer could resurrect >> an unusable connection, causing unexpected exceptions in other parts of the >> code. >> >> No new regression test; since we are removing the finalizer, I don't believe >> we will see the issue again. I can add a test for that if you think it still >> makes sense. >> >> I had to adjust `ProxyModuleMapping.java` test which used >> `sun.net.ProgressListener` as an example of a module-private interface; I >> replaced it with another public interface from the same package. >> >> Existing tier 1-3 tests continue to pass. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Use System.err Looks fine to me. - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.org/jdk/pull/11474
Re: RFR: 8294241: Deprecate URL public constructors [v4]
On Thu, 3 Nov 2022 11:20:03 GMT, Daniel Fuchs wrote:
>> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
>> to parse or construct any URL.
>>
>> The `java.net.URL` class does not itself encode or decode any URL components
>> according to the escaping mechanism defined in RFC2396. It is the
>> responsibility of the caller to encode any fields, which need to be escaped
>> prior to calling URL, and also to decode any escaped fields, that are
>> returned from URL.
>>
>> This has lead to many issues in the past. Indeed, if used improperly, there
>> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
>> URL string that can be parsed back into the same URL. This can lead to
>> constructing misleading URLs. Another issue is with `equals()` and
>> `hashCode()` which may have to perform a lookup, and do not take
>> encoding/escaping into account.
>>
>> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
>> of the shortcoming of `java.net.URL`. Conversion methods to create a URL
>> from a URI were also added. However, it was left up to the developers to use
>> `java.net.URI`, or not. This RFE proposes to deprecate all public
>> constructors of `java.net.URL`, in order to provide a stronger warning about
>> their potential misuses. To construct a URL, using `URI::toURL` should be
>> preferred.
>>
>> In order to provide an alternative to the constructors that take a stream
>> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
>> java.net.URLStreamHandler)` is provided as part of this change.
>>
>> Places in the JDK code base that were constructing `java.net.URL` have been
>> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
>> issues will be logged to revisit the calling code.
>>
>> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
>
> Daniel Fuchs has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - Update src/java.base/share/classes/java/net/URL.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Update src/java.base/share/classes/java/net/URL.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Update src/java.base/share/classes/java/net/URL.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
Marked as reviewed by michaelm (Reviewer).
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v4]
On Thu, 3 Nov 2022 10:56:28 GMT, Daniel Fuchs wrote:
>> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
>> to parse or construct any URL.
>>
>> The `java.net.URL` class does not itself encode or decode any URL components
>> according to the escaping mechanism defined in RFC2396. It is the
>> responsibility of the caller to encode any fields, which need to be escaped
>> prior to calling URL, and also to decode any escaped fields, that are
>> returned from URL.
>>
>> This has lead to many issues in the past. Indeed, if used improperly, there
>> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
>> URL string that can be parsed back into the same URL. This can lead to
>> constructing misleading URLs. Another issue is with `equals()` and
>> `hashCode()` which may have to perform a lookup, and do not take
>> encoding/escaping into account.
>>
>> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
>> of the shortcoming of `java.net.URL`. Conversion methods to create a URL
>> from a URI were also added. However, it was left up to the developers to use
>> `java.net.URI`, or not. This RFE proposes to deprecate all public
>> constructors of `java.net.URL`, in order to provide a stronger warning about
>> their potential misuses. To construct a URL, using `URI::toURL` should be
>> preferred.
>>
>> In order to provide an alternative to the constructors that take a stream
>> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
>> java.net.URLStreamHandler)` is provided as part of this change.
>>
>> Places in the JDK code base that were constructing `java.net.URL` have been
>> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
>> issues will be logged to revisit the calling code.
>>
>> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
>
> Daniel Fuchs has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - Update src/java.base/share/classes/java/net/URL.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Update src/java.base/share/classes/java/net/URL.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Update src/java.base/share/classes/java/net/URL.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
src/java.base/share/classes/java/net/URL.java line 152:
> 150: * provide any guarantee about its conformance to the URL
> 151: * syntax specification.
> 152: *
I wonder do we need a stronger statement about potential incompatibility here
(line 149 - 151)?
Something to the effect that - due to URI's stricter parsing rules not all
existing URL instances can be represented as URI's and some that are may turn
out to be opaque unexpectedly instead of hierarchical.
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: Unused class java.net.InetAddressContainer
Hi, That class was used in the old PlainSocketImpl socket implementation and was left over when we removed it. I'll file an issue to delete it. Thanks, Michael. On 27/10/2022 12:53, Andrey Turbanov wrote: Hello. As I can see, the class 'java.net.InetAddressContainer' is unused in JDK java code. Is it somehow used by VM, or is it just leftovers from some refactorings? I wonder if we can drop it. Andrey Turbanov
Re: RFR: 8294241: Deprecate URL public constructors
On Thu, 27 Oct 2022 05:14:19 GMT, ExE Boss wrote:
>> src/java.base/share/classes/java/net/JarURLConnection.java line 177:
>>
>>> 175: @SuppressWarnings("deprecation")
>>> 176: var tmp = jarFileURL = new URL(spec.substring(0, separator++));
>>> 177:
>>
>> I realise that @SuppressWarnings needs a declaration here. I wonder if we
>> could agree a better name for the unused variable name though, like
>> `unusedSW` or something better?
>
> Having unnamed local variables[^1] would probably be best for this.
>
> [^1]: https://openjdk.org/jeps/8294349
How about `_unused` or `_unused1`, `_unused2` then in the meantime?
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors
On Wed, 26 Oct 2022 16:00:56 GMT, Daniel Fuchs wrote:
> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
> to parse or construct any URL.
>
> The `java.net.URL` class does not itself encode or decode any URL components
> according to the escaping mechanism defined in RFC2396. It is the
> responsibility of the caller to encode any fields, which need to be escaped
> prior to calling URL, and also to decode any escaped fields, that are
> returned from URL.
>
> This has lead to many issues in the past. Indeed, if used improperly, there
> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
> URL string that can be parsed back into the same URL. This can lead to
> constructing misleading URLs. Another issue is with `equals()` and
> `hashCode()` which may have to perform a lookup, and do not take
> encoding/escaping into account.
>
> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
> of the shortcoming of `java.net.URL`. Conversion methods to create a URL from
> a URI were also added. However, it was left up to the developers to use
> `java.net.URI`, or not. This RFE proposes to deprecate all public
> constructors of `java.net.URL`, in order to provide a stronger warning about
> their potential misuses. To construct a URL, using `URI::toURL` should be
> preferred.
>
> In order to provide an alternative to the constructors that take a stream
> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
> java.net.URLStreamHandler)` is provided as part of this change.
>
> Places in the JDK code base that were constructing `java.net.URL` have been
> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
> issues will be logged to revisit the calling code.
>
> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
src/java.base/share/classes/java/net/JarURLConnection.java line 177:
> 175: @SuppressWarnings("deprecation")
> 176: var tmp = jarFileURL = new URL(spec.substring(0, separator++));
> 177:
I realise that @SuppressWarnings needs a declaration here. I wonder if we could
agree a better name for the unused variable name though, like `unusedSW` or
something better?
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8295470: Update openjdk.java.net => openjdk.org URLs in test code
On Tue, 18 Oct 2022 11:55:06 GMT, Magnus Ihse Bursie wrote: > This is a continuation of the effort to update all our URLs to the new > top-level domain. > > This patch updates (most) URLs in testing code. There still exists references > to openjdk.java.net, but that are not strictly used as normal URLs, which I > deemed need special care, so I left them out of this one, which is more of a > straight-forward search and replace. > > I have manually verified that the links work (or points to bugs.openjdk.org > and looks sane; I did not click on all those). I have replaced `http` with > `https`. I have replaced links to specific commits on the mercurial server > with links to the corresponding commits in the new git repos. net changes look fine - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.org/jdk/pull/10744
Re: RFR: 8289768: Clean up unused code [v3]
On Fri, 8 Jul 2022 07:08:46 GMT, Daniel Jeliński wrote: >> This patch removes many unused variables and one unused label reported by >> the compilers when relevant warnings are enabled. >> >> The unused code was found by compiling after removing `unused` from the list >> of disabled warnings for >> [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) >> and >> [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), >> and enabling >> [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) >> MSVC warning. >> >> I only removed variables that were uninitialized or initialized without side >> effects. I verified that the removed variables were not used in any >> `#ifdef`'d code. I checked that the changed code still compiles on Windows, >> Linux and Mac, both in release and debug versions. > > Daniel Jeliński has updated the pull request incrementally with two > additional commits since the last revision: > > - Update copyright > - Remove more unused variables libnet changes look fine to me - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.org/jdk/pull/9383
Re: RFR: JDK-8288746: HttpClient resources could be reclaimed more eagerly
On Mon, 20 Jun 2022 14:09:27 GMT, Daniel Fuchs wrote: > Hi, > > Please find here a patch that should help the HttpClient's SelectorManager > thread to terminate more timely, allowing the resources consumed by the > client to be released earlier. > > The idea is to use a Cleaner registered with the HttpClientFacade to wakeup > the Selector when the facade is being gc'ed. > Some tests have been modified to wait for the selector manager thread to > shutdown, and some of them have been observed to timeout when the fix is not > in place. LGTM - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.org/jdk/pull/9217
