Re: RFR: 8287663 Add a regression test for JDK-8287073
On Thu, 2 Jun 2022 14:32:28 GMT, Severin Gehwolf wrote: > This adds a regression test for a recent fix (JDK-8287073). I've restructured > the linux specific JDK code to call a separate static function to enable this > test. It'll help future tests too. > > Testing: > - [x] Container tests continue to pass + GHA > - [x] New regression test fails prior the code fix of JDK-8287073 and passes > with it @jerboaa Thanks for taking care of the test! I'm not a reviewer, but FWIW the test looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/8993
Integrated: 8287073: NPE from CgroupV2Subsystem.getInstance()
On Fri, 20 May 2022 08:34:58 GMT, Maxim Kartashev wrote: > Following the logic from the comment directly above the changed line, since > it doesn't matter which controller we pick, pick any available controller > instead of the one called "memory" specifically. This way we are guarded > against getting `null` as `anyController`, which is being immediately passed > down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept `null` > values. > > It is also worth noting that the previous checks (such as that at line 89) > make sure that there exist at least one controller in the map. This pull request has now been integrated. Changeset: 744b822a Author:Maxim Kartashev Committer: Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/744b822ab194a0f7ef4e7a4053be32c6a0889efc Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod 8287073: NPE from CgroupV2Subsystem.getInstance() Reviewed-by: vkempik, iklam - PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v2]
On Thu, 26 May 2022 15:25:32 GMT, Ioi Lam wrote:
>> Maxim Kartashev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed unnecessary null checks
>
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
> line 113:
>
>> 111: CgroupInfo anyController = infos.values().iterator().next();
>> 112: CgroupSubsystem subsystem =
>> CgroupV2Subsystem.getInstance(anyController);
>> 113: return new CgroupMetrics(subsystem);
>
> Should add `Objects.requireNonNull(anyController)` and
> `Objects.requireNonNull(subsystem)`.
I added the first, but not the second as that looks like an overkill; see the
definition of the constructor:
CgroupMetrics(CgroupSubsystem subsystem) {
this.subsystem = Objects.requireNonNull(subsystem);
}
-
PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v2]
On Thu, 26 May 2022 15:23:35 GMT, Ioi Lam wrote:
>> Maxim Kartashev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed unnecessary null checks
>
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
> line 116:
>
>> 114: } else {
>> 115: CgroupV1Subsystem subsystem =
>> CgroupV1Subsystem.getInstance(infos);
>> 116: return new CgroupV1MetricsImpl(subsystem);
>
> This shouldn't be changed because the current implementation of
> `CgroupV1Subsystem.getInstance(infos)` has a path that returns null.
>
> Maybe that's impossible, because when we call
> `CgroupV1Subsystem.getInstance`, we must have at least one v1 subsystem in
> `infos`. However, that's not related to this issue. Please fix that in a
> separate RFE. For example, `CgroupV1Subsystem.getInstance(infos)` can be
> changed to throw an exception instead if return null.
Fine by me; done.
-
PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v3]
> Following the logic from the comment directly above the changed line, since > it doesn't matter which controller we pick, pick any available controller > instead of the one called "memory" specifically. This way we are guarded > against getting `null` as `anyController`, which is being immediately passed > down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept `null` > values. > > It is also worth noting that the previous checks (such as that at line 89) > make sure that there exist at least one controller in the map. Maxim Kartashev has updated the pull request incrementally with one additional commit since the last revision: Made requested changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/8803/files - new: https://git.openjdk.java.net/jdk/pull/8803/files/932ff6d1..9e6c0f37 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8803=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8803=01-02 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8803.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8803/head:pull/8803 PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v2]
> Following the logic from the comment directly above the changed line, since > it doesn't matter which controller we pick, pick any available controller > instead of the one called "memory" specifically. This way we are guarded > against getting `null` as `anyController`, which is being immediately passed > down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept `null` > values. > > It is also worth noting that the previous checks (such as that at line 89) > make sure that there exist at least one controller in the map. Maxim Kartashev has updated the pull request incrementally with one additional commit since the last revision: Removed unnecessary null checks - Changes: - all: https://git.openjdk.java.net/jdk/pull/8803/files - new: https://git.openjdk.java.net/jdk/pull/8803/files/f17af433..932ff6d1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8803=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8803=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8803.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8803/head:pull/8803 PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v2]
On Thu, 26 May 2022 06:28:22 GMT, Peter Levart wrote:
>> @plevart Are you asking about the reason for the crash or about the changes?
>> If it's the former, then I believe that the crash comes not from
>> `getInstance()` returning `null`, but from further down the stack because
>> `null` is being passed to `getInstance()`. I could be wrong in interpreting
>> the report, though.
>>
>> If the question's about the changes, then those are restricted to CgroupV2,
>> so I'm not sure how `CgroupV1Subsystem.getInstance(...)` returning null is
>> related. FWIW, I also don't think we are going to get here if there are no
>> active controllers. There's this code a few lines above:
>>
>> if (!result.isAnyControllersEnabled()) {
>> return null;
>> }
>
> I was just contemplating the code around the change as it appears to have
> unnecessary checks which result in dead code. From the point of fixing just
> this concrete NPE, they are irrelevant. So while this code might benefit from
> cleanup, perhaps this PR is not the place to do it. Perhaps it is a matter of
> another issue and PR.
@plevart I think I now understand what you meant and removed the unnecessary
checks. Please, have a look.
-
PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance()
On Wed, 25 May 2022 14:47:06 GMT, Peter Levart wrote:
>> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
>> line 113:
>>
>>> 111: CgroupInfo anyController =
>>> infos.values().iterator().next();
>>> 112: CgroupSubsystem subsystem =
>>> CgroupV2Subsystem.getInstance(anyController);
>>> 113: return subsystem != null ? new CgroupMetrics(subsystem) :
>>> null;
>>
>> Looking at implementation of CgroupV2Subsystem.getInstance(...), it seems
>> that it always returns != null ...
>
> `CgroupV1Subsystem.getInstance(...)` also claims that it never returns
> `null`, but has a code-path that actually returns `null` (when there is no
> active controller). Is this a possible outcome?
@plevart Are you asking about the reason for the crash or about the changes?
If it's the former, then I believe that the crash comes not from
`getInstance()` returning `null`, but from further down the stack because
`null` is being passed to `getInstance()`. I could be wrong in interpreting the
report, though.
If the question's about the changes, then those are restricted to CgroupV2, so
I'm not sure how `CgroupV1Subsystem.getInstance(...)` returning null is
related. FWIW, I also don't think we are going to get here if there are no
active controllers. There's this code a few lines above:
if (!result.isAnyControllersEnabled()) {
return null;
}
-
PR: https://git.openjdk.java.net/jdk/pull/8803
RFR: 8287073: NPE from CgroupV2Subsystem.getInstance()
Following the logic from the comment directly above the changed line, since it doesn't matter which controller we pick, pick any available controller instead of the one called "memory" specifically. This way we are guarded against getting `null` as `anyController`, which is being immediately passed down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept `null` values. It is also worth noting that the previous checks (such as that at line 89) make sure that there exist at least one controller in the map. - Commit messages: - 8287073: NPE from CgroupV2Subsystem.getInstance() Changes: https://git.openjdk.java.net/jdk/pull/8803/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8803=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287073 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8803.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8803/head:pull/8803 PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]
On Thu, 12 May 2022 04:25:24 GMT, David Holmes wrote: >> This change seem to have made this group of declarations obsolete: >> `src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h:157` (follow >> the [link]( >> https://github.com/openjdk/jdk/blob/89de756ffbefac452c7df559e2a4eb50bf71368b/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L157)). >> Does anyone have plans to drop those? Have any bugs been filed for that? If >> not, I'll attend to that myself. > > @mkartashev all references to WINVER in the AWT code seem obsolete. Possibly > most of the IS_WINxxx usages could also be replaced. @dholmes-ora @MBaesken Thank you! Filed [JDK-8286634](https://bugs.openjdk.java.net/browse/JDK-8286634). - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]
On Wed, 4 May 2022 08:00:08 GMT, Matthias Baesken wrote: >> Currently we set _WIN32_WINNT at various places in the codebase; this is >> used to target a minimum Windows version we want to support. See also for >> more detailled information : >> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt >> Macros for Conditional Declarations >> "Certain functions that depend on a particular version of Windows are >> declared using conditional code. This enables you to use the compiler to >> detect whether your application uses functions that are not supported on its >> target version(s) of Windows." >> >> However currently we have quite a lot of differing settings of _WIN32_WINNT >> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible >> would make sense because we have this setting already at java_props_md.c >> (so targeting older Windows versions at other places is most likely not >> useful). > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust API level to Windows 8 for security.cpp and do some cleanup This change seem to have made this group of declarations obsolete: `src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h:157` (follow the [link]( https://github.com/openjdk/jdk/blob/89de756ffbefac452c7df559e2a4eb50bf71368b/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L157)). Does anyone have plans to drop those? Have any bugs been filed for that? If not, I'll attend to that myself. - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder
On Fri, 4 Mar 2022 23:20:21 GMT, Roger Riggs wrote:
> Quoting related changes in https://bugs.openjdk.java.net/browse/JDK-8250568
> modified the way that
> process builder recognized argument strings, causing some arguments to be
> doubly quoted and malformed.
>
> ProcessBuilder encodes command arguments in two ways, a looser legacy encoding
> and stricter encoding that prevents quotes from being misinterpreted.
> The system property jdk.lang.Process.allowAmbiguousCommands controls which is
> in effect.
>
> When the property is "true" or not set, arguments are inserted into the
> Windows command line
> with minimal changes. Arguments containing space or tab are quoted to
> prevent them being split.
> Arguments that start and end with double-quote are left alone.
> Some executables interpret a backslash before the final quote as an escape;
> if the argument
> contains first and last quotes, backslashes are ignored.
>
> When the allowAmbigousCommands property is `false`, care is taken to ensure
> that
> the final quote of an argument is the closing quote for the argument and is
> not
> interpreted as a literal quote by a preceding quote (or an odd number of
> quotes).
>
> The PR includes a test matrix of the cases where an argument with spaces and
> a final backslash
> is passed with each combination of `allowAmbiguousCommands = true and false`,
> launched executable, java, .cmd, and .vbs and when the argument is surrounded
> with double-quotes.
>
> The priority for allowAmbiguousCommands = false is that no argument is split
> or joined to another argument.
> In some cases, backslashes are doubled to prevent a double-quote from being
> interpreted incorrectly.
> The trailing backslash in an argument occurs rarely exception when the
> argument is a directory.
> In that case, the addition of trailing backslashes is benign when the string
> is used as a filesystem path.
>
> See also PR#7504, for background and a proposal.
src/java.base/windows/classes/java/lang/ProcessImpl.java line 302:
> 300: */
> 301: private static String unQuote(String str) {
> 302: if (!str.startsWith("\"") || !str.endsWith("\"") || str.length()
> < 2)
This method has never been doing what it says in the comment above. Maybe drop
the comment altogether as the code is fairly self-explanatory?
-
PR: https://git.openjdk.java.net/jdk/pull/7709
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v4]
On Mon, 7 Mar 2022 16:46:28 GMT, Roger Riggs wrote:
>> Olga Mikhaltsova has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Reverted addition of the test via echo
>
> As an alternative fix, please take a look at Draft PR:
> https://github.com/openjdk/jdk/pull/7709.
>
> In the default handling of arguments, the check for what is quoted is
> reverted to prior to 8255068. First and last quotes are sufficient to
> identify a "quoted" string. The check for a backslash ("\") is removed.
> This original check is sufficient for
> `jdk.lang.Process.allowAmbiguousCommands = true`.
>
> For the case where the system property
> `jdk.lang.Process.allowAmbiguousCommands = false`
> and the argument has first and last quotes, a backslash ("\") before the
> final quote must not allow the quote to interpreted as a literal quote and
> merge the following argument. The backslashes will doubled to prevent the
> interpretation of the quote as a literal. This is the correct encoding if
> the command uses the ".exe" encoding, when reparsing the arguments the
> doubled backslashes are reduced to the original contents.
> When the command is using the simpler parsing that does not support literal
> quotes, the backslash before the quote is typically is a trailing backslash
> on a file path and in that case the additional backslash is redundant and has
> no effect on the interpretation of the argument as a directory path.
>
> The PR includes a test of the 12 combinations of invoking an "java"/.exe
> program, a .cmd script, and a Visual Basic script (which uses the .exe rules
> but different command line parser); with and without application quotes and
> compares the actual results with the expected arguments.
@RogerRiggs I believe your patch fixes the use case(s) we are interested in.
Would be good to see it merged into `master`.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Mon, 28 Feb 2022 21:30:27 GMT, Roger Riggs wrote:
>> Olga Mikhaltsova has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add a new line to the end of test file for JDK-8282008
>
> (I'm still working on a more nuanced fix that works with .exe, .cmd, and with
> allowAmbiguousCommands both true and false).
>
> The suggested workaround was to remove the application quotes and let
> ProcessBuilder do the quoting.
> That resulted in an extra backslash "\" at the end of a file path. In my
> investigation, the extra "\" doesn't prevent the
> string from being correctly used as a directory path in either VisualBasic or
> cmd.exe.
> So I'm curious, in the original application that uncovered this problem, what
> is/was reported as the error?
> Was the original application retested with the workaround?
>
> The case of the backslash at the end of an argument occurs mainly in a
> directory path.
> Yes, the argument is different, but does it make a difference that matters in
> the context in which it appears.
@RogerRiggs
Our use case was something like this `java -classpath "C:\Program
Files\MySQL\JDBC\" ...`. More specifically, while this works after JDK-8250568
(the string ends with `"`)
ProcessBuilder("java.exe", "-classpath", ""C:\\New folder"", "Test",
"test");
this doesn't and, as I understand, shouldn't (the string ends with `"`):
ProcessBuilder("java.exe", "-classpath", ""C:\\New folder\\"", "Test",
"test");
and produces errors like these
Error: Could not find or load main class folder"" Test test
Caused by: java.lang.ClassNotFoundException: folder"" Test test
However, the following still doesn't work, but, I believe, should (the string
ends with `\"`):
ProcessBuilder("java.exe", "-classpath", ""C:\\New folder"", "Test",
"test");
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
Actually, this change should be made even more generic because the string might
end with any even number of the backslash characters followed by a
free-standing quote, in which case additional quoting should not be required.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
It is apparent there is no one "correct" way to quote, but one of the key
features of the Java ecosystem has been its backwards compatibility. In that
light, this change allows our clients to continue doing what they did the way
they did it without the need for modification of their Java code or their
(maybe even foreign) native code. FWIW.
Separately from the above, I wonder if this change would make the fix less
controversial?
-if (str.endsWith("\\"")) {
+if (str.endsWith("\\"") && !str.endsWith(""")) {
This way we verify that the end quote is really just an escaped quote, while
correctly identifying escaped backslash as having nothing to do with the
following quote.
-
PR: https://git.openjdk.java.net/jdk/pull/7504
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
On Mon, 7 Jun 2021 18:46:11 GMT, Naoto Sato wrote: >> @mkartashev thank you for the detailed explanation. >> >> It is not clear to me that the JDK's conformance to being a Unicode >> application has significantly changed since the evaluation of JDK-8017274 - >> @naotoj can you comment on that and related discussion from the CCC for >> JDK-4958170 ? In particular I'm not sure that using the platform encoding is >> wrong, nor how we can have a path that cannot be represented by the platform >> encoding? >> >> Not being an expert in this area I cannot evaluate the affects of these >> shared code changes on other platforms, and so am reluctant to introduce any >> change that affects any non-Windows platforms. Also the JVM and JNI work >> with modified-UTF8 so I do not think we should diverge from that. >> I would hate to see windows specific code introduced into the JDK or JVM's >> shared code for these APIs, but that may be the only choice to avoid >> potential disruption to other platforms. Though perhaps we could push the >> initial conversion down into the JVM? > > @dholmes-ora Sorry, I don't think anything has changed as to the encoding as > of JDK-8017274. For some reason, I had the impression that JVM_LoadLibrary() > accepts UTF-8 (either modified or standard), but that was not correct. It is > using the platform encoded string for the pathname. > > @mkartashev As you mentioned in another comment, the only way to fix this > issue is to pass UTF-8 down to JVM_LoadLibray, but I don't think it is > feasible. One reason is the effort is too great, and the other is that all VM > implementations would need to be modified. @naotoj Then I guess this bug will have to wait until Windows evolves to the point when its platform encoding is UTF-8. In the mean time, I'm closing this PR. Thank you all so much for your time! - PR: https://git.openjdk.java.net/jdk/pull/4169
Withdrawn: 8195129: System.load() fails to load from unicode paths
On Mon, 24 May 2021 16:43:09 GMT, Maxim Kartashev wrote: > Character strings within JVM are produced and consumed in several formats. > Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() > or dlopen()) consume strings also in UTF8. On Windows, however, the situation > is far less simple: some new(er) APIs expect UTF16 (wide-character strings), > some older APIs can only work with strings in a "platform" format, where not > all UTF8 characters can be represented; which ones can depends on the current > "code page". > > This commit switches the Windows version of native library loading code to > using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use > of various string formats in the surrounding code. > > Namely, exception messages are made to consume strings explicitly in the UTF8 > format, while logging functions (that end up using legacy Windows API) are > made to consume "platform" strings in most cases. One exception is > `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, > which can, of course, be fixed, but was considered not worth the additional > code (NB: this isn't a new bug). > > The test runs in a separate JVM in order to make NIO happy about non-ASCII > characters in the file name; tests are executed with LC_ALL=C and that > doesn't let NIO work with non-ASCII file names even on Linux or MacOS. > > Tested by running `test/hotspot/jtreg:tier1` on Linux and > `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (` > jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran > on those platforms as well. > > Results from Linux: > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/hotspot/jtreg:tier1 1784 1784 0 0 > > == > TEST SUCCESS > > > Building target 'run-test-only' in configuration 'linux-x86_64-server-release' > Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', > will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 > > > Results from Windows 10: > > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/hotspot/jtreg/runtime746 746 0 0 > == > TEST SUCCESS > Finished building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > > > Building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v5]
On Fri, 4 Jun 2021 13:36:27 GMT, Maxim Kartashev wrote: >> Character strings within JVM are produced and consumed in several formats. >> Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() >> or dlopen()) consume strings also in UTF8. On Windows, however, the >> situation is far less simple: some new(er) APIs expect UTF16 (wide-character >> strings), some older APIs can only work with strings in a "platform" format, >> where not all UTF8 characters can be represented; which ones can depends on >> the current "code page". >> >> This commit switches the Windows version of native library loading code to >> using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use >> of various string formats in the surrounding code. >> >> Namely, exception messages are made to consume strings explicitly in the >> UTF8 format, while logging functions (that end up using legacy Windows API) >> are made to consume "platform" strings in most cases. One exception is >> `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, >> which can, of course, be fixed, but was considered not worth the additional >> code (NB: this isn't a new bug). >> >> The test runs in a separate JVM in order to make NIO happy about non-ASCII >> characters in the file name; tests are executed with LC_ALL=C and that >> doesn't let NIO work with non-ASCII file names even on Linux or MacOS. >> >> Tested by running `test/hotspot/jtreg:tier1` on Linux and >> `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (` >> jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran >> on those platforms as well. >> >> Results from Linux: >> >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >> >>jtreg:test/hotspot/jtreg:tier1 1784 1784 0 0 >> >> == >> TEST SUCCESS >> >> >> Building target 'run-test-only' in configuration >> 'linux-x86_64-server-release' >> Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', >> will run: >> * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode >> >> Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' >> Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java >> Test results: passed: 1 >> >> >> Results from Windows 10: >> >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/hotspot/jtreg/runtime746 746 0 0 >> == >> TEST SUCCESS >> Finished building target 'run-test-only' in configuration >> 'windows-x86_64-server-fastdebug' >> >> >> Building target 'run-test-only' in configuration >> 'windows-x86_64-server-fastdebug' >> Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: >> * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode >> >> Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' >> Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java >> Test results: passed: 1 > > Maxim Kartashev has updated the pull request incrementally with one > additional commit since the last revision: > > Updated the test to run on Windows only and to use a character from the > supplementary plane in the path name. I came to realize that changing `os::dll_load()` to accept UTF-8 (standard or modified) will break all the users of that function except `JVM_LoadLibrary()`. Consider `os::native_java_library()` that still operates with the platform encoding on Windows and works correctly if CWD contains Latin-1 characters (assuming 1252 code page). With this change, `java` will fail to start if its path name contains, say, Æ because `os::dll_load()` will expect it to be encoded as `c3 86` (UTF-8), but will get `c6` (Latin-1) instead. One possible solution is to update all the call sites of `os::dll_load()` (quite laborous), another is to introduce `os::dll_load_utf8()` and change only `JVM_LoadLibrary()` at this point in time. Advice is welcome. - PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
On Sun, 6 Jun 2021 22:25:44 GMT, David Holmes wrote: >> I think we need to establish some common ground before proceeding further >> with this fix. It's a bit of a long read; please, bear with me. >> >> The path name starts its life as a `jstring` in >> `Java_jdk_internal_loader_NativeLibraries_load()`, its encoding is >> irrelevant at this point. >> >> Next, the name has to be passed down to `JVM_LoadLibrary()` that takes >> `char*`. So we need to convert form `jstring` to `char*` (point (a)). >> Following that, `os::dll_load()` that actually performs loading in a >> platform-specific manner also receives `char*`. All platform implementations >> of `os::dll_load()` pass the path name down to their respective platform's >> APIs unmodified, but I think that's just incidental and here we have another >> possible point of conversion (point (b)). Other consumers of the path name >> are exception(c) and logging(d) messages; they also take `char*`, but >> potentially of a different encoding. >> >> Let me try to enumerate all conceivably valid conversions for >> `JVM_LoadLibrary()` consumption (point (a)): >> 1. jstring -> platform-specific encoding (status quo meaning possibly lossy >> encoding on Windows and UTF-8 elsewhere AFAICT), >> 2. jstring -> modified UTF-8, >> 3. jstring -> UTF-8. >> >> This bug [8195129](https://bugs.openjdk.java.net/browse/JDK-8195129) occurs >> because conversion (1) may loose information on Windows if the platform >> encoding happens to be NOT UTF-8 (which it often - or even always - is). So >> that's a no-go and we are left with either (2) or (3). >> >> On MacOS and Linux, "platform" encoding already is UTF-8 and since all the >> platform APIs happily consume UTF-8, no further conversion is necessary >> (neither for actual library loading, nor for log or exception messages; the >> latter have to convert to UTF-16, but do that under the hood). >> >> On Windows, we require at least these variants of the path name: >> 1. UTF16 for library loading (Unicode Windows API), >> 2. "platform" encoding for logging (yes, loosing information here, but >> that's tolerable), >> 3. "platform" (lossy) or UTF8 (lossless) encoding for exception messages >> (prefer lossless). >> >> This is what's behind my choice of UTF-8 for the path name encoding as it >> gets passed down to `JVM_LoadLibrary()`. We can go with modified UTF-8, of >> course, in which case all platforms - not just Windows - will have to do the >> conversion on their own, loosing the benefit of the knowledge about the >> original string encoding (the String.coder field of jstring). > > @mkartashev thank you for the detailed explanation. > > It is not clear to me that the JDK's conformance to being a Unicode > application has significantly changed since the evaluation of JDK-8017274 - > @naotoj can you comment on that and related discussion from the CCC for > JDK-4958170 ? In particular I'm not sure that using the platform encoding is > wrong, nor how we can have a path that cannot be represented by the platform > encoding? > > Not being an expert in this area I cannot evaluate the affects of these > shared code changes on other platforms, and so am reluctant to introduce any > change that affects any non-Windows platforms. Also the JVM and JNI work with > modified-UTF8 so I do not think we should diverge from that. > I would hate to see windows specific code introduced into the JDK or JVM's > shared code for these APIs, but that may be the only choice to avoid > potential disruption to other platforms. Though perhaps we could push the > initial conversion down into the JVM? > I think I am hesitant to change the JVM interface from modified UTF-8 to > standard UTF-8, AFAICT all platforms except Windows already use standard UTF-8 on that path (from `Java_jdk_internal_loader_NativeLibraries_load()` to `JVM_LoadLibrary()`) because the "platform" encoding for those happens to be "UTF-8". So at the current stage this patch actually maintains status quo for all platforms except Windows, the only platform where the bug exists. But I am not against changing the encoding to modified UTF-8 and updating os::dll_load() for all platforms. Just wanted to have some consensus before proceeding with that change. - PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
On Fri, 4 Jun 2021 02:12:42 GMT, David Holmes wrote: >> I am not sure we can pass non `modified UTF-8` through `JVM_LoadLibrary()`. >> Probably some VM folks can enlighten here? > > Not an expert by my understanding is that the VM only deals with modified > UTF-8, as does JNI. So the incoming string should be modified-UTF8 IMO and > then converted to UTF16. > > That said, this is shared code being modified on the JDK side so you can't > just change the type of string being passed in without updating all the > implementations of os::dll_load to support that! I think we need to establish some common ground before proceeding further with this fix. It's a bit of a long read; please, bear with me. The path name starts its life as a `jstring` in `Java_jdk_internal_loader_NativeLibraries_load()`, its encoding is irrelevant at this point. Next, the name has to be passed down to `JVM_LoadLibrary()` that takes `char*`. So we need to convert form `jstring` to `char*` (point (a)). Following that, `os::dll_load()` that actually performs loading in a platform-specific manner also receives `char*`. All platform implementations of `os::dll_load()` pass the path name down to their respective platform's APIs unmodified, but I think that's just incidental and here we have another possible point of conversion (point (b)). Other consumers of the path name are exception(c) and logging(d) messages; they also take `char*`, but potentially of a different encoding. Let me try to enumerate all conceivably valid conversions for `JVM_LoadLibrary()` consumption (point (a)): 1. jstring -> platform-specific encoding (status quo meaning possibly lossy encoding on Windows and UTF-8 elsewhere AFAICT), 2. jstring -> modified UTF-8, 3. jstring -> UTF-8. This bug [8195129](https://bugs.openjdk.java.net/browse/JDK-8195129) occurs because conversion (1) may loose information on Windows if the platform encoding happens to be NOT UTF-8 (which it often - or even always - is). So that's a no-go and we are left with either (2) or (3). On MacOS and Linux, "platform" encoding already is UTF-8 and since all the platform APIs happily consume UTF-8, no further conversion is necessary (neither for actual library loading, nor for log or exception messages; the latter have to convert to UTF-16, but do that under the hood). On Windows, we require at least these variants of the path name: 1. UTF16 for library loading (Unicode Windows API), 2. "platform" encoding for logging (yes, loosing information here, but that's tolerable), 3. "platform" (lossy) or UTF8 (lossless) encoding for exception messages (prefer lossless). This is what's behind my choice of UTF-8 for the path name encoding as it gets passed down to `JVM_LoadLibrary()`. We can go with modified UTF-8, of course, in which case all platforms - not just Windows - will have to do the conversion on their own, loosing the benefit of the knowledge about the original string encoding (the String.coder field of jstring). - PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v5]
> Character strings within JVM are produced and consumed in several formats. > Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() > or dlopen()) consume strings also in UTF8. On Windows, however, the situation > is far less simple: some new(er) APIs expect UTF16 (wide-character strings), > some older APIs can only work with strings in a "platform" format, where not > all UTF8 characters can be represented; which ones can depends on the current > "code page". > > This commit switches the Windows version of native library loading code to > using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use > of various string formats in the surrounding code. > > Namely, exception messages are made to consume strings explicitly in the UTF8 > format, while logging functions (that end up using legacy Windows API) are > made to consume "platform" strings in most cases. One exception is > `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, > which can, of course, be fixed, but was considered not worth the additional > code (NB: this isn't a new bug). > > The test runs in a separate JVM in order to make NIO happy about non-ASCII > characters in the file name; tests are executed with LC_ALL=C and that > doesn't let NIO work with non-ASCII file names even on Linux or MacOS. > > Tested by running `test/hotspot/jtreg:tier1` on Linux and > `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (` > jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran > on those platforms as well. > > Results from Linux: > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/hotspot/jtreg:tier1 1784 1784 0 0 > > == > TEST SUCCESS > > > Building target 'run-test-only' in configuration 'linux-x86_64-server-release' > Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', > will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 > > > Results from Windows 10: > > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/hotspot/jtreg/runtime746 746 0 0 > == > TEST SUCCESS > Finished building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > > > Building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 Maxim Kartashev has updated the pull request incrementally with one additional commit since the last revision: Updated the test to run on Windows only and to use a character from the supplementary plane in the path name. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4169/files - new: https://git.openjdk.java.net/jdk/pull/4169/files/97c918ca..a5d45dca Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4169=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4169=03-04 Stats: 8 lines in 2 files changed: 1 ins; 5 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4169.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4169/head:pull/4169 PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
On Thu, 3 Jun 2021 17:51:54 GMT, Naoto Sato wrote: > I think we can simply limit the test platform only to Windows in @requires > tag in the test. Also, I would see the test case using some supplementary > characters. Done. - PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
On Tue, 1 Jun 2021 18:42:34 GMT, Naoto Sato wrote:
>> Maxim Kartashev has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Coding style-related corrections.
>> - Corrected the test to use Platform.sharedLibraryExt()
>
> test/hotspot/jtreg/runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java
> line 42:
>
>> 40: String nativePathSetting = "-Dtest.nativepath=" +
>> getSystemProperty("test.nativepath");
>> 41: ProcessBuilder pb =
>> ProcessTools.createTestJvm(nativePathSetting,
>> LoadLibraryUnicode.class.getName());
>> 42: pb.environment().put("LC_ALL", "en_US.UTF-8");
>
> Some environments/user configs may not have `UTF-8` codeset on the platform.
> May need to gracefully exit in such a case.
I added `java.nio.charset.Charset.isSupported("UTF-8")` check to the test. Hope
that's enough for the environments without `UTF-8`.
-
PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
On Tue, 1 Jun 2021 18:24:05 GMT, Naoto Sato wrote:
>> Maxim Kartashev has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Coding style-related corrections.
>> - Corrected the test to use Platform.sharedLibraryExt()
>
> src/hotspot/os/windows/os_windows.cpp line 1491:
>
>> 1489: static errno_t convert_UTF8_to_UTF16(char const* utf8_str, LPWSTR*
>> utf16_str) {
>> 1490: return convert_to_UTF16(utf8_str, CP_UTF8, utf16_str);
>> 1491: }
>
> IIUC, `utf8_str` is the "modified" UTF-8 string in JNI. Using it as the
> standard UTF-8 (I believe Windows' encoding `CP_UTF8` is the one) may end up
> in incorrect conversions in some corner cases, e.g., for supplementary
> characters.
Right; I changed the code in NativeLibraries.c to pass down true UTF-8 instead
of "modified UTF-8". Please, take a look.
-
PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v4]
> Character strings within JVM are produced and consumed in several formats. > Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() > or dlopen()) consume strings also in UTF8. On Windows, however, the situation > is far less simple: some new(er) APIs expect UTF16 (wide-character strings), > some older APIs can only work with strings in a "platform" format, where not > all UTF8 characters can be represented; which ones can depends on the current > "code page". > > This commit switches the Windows version of native library loading code to > using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use > of various string formats in the surrounding code. > > Namely, exception messages are made to consume strings explicitly in the UTF8 > format, while logging functions (that end up using legacy Windows API) are > made to consume "platform" strings in most cases. One exception is > `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, > which can, of course, be fixed, but was considered not worth the additional > code (NB: this isn't a new bug). > > The test runs in a separate JVM in order to make NIO happy about non-ASCII > characters in the file name; tests are executed with LC_ALL=C and that > doesn't let NIO work with non-ASCII file names even on Linux or MacOS. > > Tested by running `test/hotspot/jtreg:tier1` on Linux and > `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (` > jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran > on those platforms as well. > > Results from Linux: > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/hotspot/jtreg:tier1 1784 1784 0 0 > > == > TEST SUCCESS > > > Building target 'run-test-only' in configuration 'linux-x86_64-server-release' > Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', > will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 > > > Results from Windows 10: > > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/hotspot/jtreg/runtime746 746 0 0 > == > TEST SUCCESS > Finished building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > > > Building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 Maxim Kartashev has updated the pull request incrementally with one additional commit since the last revision: Updated Java_jdk_internal_loader_NativeLibraries_load() to use a new helper function to get the library name encoded as true UTF-8 (not in "modified UTF-8" form). Also updated the test to automatically pass if "UTF-8" is not supported by NIO on the platform. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4169/files - new: https://git.openjdk.java.net/jdk/pull/4169/files/c8ef8b64..97c918ca Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4169=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4169=02-03 Stats: 71 lines in 4 files changed: 57 ins; 6 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/4169.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4169/head:pull/4169 PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v2]
On Thu, 27 May 2021 05:13:44 GMT, David Holmes wrote:
>> Maxim Kartashev has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR. The pull request
>> contains one new commit since the last revision:
>>
>> 8195129: System.load() fails to load from unicode paths
>
> src/hotspot/os/windows/os_windows.cpp line 1541:
>
>> 1539: void * os::dll_load(const char *utf8_name, char *ebuf, int ebuflen) {
>> 1540: LPWSTR utf16_name = NULL;
>> 1541: errno_t err = convert_UTF8_to_UTF16(utf8_name, _name);
>
> Do you have any figures on the cost of this additional conversion in relation
> to startup time?
>
> I'm already concerned to see that we have to perform each conversion twice
> via MultiByteToWideChar/WideCharToMultiByte, once to get the size and then to
> actually get the characters! This seems potentially very inefficient.
I measured time spend converting the library path name relative to the overall
time of a (successful) `os::dll_load()` call. It varies between a fraction of a
percent to ~3% (see below for the actual data from a `release` build). I'll
defer to your expertise wrt to these numbers.
What can be done here (without changing os::dll_load() API) is to have a "fast
path" for ascii-only path names, in which case no conversion is necessary, and
a "slow path" for all the rest. This will complicate things a bit, but not by
much, I believe. I am certainly willing to give that a try. Let me know what do
you think.
./build/windows-x86_64-server-release/images/jdk/bin/java -jar
./build/windows-x86_64-server-fastdebug/images/jdk/demo/jfc/SwingSet2/SwingSet2.jar
0.050% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\jimage.dll
2.273% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\java.dll
0.177% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\net.dll
0.541% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\nio.dll
0.359% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\zip.dll
3.186% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\jimage.dll
0.075% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\awt.dll
0.232% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\freetype.dll
0.136% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\fontmanager.dll
0.170% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\javajpeg.dll
-
PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v2]
On Thu, 27 May 2021 05:18:50 GMT, David Holmes wrote:
>> Maxim Kartashev has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR. The pull request
>> contains one new commit since the last revision:
>>
>> 8195129: System.load() fails to load from unicode paths
>
> test/hotspot/jtreg/runtime/jni/loadLibraryUnicode/LoadLibraryUnicode.java
> line 48:
>
>> 46: } else {
>> 47: throw new Error("Unsupported OS");
>> 48: }
>
> Please use the test library function `Platform.sharedLibraryExt()` to get the
> library file extension.
Thanks for the suggestion. Rewrote this piece.
(Side note: since the libraries' prefix differs between platforms also, it'd be
nice to have something like `Platform.sharedLibraryName(name)`; this is the way
all the code that uses `Platform.sharedLibraryExt()` is structured anyway. But
I guess it's best not to conflate things).
-
PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v2]
On Thu, 27 May 2021 04:23:16 GMT, David Holmes wrote: >> Maxim Kartashev has refreshed the contents of this pull request, and >> previous commits have been removed. The incremental views will show >> differences compared to the previous content of the PR. The pull request >> contains one new commit since the last revision: >> >> 8195129: System.load() fails to load from unicode paths > > src/hotspot/os/windows/os_windows.cpp line 1462: > >> 1460: const int flag_source_str_is_null_terminated = -1; >> 1461: const int flag_estimate_chars_count = 0; >> 1462: int utf16_chars_count_estimated = >> MultiByteToWideChar(source_encoding, > > Your local naming style is somewhat excessive. You could just comment the > values of the flags when you pass them eg: > > MultiByteToWideChar(source_encoding, > MB_ERR_INVALID_CHARS, >source_str, >-1, //source is null-terminated > NULL, // no output buffer > 0); // calculate required buffer size > > Or you could just add a comment before the call: > > // Perform a dummy conversion so that we can get the required size of the > buffer to > // allocate. The source is null-terminated. > > Trying to document parameter semantics by variable naming doesn't work in my > opinion - at some point if you want to know you have to RTFM for the API. > > And utf16_len is perfectly adequate for the returned size. Fair enough. Corrected. - PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
> Character strings within JVM are produced and consumed in several formats. > Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() > or dlopen()) consume strings also in UTF8. On Windows, however, the situation > is far less simple: some new(er) APIs expect UTF16 (wide-character strings), > some older APIs can only work with strings in a "platform" format, where not > all UTF8 characters can be represented; which ones can depends on the current > "code page". > > This commit switches the Windows version of native library loading code to > using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use > of various string formats in the surrounding code. > > Namely, exception messages are made to consume strings explicitly in the UTF8 > format, while logging functions (that end up using legacy Windows API) are > made to consume "platform" strings in most cases. One exception is > `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, > which can, of course, be fixed, but was considered not worth the additional > code (NB: this isn't a new bug). > > The test runs in a separate JVM in order to make NIO happy about non-ASCII > characters in the file name; tests are executed with LC_ALL=C and that > doesn't let NIO work with non-ASCII file names even on Linux or MacOS. > > Tested by running `test/hotspot/jtreg:tier1` on Linux and > `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (` > jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran > on those platforms as well. > > Results from Linux: > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/hotspot/jtreg:tier1 1784 1784 0 0 > > == > TEST SUCCESS > > > Building target 'run-test-only' in configuration 'linux-x86_64-server-release' > Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', > will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 > > > Results from Windows 10: > > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/hotspot/jtreg/runtime746 746 0 0 > == > TEST SUCCESS > Finished building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > > > Building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 Maxim Kartashev has updated the pull request incrementally with two additional commits since the last revision: - Coding style-related corrections. - Corrected the test to use Platform.sharedLibraryExt() - Changes: - all: https://git.openjdk.java.net/jdk/pull/4169/files - new: https://git.openjdk.java.net/jdk/pull/4169/files/fe661dff..c8ef8b64 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4169=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4169=01-02 Stats: 43 lines in 2 files changed: 7 ins; 9 del; 27 mod Patch: https://git.openjdk.java.net/jdk/pull/4169.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4169/head:pull/4169 PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v2]
> Character strings within JVM are produced and consumed in several formats. > Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() > or dlopen()) consume strings also in UTF8. On Windows, however, the situation > is far less simple: some new(er) APIs expect UTF16 (wide-character strings), > some older APIs can only work with strings in a "platform" format, where not > all UTF8 characters can be represented; which ones can depends on the current > "code page". > > This commit switches the Windows version of native library loading code to > using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use > of various string formats in the surrounding code. > > Namely, exception messages are made to consume strings explicitly in the UTF8 > format, while logging functions (that end up using legacy Windows API) are > made to consume "platform" strings in most cases. One exception is > `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, > which can, of course, be fixed, but was considered not worth the additional > code (NB: this isn't a new bug). > > The test runs in a separate JVM in order to make NIO happy about non-ASCII > characters in the file name; tests are executed with LC_ALL=C and that > doesn't let NIO work with non-ASCII file names even on Linux or MacOS. > > Tested by running `test/hotspot/jtreg:tier1` on Linux and > `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (` > jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran > on those platforms as well. > > Results from Linux: > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/hotspot/jtreg:tier1 1784 1784 0 0 > > == > TEST SUCCESS > > > Building target 'run-test-only' in configuration 'linux-x86_64-server-release' > Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', > will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 > > > Results from Windows 10: > > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/hotspot/jtreg/runtime746 746 0 0 > == > TEST SUCCESS > Finished building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > > > Building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 Maxim Kartashev has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8195129: System.load() fails to load from unicode paths - Changes: - all: https://git.openjdk.java.net/jdk/pull/4169/files - new: https://git.openjdk.java.net/jdk/pull/4169/files/265f7907..fe661dff Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4169=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4169=00-01 Stats: 55 lines in 3 files changed: 24 ins; 1 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/4169.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4169/head:pull/4169 PR: https://git.openjdk.java.net/jdk/pull/4169
RFR: 8195129: System.load() fails to load from unicode paths
Character strings within JVM are produced and consumed in several formats. Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() or dlopen()) consume strings also in UTF8. On Windows, however, the situation is far less simple: some new(er) APIs expect UTF16 (wide-character strings), some older APIs can only work with strings in a "platform" format, where not all UTF8 characters can be represented; which ones can depends on the current "code page". This commit switches the Windows version of native library loading code to using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use of various string formats in the surrounding code. Namely, exception messages are made to consume strings explicitly in the UTF8 format, while logging functions (that end up using legacy Windows API) are made to consume "platform" strings in most cases. One exception is `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, which can, of course, be fixed, but was considered not worth the additional code (NB: this isn't a new bug). The test runs in a separate JVM in order to make NIO happy about non-ASCII characters in the file name; tests are executed with LC_ALL=C and that doesn't let NIO work with non-ASCII file names even on Linux or MacOS. Tested by running `test/hotspot/jtreg:tier1` on Linux and `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (` jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran on those platforms as well. Results from Linux: Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/hotspot/jtreg:tier1 1784 1784 0 0 == TEST SUCCESS Building target 'run-test-only' in configuration 'linux-x86_64-server-release' Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java Test results: passed: 1 Results from Windows 10: Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/hotspot/jtreg/runtime746 746 0 0 == TEST SUCCESS Finished building target 'run-test-only' in configuration 'windows-x86_64-server-fastdebug' Building target 'run-test-only' in configuration 'windows-x86_64-server-fastdebug' Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java Test results: passed: 1 - Commit messages: - 8195129: System.load() fails to load from unicode paths Changes: https://git.openjdk.java.net/jdk/pull/4169/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4169=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8195129 Stats: 294 lines in 7 files changed: 247 ins; 27 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/4169.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4169/head:pull/4169 PR: https://git.openjdk.java.net/jdk/pull/4169
