RE: [EXTERNAL] JDK-8234076 bug fix candidate
Thank you Henry for fixing the test issue, and thank you Kumar for the prompt
review! In hindsight I should've run the tests on MacOS too, I'll make sure I
do that for future work.
I truly appreciate the help you've given me in making this bug fix.
Cheers,
Nikola
-Original Message-
From: Kumar Srinivasan
Sent: December 12, 2019 8:32 AM
To: Henry Jen
Cc: core-libs-dev@openjdk.java.net; Nikola Grcevski
Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
Hi Henry,
I approve this. Appreciate you pushing it, as usual *all* possible tests need
to be run. ;)
Nikola, welcome to the OpenJDK community.
Thanks
Kumar
> On Dec 11, 2019, at 10:29 PM, Henry Jen wrote:
>
> Nikola,
>
> The change looks fine to me as well, I’ll run the test and push it before
> RDP1 if everything is good with Kumar as reviewer.
>
> Cheers,
> Henry
>
>
>> On Dec 11, 2019, at 2:26 PM, Kumar Srinivasan wrote:
>>
>> Hi Nikola,
>>
>> Thank you for making the changes. Looks good to me.
>>
>> Kumar Srinivasan
>> PS: my new and official email address: kusriniva...@vmware.com
>>
>>
>> On Wed, Dec 11, 2019 at 10:04 AM Nikola Grcevski
>> wrote:
>> Thank you again for reviewing my code Kumar!
>>
>> I have applied your refactoring suggestions. Using the array approach as you
>> suggested made the test code a lot more tidier. I’m attaching the updated
>> patch after my signature and the external webrev is here for reference:
>>
>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrcevski.github.io%2FJDK-8234076%2Fwebrev%2Fdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086603237sdata=i9pxLqzp2zCmsMbFDB56Aimdq1VVbYxjeaP%2ByBtNA3w%3Dreserved=0
>>
>>
>> Cheers,
>> Nikola
>>
>> diff -r bd436284147d src/java.base/share/native/libjli/args.c
>> --- a/src/java.base/share/native/libjli/args.c Wed Nov 20 08:12:14 2019
>> +0800
>> +++ b/src/java.base/share/native/libjli/args.c Wed Dec 11 12:09:29 2019
>> -0500
>> @@ -130,6 +130,8 @@
>> }
>> } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
>> stopExpansion = JNI_TRUE;
>> +} else if (JLI_StrCCmp(arg, "--module=") == 0) {
>> +idx = argsCount;
>> }
>> } else {
>> if (!expectingNoDashArg) {
>> @@ -449,6 +451,7 @@
>> return JLI_StrCmp(arg, "-jar") == 0 ||
>>JLI_StrCmp(arg, "-m") == 0 ||
>>JLI_StrCmp(arg, "--module") == 0 ||
>> + JLI_StrCCmp(arg, "--module=") == 0 ||
>>JLI_StrCmp(arg, "--dry-run") == 0 ||
>>JLI_StrCmp(arg, "-h") == 0 ||
>>JLI_StrCmp(arg, "-?") == 0 ||
>> diff -r bd436284147d src/java.base/windows/native/libjli/java_md.c
>> --- a/src/java.base/windows/native/libjli/java_md.c Wed Nov 20 08:12:14
>> 2019 +0800
>> +++ b/src/java.base/windows/native/libjli/java_md.c Wed Dec 11 12:09:29
>> 2019 -0500
>> @@ -34,6 +34,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>> #include "java.h"
>> @@ -1015,6 +1016,17 @@
>>
>> // sanity check, match the args we have, to the holy grail
>> idx = JLI_GetAppArgIndex();
>> +
>> +// First arg index is NOT_FOUND
>> +if (idx < 0) {
>> +// The only allowed value should be NOT_FOUND (-1) unless another
>> change introduces
>> +// a different negative index
>> +assert (idx == -1);
>> +JLI_TraceLauncher("Warning: first app arg index not found, %d\n",
>> idx);
>> +JLI_TraceLauncher("passing arguments as-is.\n");
>> +return NewPlatformStringArray(env, strv, argc);
>> +}
>> +
>> isTool = (idx == 0);
>> if (isTool) { idx++; } // skip tool name
>> JLI_TraceLauncher("AppArgIndex: %d points to %s\n", idx,
>> stdargs[idx].arg);
>> diff -r bd436284147d test/jdk/tools/launcher/ArgsEnvVar.java
>> --- a/test/jdk/tools/launcher/ArgsEnvVar.java Wed Nov 20 08:12:14 2019
>> +0800
>> +++ b/test/jdk/tools/launcher/ArgsEnvVar.java Wed Dec 11 12:09:29 2019
>> -0500
>> @@ -37,6 +37,8 @@
>> import java.util.List;
>> import java.util.Map;
>> import java.util.regex.Pattern;
>> +import java.nio.file.Paths;
>> +import java.nio.file.Path;
>>
>> public class Arg
Re: [EXTERNAL] JDK-8234076 bug fix candidate
>> +// We should be able to find the argument --help as an application
>> argument
>> +ProcessTools.executeProcess(
>> +createProcessWithLauncherDebugging(
>> +"--module-path=" + dir,
>> +"--module=" + mid,
>> +"--help"))
>> +.outputTo(System.out)
>> +.errorTo(System.out)
>> +.shouldContain("F--help");
>> +
>> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> <...src/test>/*.java --help
>> +// We should be able to see argument expansion happen
>> +ProcessTools.executeProcess(
>> +createProcessWithLauncherDebugging(
>> +"--module-path=" + dir,
>> +"--module=" + mid,
>> +SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
>> +"--help"))
>> +.outputTo(System.out)
>> +.errorTo(System.out)
>> +.shouldContain("F--help")
>> +.shouldContain("module-info.java");
>> +}
>> +
>> +
>> +/**
>> + * Test that --module= is terminating for VM argument processing just
>> like --module
>> + */
>> +public void testLongFormModuleOptionTermination() throws Exception {
>> +String dir = MODS_DIR.toString();
>> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
>> +
>> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> +// The first --module= will terminate the VM arguments processing.
>> The second pair of module-path and module will be
>> +// deemed as application arguments
>> +OutputAnalyzer output = ProcessTools.executeProcess(
>> +createProcessWithLauncherDebugging(
>> +"--module-path=" + dir,
>> +"--module=" + mid,
>> +"--module-path=" + dir,
>> +"--module=" + mid))
>> +.outputTo(System.out)
>> +.errorTo(System.out)
>> +.shouldContain("argv[ 0] = '--module-path=" + dir)
>> +.shouldContain("argv[ 1] = '--module=" + mid);
>> +
>> +if (IS_WINDOWS) {
>> +output.shouldContain("F--module-path=" +
>> dir).shouldContain("F--module=" + mid);
>> +}
>> +
>> +// java --module=$TESTMODULE/$MAINCLASS --module-path=mods
>> +// This command line will not work as --module= is terminating and
>> the module will be not found
>> +int exitValue = exec("--module=" + mid, "--module-path" + dir);
>> +assertTrue(exitValue != 0);
>> +}
>> }
>>
>> From: Kumar Srinivasan
>> Sent: December 10, 2019 8:43 PM
>> To: Nikola Grcevski
>> Cc: Henry Jen ; Alan Bateman
>> ; core-libs-dev@openjdk.java.net
>> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>>
>> [Resend; cc'ing the group]
>>
>> Hi Nikola,
>>
>> Generally looks great to me. I would have moved initModulesDir to
>> TestHelper.createSimpleModule(File dir).
>> TestHelper.java contains a collection of test related utilities needed by
>> the launcher, it will help someone
>> else use it to create a simple module.
>>
>> I would have rewritten this
>> +ArrayList scratchpad = new ArrayList<>();
>> +scratchpad.add("module test {}");
>> +createFile(new File(testDir, "module-info.java"), scratchpad);
>> +scratchpad.clear();
>> +scratchpad.add("package launcher;");
>> +scratchpad.add("import java.util.Arrays;");
>> +scratchpad.add("public class Main {");
>> +scratchpad.add("public static void main(String[] args) {");
>> +scratchpad.add("System.out.println(Arrays.toString(args));");
>> +scratchpad.add("}");
>> +scratchpad.add("}");
>> +createFile(new File(launcherTestDir, "Main.java"), scratchpad);
>>
>> as follows:
>> String a1[] = {"module test{}"};
>> createFile(new File(testDir, "module-info.java", Arrays.asList(a1));
>>
>> String a2[] = {
>> "package launcher;",
&g
Re: [EXTERNAL] JDK-8234076 bug fix candidate
ot;);
>> +
>> +return pb;
>> +}
>> +
>> + /**
>> + * Test the ability for the Windows launcher to do proper application
>> argument
>> + * detection and expansion, when using the long form module option and
>> all passed in
>> + * command line arguments are prefixed with a dash.
>> + *
>> + * These tests are not expected to work on *nixes, and are ignored.
>> + */
>> +public void testWindowsWithLongFormModuleOption() throws Exception {
>> +if (!IS_WINDOWS) {
>> +return;
>> +}
>> +
>> +String dir = MODS_DIR.toString();
>> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
>> +
>> + // java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
>> +// We should be able to find the argument --help as an application
>> argument
>> +ProcessTools.executeProcess(
>> +createProcessWithLauncherDebugging(
>> +"--module-path=" + dir,
>> +"--module=" + mid,
>> +"--help"))
>> +.outputTo(System.out)
>> +.errorTo(System.out)
>> +.shouldContain("F--help");
>> +
>> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> <...src/test>/*.java --help
>> +// We should be able to see argument expansion happen
>> +ProcessTools.executeProcess(
>> +createProcessWithLauncherDebugging(
>> +"--module-path=" + dir,
>> +"--module=" + mid,
>> +SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
>> +"--help"))
>> +.outputTo(System.out)
>> +.errorTo(System.out)
>> +.shouldContain("F--help")
>> +.shouldContain("module-info.java");
>> +}
>> +
>> +
>> +/**
>> + * Test that --module= is terminating for VM argument processing just
>> like --module
>> + */
>> +public void testLongFormModuleOptionTermination() throws Exception {
>> +String dir = MODS_DIR.toString();
>> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
>> +
>> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> +// The first --module= will terminate the VM arguments processing.
>> The second pair of module-path and module will be
>> +// deemed as application arguments
>> +OutputAnalyzer output = ProcessTools.executeProcess(
>> +createProcessWithLauncherDebugging(
>> +"--module-path=" + dir,
>> +"--module=" + mid,
>> +"--module-path=" + dir,
>> +"--module=" + mid))
>> +.outputTo(System.out)
>> +.errorTo(System.out)
>> +.shouldContain("argv[ 0] = '--module-path=" + dir)
>> +.shouldContain("argv[ 1] = '--module=" + mid);
>> +
>> +if (IS_WINDOWS) {
>> +output.shouldContain("F--module-path=" +
>> dir).shouldContain("F--module=" + mid);
>> +}
>> +
>> +// java --module=$TESTMODULE/$MAINCLASS --module-path=mods
>> +// This command line will not work as --module= is terminating and
>> the module will be not found
>> +int exitValue = exec("--module=" + mid, "--module-path" + dir);
>> +assertTrue(exitValue != 0);
>> +}
>> }
>>
>> From: Kumar Srinivasan
>> Sent: December 10, 2019 8:43 PM
>> To: Nikola Grcevski
>> Cc: Henry Jen ; Alan Bateman
>> ; core-libs-dev@openjdk.java.net
>> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>>
>> [Resend; cc'ing the group]
>>
>> Hi Nikola,
>>
>> Generally looks great to me. I would have moved initModulesDir to
>> TestHelper.createSimpleModule(File dir).
>> TestHelper.java contains a collection of test related utilities needed by
>> the launcher, it will help someone
>> else use it to create a simple module.
>>
>> I would have rewritten this
>> +ArrayList scratchpad = new ArrayList<>();
>> +scratchpad.add("module test {}");
>> +createFile(new File(testDir, "module-in
Re: [EXTERNAL] JDK-8234076 bug fix candidate
9:29
> 2019 -0500
> @@ -246,6 +246,10 @@
> if (!tr.contains("Error: Could not find or load main class
> AbsentClass")) {
> throw new RuntimeException("Test Fails");
> }
> +
> +// Make sure we handle correctly the module long form (--module=)
> +tr = doExec(javaCmd, "-XX:NativeMemoryTracking=summary",
> "--module=jdk.compiler/com.sun.tools.javac.Main", "--help");
> +ensureNoWarnings(tr);
> }
>
> void ensureNoWarnings(TestResult tr) {
> diff -r bd436284147d test/jdk/tools/launcher/modules/basic/BasicTest.java
> --- a/test/jdk/tools/launcher/modules/basic/BasicTest.java Wed Nov 20
> 08:12:14 2019 +0800
> +++ b/test/jdk/tools/launcher/modules/basic/BasicTest.java Wed Dec 11
> 12:09:29 2019 -0500
> @@ -29,6 +29,7 @@
> * jdk.jlink
> * @build BasicTest jdk.test.lib.compiler.CompilerUtils
> * @run testng BasicTest
> + * @bug 8234076
> * @summary Basic test of starting an application as a module
> */
>
> @@ -40,6 +41,8 @@
>
> import jdk.test.lib.compiler.CompilerUtils;
> import jdk.test.lib.process.ProcessTools;
> +import jdk.test.lib.process.OutputAnalyzer;
> +import jdk.test.lib.Utils;
>
> import org.testng.annotations.BeforeTest;
> import org.testng.annotations.Test;
> @@ -70,6 +73,8 @@
> // the module main class
> private static final String MAIN_CLASS = "jdk.test.Main";
>
> +// for Windows specific launcher tests
> +static final boolean IS_WINDOWS = System.getProperty("os.name",
> "unknown").startsWith("Windows");
>
> @BeforeTest
> public void compileTestModule() throws Exception {
> @@ -259,4 +264,87 @@
> assertTrue(exitValue != 0);
> }
>
> +
> +/**
> + * Helper method that creates a ProcessBuilder with command line
> arguments
> + * while setting the _JAVA_LAUNCHER_DEBUG environment variable.
> + */
> +private ProcessBuilder createProcessWithLauncherDebugging(String...
> cmds) {
> +ProcessBuilder pb =
> ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(cmds));
> +pb.environment().put("_JAVA_LAUNCHER_DEBUG", "true");
> +
> +return pb;
> +}
> +
> + /**
> + * Test the ability for the Windows launcher to do proper application
> argument
> + * detection and expansion, when using the long form module option and
> all passed in
> + * command line arguments are prefixed with a dash.
> + *
> + * These tests are not expected to work on *nixes, and are ignored.
> + */
> +public void testWindowsWithLongFormModuleOption() throws Exception {
> +if (!IS_WINDOWS) {
> +return;
> +}
> +
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
> +// We should be able to find the argument --help as an application
> argument
> +ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +"--help"))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help");
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
> <...src/test>/*.java --help
> +// We should be able to see argument expansion happen
> +ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
> + "--help"))
> + .outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help")
> +.shouldContain("module-info.java");
> +}
> +
> +
> +/**
> + * Test that --module= is terminating for VM argument processing just
> like --module
> + */
> +public void testLongFormModuleOptionTermination() throws Exception {
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
> --module-path=mods --module=$TESTMODULE/$MAINCLASS
> +// The first --module= will terminate the VM arguments processing.
> The second
Re: [EXTERNAL] JDK-8234076 bug fix candidate
; }
> +
> +// Make sure we handle correctly the module long form (--module=)
> +tr = doExec(javaCmd, "-XX:NativeMemoryTracking=summary",
> "--module=jdk.compiler/com.sun.tools.javac.Main", "--help");
> +ensureNoWarnings(tr);
> }
>
> void ensureNoWarnings(TestResult tr) {
> diff -r bd436284147d test/jdk/tools/launcher/modules/basic/BasicTest.java
> --- a/test/jdk/tools/launcher/modules/basic/BasicTest.java Wed Nov 20
> 08:12:14 2019 +0800
> +++ b/test/jdk/tools/launcher/modules/basic/BasicTest.java Wed Dec 11
> 12:09:29 2019 -0500
> @@ -29,6 +29,7 @@
> * jdk.jlink
> * @build BasicTest jdk.test.lib.compiler.CompilerUtils
> * @run testng BasicTest
> + * @bug 8234076
> * @summary Basic test of starting an application as a module
> */
>
> @@ -40,6 +41,8 @@
>
> import jdk.test.lib.compiler.CompilerUtils;
> import jdk.test.lib.process.ProcessTools;
> +import jdk.test.lib.process.OutputAnalyzer;
> +import jdk.test.lib.Utils;
>
> import org.testng.annotations.BeforeTest;
> import org.testng.annotations.Test;
> @@ -70,6 +73,8 @@
> // the module main class
> private static final String MAIN_CLASS = "jdk.test.Main";
>
> +// for Windows specific launcher tests
> +static final boolean IS_WINDOWS = System.getProperty("os.name",
> "unknown").startsWith("Windows");
>
> @BeforeTest
> public void compileTestModule() throws Exception {
> @@ -259,4 +264,87 @@
> assertTrue(exitValue != 0);
> }
>
> +
> +/**
> + * Helper method that creates a ProcessBuilder with command line
> arguments
> + * while setting the _JAVA_LAUNCHER_DEBUG environment variable.
> + */
> +private ProcessBuilder createProcessWithLauncherDebugging(String...
> cmds) {
> +ProcessBuilder pb =
> ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(cmds));
> +pb.environment().put("_JAVA_LAUNCHER_DEBUG", "true");
> +
> +return pb;
> +}
> +
> + /**
> + * Test the ability for the Windows launcher to do proper application
> argument
> + * detection and expansion, when using the long form module option
> and all passed in
> + * command line arguments are prefixed with a dash.
> + *
> + * These tests are not expected to work on *nixes, and are ignored.
> + */
> +public void testWindowsWithLongFormModuleOption() throws Exception {
> +if (!IS_WINDOWS) {
> +return;
> +}
> +
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
> +// We should be able to find the argument --help as an
> application argument
> +ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +"--help"))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help");
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
> <...src/test>/*.java --help
> +// We should be able to see argument expansion happen
> +ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
> +"--help"))
> + .outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help")
> +.shouldContain("module-info.java");
> +}
> +
> +
> +/**
> + * Test that --module= is terminating for VM argument processing just
> like --module
> + */
> +public void testLongFormModuleOptionTermination() throws Exception {
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
> --module-path=mods --module=$TESTMODULE/$MAINCLASS
> +// The first --module= will terminate the VM arguments
> processing. The second pair of module-path and module will be
> +// deemed as application arguments
> +OutputAnalyzer output = ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +
RE: [EXTERNAL] JDK-8234076 bug fix candidate
dows");
@BeforeTest
public void compileTestModule() throws Exception {
@@ -259,4 +264,87 @@
assertTrue(exitValue != 0);
}
+
+/**
+ * Helper method that creates a ProcessBuilder with command line arguments
+ * while setting the _JAVA_LAUNCHER_DEBUG environment variable.
+ */
+private ProcessBuilder createProcessWithLauncherDebugging(String... cmds) {
+ProcessBuilder pb =
ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(cmds));
+pb.environment().put("_JAVA_LAUNCHER_DEBUG", "true");
+
+return pb;
+}
+
+ /**
+ * Test the ability for the Windows launcher to do proper application
argument
+ * detection and expansion, when using the long form module option and all
passed in
+ * command line arguments are prefixed with a dash.
+ *
+ * These tests are not expected to work on *nixes, and are ignored.
+ */
+public void testWindowsWithLongFormModuleOption() throws Exception {
+if (!IS_WINDOWS) {
+return;
+}
+
+String dir = MODS_DIR.toString();
+String mid = TEST_MODULE + "/" + MAIN_CLASS;
+
+// java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
+// We should be able to find the argument --help as an application
argument
+ProcessTools.executeProcess(
+createProcessWithLauncherDebugging(
+"--module-path=" + dir,
+"--module=" + mid,
+"--help"))
+.outputTo(System.out)
+.errorTo(System.out)
+.shouldContain("F--help");
+
+// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
<...src/test>/*.java --help
+// We should be able to see argument expansion happen
+ProcessTools.executeProcess(
+createProcessWithLauncherDebugging(
+"--module-path=" + dir,
+"--module=" + mid,
+SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
+"--help"))
+.outputTo(System.out)
+.errorTo(System.out)
+.shouldContain("F--help")
+.shouldContain("module-info.java");
+}
+
+
+/**
+ * Test that --module= is terminating for VM argument processing just like
--module
+ */
+public void testLongFormModuleOptionTermination() throws Exception {
+String dir = MODS_DIR.toString();
+String mid = TEST_MODULE + "/" + MAIN_CLASS;
+
+// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
--module-path=mods --module=$TESTMODULE/$MAINCLASS
+// The first --module= will terminate the VM arguments processing. The
second pair of module-path and module will be
+// deemed as application arguments
+OutputAnalyzer output = ProcessTools.executeProcess(
+createProcessWithLauncherDebugging(
+ "--module-path=" + dir,
+"--module=" + mid,
+"--module-path=" + dir,
+"--module=" + mid))
+.outputTo(System.out)
+.errorTo(System.out)
+.shouldContain("argv[ 0] = '--module-path=" + dir)
+.shouldContain("argv[ 1] = '--module=" + mid);
+
+if (IS_WINDOWS) {
+output.shouldContain("F--module-path=" +
dir).shouldContain("F--module=" + mid);
+}
+
+// java --module=$TESTMODULE/$MAINCLASS --module-path=mods
+// This command line will not work as --module= is terminating and the
module will be not found
+int exitValue = exec("--module=" + mid, "--module-path" + dir);
+assertTrue(exitValue != 0);
+}
}
From: Kumar Srinivasan
Sent: December 10, 2019 8:43 PM
To: Nikola Grcevski
Cc: Henry Jen ; Alan Bateman ;
core-libs-dev@openjdk.java.net
Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
[Resend; cc'ing the group]
Hi Nikola,
Generally looks great to me. I would have moved initModulesDir to
TestHelper.createSimpleModule(File dir).
TestHelper.java contains a collection of test related utilities needed by the
launcher, it will help someone
else use it to create a simple module.
I would have rewritten this
+ ArrayList scratchpad = new ArrayList<>();
+ scratchpad.add("module test {}");
+ createFile(new File(testDir, "module-info.java"), scratchpad);
+ scratchpad.clear();
+ scratchpad.add("package launcher;");
+ scratchpad.add("import java.util.Arrays;");
+ scratchpad.add("public class Main {");
+ scratchpad.add("public static void main(String[] args) {");
+ scratchpad.add("System.out.println(Arrays.toString(args)
Re: [EXTERNAL] JDK-8234076 bug fix candidate
ProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> + SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
> +"--help"))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help")
> +.shouldContain("module-info.java");
> +}
> +
> +
> +/**
> + * Test that --module= is terminating for VM argument processing just
> like --module
> + */
> +public void testLongFormModuleOptionTermination() throws Exception {
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
> --module-path=mods --module=$TESTMODULE/$MAINCLASS
> +// The first --module= will terminate the VM arguments
> processing. The second pair of module-path and module will be
> +// deemed as application arguments
> +OutputAnalyzer output = ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +"--module-path=" + dir,
> +"--module=" + mid))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("argv[ 0] = '--module-path=" + dir)
> +.shouldContain("argv[ 1] = '--module=" + mid);
> +
> +if (IS_WINDOWS) {
> +output.shouldContain("F--module-path=" +
> dir).shouldContain("F--module=" + mid);
> +}
> +
> +// java --module=$TESTMODULE/$MAINCLASS --module-path=mods
> +// This command line will not work as --module= is terminating
> and the module will be not found
> +int exitValue = exec("--module=" + mid, "--module-path" + dir);
> +assertTrue(exitValue != 0);
> +}
> }
>
>
> From: Kumar Srinivasan
> Sent: December 7, 2019 10:28 PM
> To: Henry Jen
> Cc: Nikola Grcevski ; Alan Bateman <
> alan.bate...@oracle.com>; core-libs-dev@openjdk.java.net
> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>
> Hi,
>
> The launcher fix looks good, despite the launcher fix being simple, please
> note,
> it sometimes requires a lot more effort to write a test for it, please
> read on
>
> Henry excellent catch wrt. isTerminalOp the launcher!!!, that tickled my
> memory and I recalled
> this change for VM's native memory tracking.
>
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fhg.openjdk.java.net%2Fjdk9%2Fjdk9%2Fjdk%2Ffile%2F37d1442d53bc%2Ftest%2Ftools%2Flauncher%2FTestSpecialArgs.java%23l243=02%7C01%7CNikola.Grcevski%40microsoft.com%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875799903=WOjzJJtIY0y4rB2liNkH4nUMNLq2uEnJ8J01gWFgt5w%3D=0
> Inspecting the above changeset, we might have to add another test for JVM
> native memory tracking,
> since this is a terminal argument.
>
> Maybe TestHelper could create a simple module, which prints its args,
>
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fhg.openjdk.java.net%2Fjdk%2Fjdk%2Ffile%2F31882abe1494%2Ftest%2Fjdk%2Ftools%2Flauncher%2FTestHelper.java=02%7C01%7CNikola.Grcevski%40microsoft.com%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875809897=1GLeqSPxVRhGVgE7Cxf6w5l%2F34uOHCGTq2fRIjoxaGg%3D=0
>
> Kumar Srinivasan
>
> On Fri, Dec 6, 2019 at 2:44 PM Henry Jen <mailto:henry@oracle.com>
> wrote:
> Thanks for the work, the test case covers that when —module= is used
> before other options, which is good.
>
> But we need another to make sure that when —module= is put in an argument
> file or environment variable, that should not be allowed. The fix is
> simple, we need to add that to isTerminalOp() method.
>
> Cheers,
> Henry
>
> > On Dec 6, 2019, at 12:28 PM, Nikola Grcevski nikola.grcev...@microsoft.com> wrote:
> >
> > Thank you Henry!
> >
> > I have modified the fix in checkArg to use JLI_StrCCmp as suggested
> below and I have extended the BasicTest.java (in
> test/jdk/tools/launcher/modules/basic) with few additional tests.
> >
> > I don't have author status yet, therefore I'm attaching the patch right
> after my signature. I also updated my external webrev if that's easier to
> review (
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrcevski.github.io%2FJDK-8234076%2Fwebrev%2F=02%7C01%7C
RE: [EXTERNAL] JDK-8234076 bug fix candidate
ame",
"unknown").startsWith("Windows");
@BeforeTest
public void compileTestModule() throws Exception {
@@ -259,4 +264,87 @@
assertTrue(exitValue != 0);
}
+
+/**
+ * Helper method that creates a ProcessBuilder with command line arguments
+ * while setting the _JAVA_LAUNCHER_DEBUG environment variable.
+ */
+private ProcessBuilder createProcessWithLauncherDebugging(String... cmds) {
+ProcessBuilder pb =
ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(cmds));
+pb.environment().put("_JAVA_LAUNCHER_DEBUG", "true");
+
+return pb;
+}
+
+ /**
+ * Test the ability for the Windows launcher to do proper application
argument
+ * detection and expansion, when using the long form module option and all
passed in
+ * command line arguments are prefixed with a dash.
+ *
+ * These tests are not expected to work on *nixes, and are ignored.
+ */
+public void testWindowsWithLongFormModuleOption() throws Exception {
+if (!IS_WINDOWS) {
+return;
+}
+
+String dir = MODS_DIR.toString();
+String mid = TEST_MODULE + "/" + MAIN_CLASS;
+
+// java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
+// We should be able to find the argument --help as an application
argument
+ProcessTools.executeProcess(
+createProcessWithLauncherDebugging(
+"--module-path=" + dir,
+"--module=" + mid,
+"--help"))
+.outputTo(System.out)
+.errorTo(System.out)
+.shouldContain("F--help");
+
+// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
<...src/test>/*.java --help
+// We should be able to see argument expansion happen
+ProcessTools.executeProcess(
+createProcessWithLauncherDebugging(
+"--module-path=" + dir,
+"--module=" + mid,
+SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
+"--help"))
+.outputTo(System.out)
+.errorTo(System.out)
+.shouldContain("F--help")
+.shouldContain("module-info.java");
+}
+
+
+/**
+ * Test that --module= is terminating for VM argument processing just like
--module
+ */
+public void testLongFormModuleOptionTermination() throws Exception {
+String dir = MODS_DIR.toString();
+String mid = TEST_MODULE + "/" + MAIN_CLASS;
+
+// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
--module-path=mods --module=$TESTMODULE/$MAINCLASS
+// The first --module= will terminate the VM arguments processing. The
second pair of module-path and module will be
+// deemed as application arguments
+OutputAnalyzer output = ProcessTools.executeProcess(
+createProcessWithLauncherDebugging(
+ "--module-path=" + dir,
+"--module=" + mid,
+"--module-path=" + dir,
+"--module=" + mid))
+.outputTo(System.out)
+.errorTo(System.out)
+.shouldContain("argv[ 0] = '--module-path=" + dir)
+.shouldContain("argv[ 1] = '--module=" + mid);
+
+if (IS_WINDOWS) {
+output.shouldContain("F--module-path=" +
dir).shouldContain("F--module=" + mid);
+}
+
+// java --module=$TESTMODULE/$MAINCLASS --module-path=mods
+// This command line will not work as --module= is terminating and the
module will be not found
+int exitValue = exec("--module=" + mid, "--module-path" + dir);
+assertTrue(exitValue != 0);
+}
}
From: Kumar Srinivasan
Sent: December 7, 2019 10:28 PM
To: Henry Jen
Cc: Nikola Grcevski ; Alan Bateman
; core-libs-dev@openjdk.java.net
Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
Hi,
The launcher fix looks good, despite the launcher fix being simple, please
note,
it sometimes requires a lot more effort to write a test for it, please read
on
Henry excellent catch wrt. isTerminalOp the launcher!!!, that tickled my
memory and I recalled
this change for VM's native memory tracking.
https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fhg.openjdk.java.net%2Fjdk9%2Fjdk9%2Fjdk%2Ffile%2F37d1442d53bc%2Ftest%2Ftools%2Flauncher%2FTestSpecialArgs.java%23l243=02%7C01%7CNikola.Grcevski%40microsoft.com%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875799903=WOjzJJtIY0y4rB2liNkH4nUMNLq2uEnJ8J01gWFgt5w%3D=0
Inspecting the above changeset, we might have to add another test for JVM
native memory tracking,
since this is a terminal argument.
M
Re: [EXTERNAL] JDK-8234076 bug fix candidate
tatic final String MAIN_CLASS = "jdk.test.Main";
> >
> > +// for Windows specific launcher tests
> > +static final boolean IS_WINDOWS = System.getProperty("os.name",
> "unknown").startsWith("Windows");
> >
> > @BeforeTest
> > public void compileTestModule() throws Exception {
> > @@ -259,4 +264,87 @@
> > assertTrue(exitValue != 0);
> > }
> >
> > +
> > +/**
> > + * Helper method that creates a ProcessBuilder with command line
> arguments
> > + * while setting the _JAVA_LAUNCHER_DEBUG environment variable.
> > + */
> > +private ProcessBuilder createProcessWithLauncherDebugging(String...
> cmds) {
> > +ProcessBuilder pb =
> ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(cmds));
> > +pb.environment().put("_JAVA_LAUNCHER_DEBUG", "true");
> > +
> > +return pb;
> > +}
> > +
> > + /**
> > + * Test the ability for the Windows launcher to do proper
> application argument
> > + * detection and expansion, when using the long form module option
> and all passed in
> > + * command line arguments are prefixed with a dash.
> > + *
> > + * These tests are not expected to work on *nixes, and are ignored.
> > + */
> > +public void testWindowsWithLongFormModuleOption() throws Exception {
> > +if (!IS_WINDOWS) {
> > +return;
> > +}
> > +
> > +String dir = MODS_DIR.toString();
> > +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> > +
> > +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
> --help
> > +// We should be able to find the argument --help as an
> application argument
> > +ProcessTools.executeProcess(
> > +createProcessWithLauncherDebugging(
> > +"--module-path=" + dir,
> > +"--module=" + mid,
> > +"--help"))
> > +.outputTo(System.out)
> > +.errorTo(System.out)
> > +.shouldContain("F--help");
> > +
> > +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
> <...src/test>/*.java --help
> > +// We should be able to see argument expansion happen
> > +ProcessTools.executeProcess(
> > +createProcessWithLauncherDebugging(
> > +"--module-path=" + dir,
> > +"--module=" + mid,
> > +SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
> > +"--help"))
> > +.outputTo(System.out)
> > +.errorTo(System.out)
> > +.shouldContain("F--help")
> > +.shouldContain("module-info.java");
> > +}
> > +
> > +
> > +/**
> > + * Test that --module= is terminating for VM argument processing
> just like --module
> > + */
> > +public void testLongFormModuleOptionTermination() throws Exception {
> > +String dir = MODS_DIR.toString();
> > +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> > +
> > +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
> --module-path=mods --module=$TESTMODULE/$MAINCLASS
> > +// The first --module= will terminate the VM arguments
> processing. The second pair of module-path and module will be
> > + // deemed as application arguments
> > +OutputAnalyzer output = ProcessTools.executeProcess(
> > +createProcessWithLauncherDebugging(
> > +"--module-path=" + dir,
> > +"--module=" + mid,
> > +"--module-path=" + dir,
> > +"--module=" + mid))
> > +.outputTo(System.out)
> > +.errorTo(System.out)
> > +.shouldContain("argv[ 0] = '--module-path=" + dir)
> > +.shouldContain("argv[ 1] = '--module=" + mid);
> > +
> > +if (IS_WINDOWS) {
> > +output.shouldContain("F--module-path=" +
> dir).shouldContain("F--module=" + mid);
> > +}
> > +
> > +// java --module=$TESTMODULE/$MAINCLASS --module-path=mods
> > +// This command line will not work as --module= is terminating
> and the module will be not found
> > +int exitValue = exec("--module="
Re: [EXTERNAL] JDK-8234076 bug fix candidate
dash.
> + *
> + * These tests are not expected to work on *nixes, and are ignored.
> + */
> +public void testWindowsWithLongFormModuleOption() throws Exception {
> +if (!IS_WINDOWS) {
> +return;
> +}
> +
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
> +// We should be able to find the argument --help as an application
> argument
> +ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +"--help"))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help");
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
> <...src/test>/*.java --help
> +// We should be able to see argument expansion happen
> +ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
> +"--help"))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help")
> +.shouldContain("module-info.java");
> +}
> +
> +
> +/**
> + * Test that --module= is terminating for VM argument processing just
> like --module
> + */
> +public void testLongFormModuleOptionTermination() throws Exception {
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
> --module-path=mods --module=$TESTMODULE/$MAINCLASS
> +// The first --module= will terminate the VM arguments processing.
> The second pair of module-path and module will be
> +// deemed as application arguments
> +OutputAnalyzer output = ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +"--module-path=" + dir,
> +"--module=" + mid))
> +.outputTo(System.out)
> + .errorTo(System.out)
> +.shouldContain("argv[ 0] = '--module-path=" + dir)
> +.shouldContain("argv[ 1] = '--module=" + mid);
> +
> +if (IS_WINDOWS) {
> +output.shouldContain("F--module-path=" +
> dir).shouldContain("F--module=" + mid);
> +}
> +
> +// java --module=$TESTMODULE/$MAINCLASS --module-path=mods
> +// This command line will not work as --module= is terminating and
> the module will be not found
> +int exitValue = exec("--module=" + mid, "--module-path" + dir);
> +assertTrue(exitValue != 0);
> +}
> }
>
>
> -Original Message-
> From: Henry Jen
> Sent: December 6, 2019 12:03 AM
> To: Nikola Grcevski
> Cc: Kumar Srinivasan ; Alan Bateman
> ; core-libs-dev@openjdk.java.net
> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>
>
>> On Dec 5, 2019, at 5:57 PM, Nikola Grcevski
>> wrote:
>>
>> Hello all again!
>>
>> Based on the suggestion by Kumar I made the following small patch to
>> checkArg src/java.base/share/native/libjli/args.c:
>>
>> --- a/src/java.base/share/native/libjli/args.c
>> +++ b/src/java.base/share/native/libjli/args.c
>> @@ -130,6 +130,8 @@ static void checkArg(const char *arg) {
>>}
>>} else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
>>stopExpansion = JNI_TRUE;
>> +} else if (JLI_StrNCmp(arg, "--module=", 9) == 0) {
>
> I would suggest use JLI_StrCCmp as in java.c. I think combine this fix with
> origin crash prevention for -1 is a safe approach and most compatible to
> current behavior.
>
> BTW, we need the patch to be hosted on cr.openjdk.java.net or you can attach
> the patch to the review thread if you are not yet an author.
>
> Cheers,
> Henry
>
>
>> +idx = argsCount;
>>}
>>} else {
>>if (!expectingNoDashArg) {
>>
>> The change is in common code and simply checks for the usage of --modul
RE: [EXTERNAL] JDK-8234076 bug fix candidate
/ We should be able to see argument expansion happen
+ProcessTools.executeProcess(
+createProcessWithLauncherDebugging(
+"--module-path=" + dir,
+"--module=" + mid,
+SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
+"--help"))
+.outputTo(System.out)
+.errorTo(System.out)
+.shouldContain("F--help")
+.shouldContain("module-info.java");
+}
+
+
+/**
+ * Test that --module= is terminating for VM argument processing just like
--module
+ */
+public void testLongFormModuleOptionTermination() throws Exception {
+String dir = MODS_DIR.toString();
+String mid = TEST_MODULE + "/" + MAIN_CLASS;
+
+// java --module-path=mods --module=$TESTMODULE/$MAINCLASS
--module-path=mods --module=$TESTMODULE/$MAINCLASS
+// The first --module= will terminate the VM arguments processing. The
second pair of module-path and module will be
+// deemed as application arguments
+OutputAnalyzer output = ProcessTools.executeProcess(
+createProcessWithLauncherDebugging(
+"--module-path=" + dir,
+"--module=" + mid,
+"--module-path=" + dir,
+"--module=" + mid))
+.outputTo(System.out)
+.errorTo(System.out)
+.shouldContain("argv[ 0] = '--module-path=" + dir)
+.shouldContain("argv[ 1] = '--module=" + mid);
+
+if (IS_WINDOWS) {
+output.shouldContain("F--module-path=" +
dir).shouldContain("F--module=" + mid);
+}
+
+// java --module=$TESTMODULE/$MAINCLASS --module-path=mods
+// This command line will not work as --module= is terminating and the
module will be not found
+ int exitValue = exec("--module=" + mid, "--module-path" + dir);
+assertTrue(exitValue != 0);
+}
}
-Original Message-
From: Henry Jen
Sent: December 6, 2019 12:03 AM
To: Nikola Grcevski
Cc: Kumar Srinivasan ; Alan Bateman
; core-libs-dev@openjdk.java.net
Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
> On Dec 5, 2019, at 5:57 PM, Nikola Grcevski
> wrote:
>
> Hello all again!
>
> Based on the suggestion by Kumar I made the following small patch to checkArg
> src/java.base/share/native/libjli/args.c:
>
> --- a/src/java.base/share/native/libjli/args.c
> +++ b/src/java.base/share/native/libjli/args.c
> @@ -130,6 +130,8 @@ static void checkArg(const char *arg) {
> }
> } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
> stopExpansion = JNI_TRUE;
> +} else if (JLI_StrNCmp(arg, "--module=", 9) == 0) {
I would suggest use JLI_StrCCmp as in java.c. I think combine this fix with
origin crash prevention for -1 is a safe approach and most compatible to
current behavior.
BTW, we need the patch to be hosted on cr.openjdk.java.net or you can attach
the patch to the review thread if you are not yet an author.
Cheers,
Henry
> +idx = argsCount;
> }
> } else {
> if (!expectingNoDashArg) {
>
> The change is in common code and simply checks for the usage of --module= and
> deems the next argument as the start of the application arguments. I can
> confirm that using -m instead of --module doesn't allow for the = separator
> to be used, so we only need to check for --module= if this is a desired
> change.
>
> I tested with various combinations on the command line and I couldn't find a
> set of arguments ordering that breaks the terminating quality of --module.
>
> I also performed series of tests to try to break the argument expansion on
> Windows with this change, but it worked in all instances. The testcase is
> working correctly with this change, as well as the javac launcher command as
> proposed by Kumar (java --module-path=mods
> --module=jdk.compiler/com.sun.tools.javac.Main *.java).
>
> I ran all the launcher tests on both Windows and Linux and all tests pass.
>
> Please let me know if this is a preferred approach to address this bug or if
> you think there's a potential problem with the change. If this is an
> acceptable fix I can create new webrev with set of tests for the various edge
> cases I tried, and new launcher specific tests that ensure argument expansion
> is performed on Windows with this module usage.
>
> Thank you,
> Nikola
>
> -Original Message-
> From: Henry Jen
> Sent: December 4, 2019 8:26 PM
> To: Kumar Srinivasan ; Alan Bateman
> ; Nikola Grcevski
>
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: [EXTER
Re: [EXTERNAL] JDK-8234076 bug fix candidate
> On Dec 5, 2019, at 5:57 PM, Nikola Grcevski
> wrote:
>
> Hello all again!
>
> Based on the suggestion by Kumar I made the following small patch to checkArg
> src/java.base/share/native/libjli/args.c:
>
> --- a/src/java.base/share/native/libjli/args.c
> +++ b/src/java.base/share/native/libjli/args.c
> @@ -130,6 +130,8 @@ static void checkArg(const char *arg) {
> }
> } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
> stopExpansion = JNI_TRUE;
> +} else if (JLI_StrNCmp(arg, "--module=", 9) == 0) {
I would suggest use JLI_StrCCmp as in java.c. I think combine this fix with
origin crash prevention for -1 is a safe approach and most compatible to
current behavior.
BTW, we need the patch to be hosted on cr.openjdk.java.net or you can attach
the patch to the review thread if you are not yet an author.
Cheers,
Henry
> +idx = argsCount;
> }
> } else {
> if (!expectingNoDashArg) {
>
> The change is in common code and simply checks for the usage of --module= and
> deems the next argument as the start of the application arguments. I can
> confirm that using -m instead of --module doesn't allow for the = separator
> to be used, so we only need to check for --module= if this is a desired
> change.
>
> I tested with various combinations on the command line and I couldn't find a
> set of arguments ordering that breaks the terminating quality of --module.
>
> I also performed series of tests to try to break the argument expansion on
> Windows with this change, but it worked in all instances. The testcase is
> working correctly with this change, as well as the javac launcher command as
> proposed by Kumar (java --module-path=mods
> --module=jdk.compiler/com.sun.tools.javac.Main *.java).
>
> I ran all the launcher tests on both Windows and Linux and all tests pass.
>
> Please let me know if this is a preferred approach to address this bug or if
> you think there's a potential problem with the change. If this is an
> acceptable fix I can create new webrev with set of tests for the various edge
> cases I tried, and new launcher specific tests that ensure argument expansion
> is performed on Windows with this module usage.
>
> Thank you,
> Nikola
>
> -Original Message-
> From: Henry Jen
> Sent: December 4, 2019 8:26 PM
> To: Kumar Srinivasan ; Alan Bateman
> ; Nikola Grcevski
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>
>> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan wrote:
>>
>> Hi Nikola,
>>
>> It looks ok to me, I will leave it to Henry and Alan to bless this.
>>
>> IMHO: I think we should fix it correctly now than later, I don't think
>> it is all that difficult AFAICT all the launcher has to do is
>> identify "--module==", then the next argument is the first index.
>>
>
> I don’t disagree, if we can decide whether —module= is allowed. Based on my
> understanding and the document for java[1], the —module= is not necessarily
> correct.
>
> If we decided to accept it, the fix would be make sure the index set
> correctly as Kumar suggested, and the fix can be relatively simple.
>
> FWIW, it’s still possible the index is NOT_FOUND if there is no main class
> specified, but that should never cause crash as if no main class is found,
> the launcher should either execute other terminal argument or display help.
>
> I agree the fix is not complete as it only make sure no crash can happen. It
> doesn’t actually make —module= illegal and show help in case of that. At this
> point, there is a discrepancy in launcher code regard —module usage, and we
> need to fix that.
>
> I’ll let Alan to comment about the —module option usage.
>
> The webrev looks good to me, although I would like to see the test be more
> close to other arguments processing test, but since this can only be
> reproduced with —module= usage, I assume this is not bad.
>
> [1]
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oracle.com%2Fen%2Fjava%2Fjavase%2F13%2Fdocs%2Fspecs%2Fman%2Fjava.htmldata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544sdata=uO2tgi1QNvXgI0wT%2FxOxB%2Bh10YpCbq37uzkKKlByUYg%3Dreserved=0
>
>> Kumar
>>
>> On Tue, Dec 3, 2019 at 5:29 PM Nikola Grcevski
>> wrote:
>> Hi Henry and Kumar,
>>
>> Thanks again for your comments! I have updated the test to be part of
>> test/jdk/tools/launcher/modules/basic, it took a lot less code to achieve
>> the same a
RE: [EXTERNAL] JDK-8234076 bug fix candidate
Hello all again!
Based on the suggestion by Kumar I made the following small patch to checkArg
src/java.base/share/native/libjli/args.c:
--- a/src/java.base/share/native/libjli/args.c
+++ b/src/java.base/share/native/libjli/args.c
@@ -130,6 +130,8 @@ static void checkArg(const char *arg) {
}
} else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
stopExpansion = JNI_TRUE;
+} else if (JLI_StrNCmp(arg, "--module=", 9) == 0) {
+idx = argsCount;
}
} else {
if (!expectingNoDashArg) {
The change is in common code and simply checks for the usage of --module= and
deems the next argument as the start of the application arguments. I can
confirm that using -m instead of --module doesn't allow for the = separator to
be used, so we only need to check for --module= if this is a desired change.
I tested with various combinations on the command line and I couldn't find a
set of arguments ordering that breaks the terminating quality of --module.
I also performed series of tests to try to break the argument expansion on
Windows with this change, but it worked in all instances. The testcase is
working correctly with this change, as well as the javac launcher command as
proposed by Kumar (java --module-path=mods
--module=jdk.compiler/com.sun.tools.javac.Main *.java).
I ran all the launcher tests on both Windows and Linux and all tests pass.
Please let me know if this is a preferred approach to address this bug or if
you think there's a potential problem with the change. If this is an acceptable
fix I can create new webrev with set of tests for the various edge cases I
tried, and new launcher specific tests that ensure argument expansion is
performed on Windows with this module usage.
Thank you,
Nikola
-Original Message-
From: Henry Jen
Sent: December 4, 2019 8:26 PM
To: Kumar Srinivasan ; Alan Bateman
; Nikola Grcevski
Cc: core-libs-dev@openjdk.java.net
Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan wrote:
>
> Hi Nikola,
>
> It looks ok to me, I will leave it to Henry and Alan to bless this.
>
> IMHO: I think we should fix it correctly now than later, I don't think
> it is all that difficult AFAICT all the launcher has to do is
> identify "--module==", then the next argument is the first index.
>
I don’t disagree, if we can decide whether —module= is allowed. Based on my
understanding and the document for java[1], the —module= is not necessarily
correct.
If we decided to accept it, the fix would be make sure the index set correctly
as Kumar suggested, and the fix can be relatively simple.
FWIW, it’s still possible the index is NOT_FOUND if there is no main class
specified, but that should never cause crash as if no main class is found, the
launcher should either execute other terminal argument or display help.
I agree the fix is not complete as it only make sure no crash can happen. It
doesn’t actually make —module= illegal and show help in case of that. At this
point, there is a discrepancy in launcher code regard —module usage, and we
need to fix that.
I’ll let Alan to comment about the —module option usage.
The webrev looks good to me, although I would like to see the test be more
close to other arguments processing test, but since this can only be reproduced
with —module= usage, I assume this is not bad.
[1]
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oracle.com%2Fen%2Fjava%2Fjavase%2F13%2Fdocs%2Fspecs%2Fman%2Fjava.htmldata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544sdata=uO2tgi1QNvXgI0wT%2FxOxB%2Bh10YpCbq37uzkKKlByUYg%3Dreserved=0
> Kumar
>
> On Tue, Dec 3, 2019 at 5:29 PM Nikola Grcevski
> wrote:
> Hi Henry and Kumar,
>
> Thanks again for your comments! I have updated the test to be part of
> test/jdk/tools/launcher/modules/basic, it took a lot less code to achieve the
> same amount of testing. I added a new test method inside BasicTest.java and
> tested on both Windows and Linux.
>
> Please find my updated webrev here for your review:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrce
> vski.github.io%2FJDK-8234076%2Fwebrev%2Fdata=02%7C01%7CNikola.Grc
> evski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f
> 141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544sdata=ee0dSSSJ%
> 2BXZogFtgPl8xFcUZ0P%2BX%2FR2G7x%2F4jjqiRIE%3Dreserved=0
>
> Cheers,
>
> Nikola
>
> -Original Message-
> From: Henry Jen
> Sent: December 3, 2019 11:39 AM
> To: Kumar Srinivasan
> Cc: Nikola Grcevski ; Alan Bateman
> ; core-libs-dev@openjdk.java.net
> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>
> Kumar,
Re: [EXTERNAL] JDK-8234076 bug fix candidate
> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan wrote: > > Hi Nikola, > > It looks ok to me, I will leave it to Henry and Alan to bless this. > > IMHO: I think we should fix it correctly now than later, I don't think it is > all that > difficult AFAICT all the launcher has to do is identify "--module==", then > the next argument is the first index. > I don’t disagree, if we can decide whether —module= is allowed. Based on my understanding and the document for java[1], the —module= is not necessarily correct. If we decided to accept it, the fix would be make sure the index set correctly as Kumar suggested, and the fix can be relatively simple. FWIW, it’s still possible the index is NOT_FOUND if there is no main class specified, but that should never cause crash as if no main class is found, the launcher should either execute other terminal argument or display help. I agree the fix is not complete as it only make sure no crash can happen. It doesn’t actually make —module= illegal and show help in case of that. At this point, there is a discrepancy in launcher code regard —module usage, and we need to fix that. I’ll let Alan to comment about the —module option usage. The webrev looks good to me, although I would like to see the test be more close to other arguments processing test, but since this can only be reproduced with —module= usage, I assume this is not bad. [1] https://docs.oracle.com/en/java/javase/13/docs/specs/man/java.html > Kumar > > On Tue, Dec 3, 2019 at 5:29 PM Nikola Grcevski > wrote: > Hi Henry and Kumar, > > Thanks again for your comments! I have updated the test to be part of > test/jdk/tools/launcher/modules/basic, it took a lot less code to achieve the > same amount of testing. I added a new test method inside BasicTest.java and > tested on both Windows and Linux. > > Please find my updated webrev here for your review: > https://grcevski.github.io/JDK-8234076/webrev/ > > Cheers, > > Nikola > > -Original Message- > From: Henry Jen > Sent: December 3, 2019 11:39 AM > To: Kumar Srinivasan > Cc: Nikola Grcevski ; Alan Bateman > ; core-libs-dev@openjdk.java.net > Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate > > Kumar, > > Great to have you look at this, you are correct, this patch doesn’t address > the wildcard expansion issue, but only to address the potential crash if a > main class is not specified as Nikola pointed out. > > We definitely need a follow up to fix wildcard expansion. The pointer to > simplify the test is helpful, it would make the test more obvious. > > Cheers, > Henry > > > On Dec 3, 2019, at 7:14 AM, Kumar Srinivasan wrote: > > > > Hi, > > > > Sorry for chiming in late in the review process, for what it's worth > > > > 1. It is not at all clear to me if this solution is correct, yes it averts > > the problem of not finding the main-class > > and subsequent crash, but it does not address wildcard arguments > > expansion. > > > > What if we have > > % java --module-path=mods > > --module=jdk.compiler/com.sun.tools.javac.Main *.java > > Where jdk.compiler is a java compiler implementation (javac). > > The user would expect the above compiler module to build all the .java > > files in that directory, > > and this fix will not address that. > > > > Some background: > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-7146424data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=9KSksL8%2BCmXSscF8oGGn5piLz2wApQ9paJUyZWbKWCw%3Dreserved=0 > > Please see all the related bugs in the above JIRA issue. > > > > Paragraph #6 in this interview surmises the wild card behavior on Windows: > > https://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fwww.princeton.edu%2F~hos%2Fmike%2Ftranscripts%2Fkernighan.htmdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=l20J1AN4vBmT19gzBxLOktBsdv260F2rMWRvCLeVb84%3Dreserved=0 > > > > 2. Though the arguments related tests are in Aaarghs.java the modules > > related tests for the launcher are at: > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fhg.openjdk.java.net%2Fjdk%2Fjdk13%2Ffile%2F0368f3a073a9%2Ftest%2Fjdk%2Ftools%2Flauncher%2Fmodules%2Fbasicdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=jsOjS1rgX4tfzJwE8Xif3NARZPRHb39Y64LvSdz1Jic%3Dre
Re: [EXTERNAL] JDK-8234076 bug fix candidate
Hi Nikola, It looks ok to me, I will leave it to Henry and Alan to bless this. IMHO: I think we should fix it correctly now than later, I don't think it is all that difficult AFAICT all the launcher has to do is identify "--module==", then the next argument is the first index. Kumar On Tue, Dec 3, 2019 at 5:29 PM Nikola Grcevski < nikola.grcev...@microsoft.com> wrote: > Hi Henry and Kumar, > > Thanks again for your comments! I have updated the test to be part of > test/jdk/tools/launcher/modules/basic, it took a lot less code to achieve > the same amount of testing. I added a new test method inside BasicTest.java > and tested on both Windows and Linux. > > Please find my updated webrev here for your review: > https://grcevski.github.io/JDK-8234076/webrev/ > > Cheers, > > Nikola > > -Original Message- > From: Henry Jen > Sent: December 3, 2019 11:39 AM > To: Kumar Srinivasan > Cc: Nikola Grcevski ; Alan Bateman < > alan.bate...@oracle.com>; core-libs-dev@openjdk.java.net > Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate > > Kumar, > > Great to have you look at this, you are correct, this patch doesn’t > address the wildcard expansion issue, but only to address the potential > crash if a main class is not specified as Nikola pointed out. > > We definitely need a follow up to fix wildcard expansion. The pointer to > simplify the test is helpful, it would make the test more obvious. > > Cheers, > Henry > > > On Dec 3, 2019, at 7:14 AM, Kumar Srinivasan > wrote: > > > > Hi, > > > > Sorry for chiming in late in the review process, for what it's worth > > > > 1. It is not at all clear to me if this solution is correct, yes it > averts the problem of not finding the main-class > > and subsequent crash, but it does not address wildcard arguments > expansion. > > > > What if we have > > % java --module-path=mods > --module=jdk.compiler/com.sun.tools.javac.Main *.java > > Where jdk.compiler is a java compiler implementation (javac). > > The user would expect the above compiler module to build all the > .java files in that directory, > > and this fix will not address that. > > > > Some background: > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-7146424data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=9KSksL8%2BCmXSscF8oGGn5piLz2wApQ9paJUyZWbKWCw%3Dreserved=0 > > Please see all the related bugs in the above JIRA issue. > > > > Paragraph #6 in this interview surmises the wild card behavior on > Windows: > > > https://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fwww.princeton.edu%2F~hos%2Fmike%2Ftranscripts%2Fkernighan.htmdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=l20J1AN4vBmT19gzBxLOktBsdv260F2rMWRvCLeVb84%3Dreserved=0 > > > > 2. Though the arguments related tests are in Aaarghs.java the modules > related tests for the launcher are at: > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fhg.openjdk.java.net%2Fjdk%2Fjdk13%2Ffile%2F0368f3a073a9%2Ftest%2Fjdk%2Ftools%2Flauncher%2Fmodules%2Fbasicdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=jsOjS1rgX4tfzJwE8Xif3NARZPRHb39Y64LvSdz1Jic%3Dreserved=0 > > Using the modules test framework may make the test simpler. > > > > Kumar Srinivasan > > > > > > On Mon, Dec 2, 2019 at 11:34 AM Nikola Grcevski < > nikola.grcev...@microsoft.com> wrote: > > Hi Alan and Henry, > > > > Thank you for reviewing my email! Henry's observation matches mine, the > shared common code for all platforms in checkArg > (src/java.base/share/native/libjli/args.c) can potentially leave the > firstAppArgIndex static set to -1, if all passed command line arguments are > prefixed with a dash. Later on the arguments are validated, however we > might crash before then on Windows because of the negative index access. In > this case, the customer command was valid (modules usage) and the program > runs successfully. > > > > I created a webrev here for the change, including a new test in > Arrrghs.java: > > > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrcevski.github.io%2FJDK-8234076%2Fwebrev%2Fdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293
RE: [EXTERNAL] JDK-8234076 bug fix candidate
Hi Henry and Kumar, Thanks again for your comments! I have updated the test to be part of test/jdk/tools/launcher/modules/basic, it took a lot less code to achieve the same amount of testing. I added a new test method inside BasicTest.java and tested on both Windows and Linux. Please find my updated webrev here for your review: https://grcevski.github.io/JDK-8234076/webrev/ Cheers, Nikola -Original Message- From: Henry Jen Sent: December 3, 2019 11:39 AM To: Kumar Srinivasan Cc: Nikola Grcevski ; Alan Bateman ; core-libs-dev@openjdk.java.net Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate Kumar, Great to have you look at this, you are correct, this patch doesn’t address the wildcard expansion issue, but only to address the potential crash if a main class is not specified as Nikola pointed out. We definitely need a follow up to fix wildcard expansion. The pointer to simplify the test is helpful, it would make the test more obvious. Cheers, Henry > On Dec 3, 2019, at 7:14 AM, Kumar Srinivasan wrote: > > Hi, > > Sorry for chiming in late in the review process, for what it's worth > > 1. It is not at all clear to me if this solution is correct, yes it averts > the problem of not finding the main-class > and subsequent crash, but it does not address wildcard arguments > expansion. > > What if we have > % java --module-path=mods --module=jdk.compiler/com.sun.tools.javac.Main > *.java > Where jdk.compiler is a java compiler implementation (javac). > The user would expect the above compiler module to build all the .java > files in that directory, > and this fix will not address that. > > Some background: > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-7146424data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=9KSksL8%2BCmXSscF8oGGn5piLz2wApQ9paJUyZWbKWCw%3Dreserved=0 > Please see all the related bugs in the above JIRA issue. > > Paragraph #6 in this interview surmises the wild card behavior on Windows: > https://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fwww.princeton.edu%2F~hos%2Fmike%2Ftranscripts%2Fkernighan.htmdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=l20J1AN4vBmT19gzBxLOktBsdv260F2rMWRvCLeVb84%3Dreserved=0 > > 2. Though the arguments related tests are in Aaarghs.java the modules related > tests for the launcher are at: > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fhg.openjdk.java.net%2Fjdk%2Fjdk13%2Ffile%2F0368f3a073a9%2Ftest%2Fjdk%2Ftools%2Flauncher%2Fmodules%2Fbasicdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=jsOjS1rgX4tfzJwE8Xif3NARZPRHb39Y64LvSdz1Jic%3Dreserved=0 > Using the modules test framework may make the test simpler. > > Kumar Srinivasan > > > On Mon, Dec 2, 2019 at 11:34 AM Nikola Grcevski > wrote: > Hi Alan and Henry, > > Thank you for reviewing my email! Henry's observation matches mine, the > shared common code for all platforms in checkArg > (src/java.base/share/native/libjli/args.c) can potentially leave the > firstAppArgIndex static set to -1, if all passed command line arguments are > prefixed with a dash. Later on the arguments are validated, however we might > crash before then on Windows because of the negative index access. In this > case, the customer command was valid (modules usage) and the program runs > successfully. > > I created a webrev here for the change, including a new test in Arrrghs.java: > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrcevski.github.io%2FJDK-8234076%2Fwebrev%2Fdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=lx%2FFVo5UOw3uhxgttVm2RKkoFPu8tmQtx0OwMvbTwJs%3Dreserved=0 > > I copied the test validation and assertion style of other code inside the > test class. > > Please let me know if you have any comments or suggestions. > > Thanks again! > > -Original Message- > From: Henry Jen > Sent: December 2, 2019 12:26 PM > To: Alan Bateman > Cc: Nikola Grcevski ; > core-libs-dev@openjdk.java.net > Subject: [EXTERNAL] Re: JDK-8234076 bug fix candidate > > The fix looks reasonable to me, basically, if the command argument format is > not legal, it’s possible we won’t find the main class when doing argument > file expansion, and the index is -1 which could cause crash on Windows > platform for the wildcard processing. >
Re: [EXTERNAL] JDK-8234076 bug fix candidate
Kumar, Great to have you look at this, you are correct, this patch doesn’t address the wildcard expansion issue, but only to address the potential crash if a main class is not specified as Nikola pointed out. We definitely need a follow up to fix wildcard expansion. The pointer to simplify the test is helpful, it would make the test more obvious. Cheers, Henry > On Dec 3, 2019, at 7:14 AM, Kumar Srinivasan wrote: > > Hi, > > Sorry for chiming in late in the review process, for what it's worth > > 1. It is not at all clear to me if this solution is correct, yes it averts > the problem of not finding the main-class > and subsequent crash, but it does not address wildcard arguments > expansion. > > What if we have > % java --module-path=mods --module=jdk.compiler/com.sun.tools.javac.Main > *.java > Where jdk.compiler is a java compiler implementation (javac). > The user would expect the above compiler module to build all the .java > files in that directory, > and this fix will not address that. > > Some background: > https://bugs.openjdk.java.net/browse/JDK-7146424 > Please see all the related bugs in the above JIRA issue. > > Paragraph #6 in this interview surmises the wild card behavior on Windows: > https://www.princeton.edu/~hos/mike/transcripts/kernighan.htm > > 2. Though the arguments related tests are in Aaarghs.java the modules related > tests for the launcher are at: > https://hg.openjdk.java.net/jdk/jdk13/file/0368f3a073a9/test/jdk/tools/launcher/modules/basic > Using the modules test framework may make the test simpler. > > Kumar Srinivasan > > > On Mon, Dec 2, 2019 at 11:34 AM Nikola Grcevski > wrote: > Hi Alan and Henry, > > Thank you for reviewing my email! Henry's observation matches mine, the > shared common code for all platforms in checkArg > (src/java.base/share/native/libjli/args.c) can potentially leave the > firstAppArgIndex static set to -1, if all passed command line arguments are > prefixed with a dash. Later on the arguments are validated, however we might > crash before then on Windows because of the negative index access. In this > case, the customer command was valid (modules usage) and the program runs > successfully. > > I created a webrev here for the change, including a new test in Arrrghs.java: > > https://grcevski.github.io/JDK-8234076/webrev/ > > I copied the test validation and assertion style of other code inside the > test class. > > Please let me know if you have any comments or suggestions. > > Thanks again! > > -Original Message- > From: Henry Jen > Sent: December 2, 2019 12:26 PM > To: Alan Bateman > Cc: Nikola Grcevski ; > core-libs-dev@openjdk.java.net > Subject: [EXTERNAL] Re: JDK-8234076 bug fix candidate > > The fix looks reasonable to me, basically, if the command argument format is > not legal, it’s possible we won’t find the main class when doing argument > file expansion, and the index is -1 which could cause crash on Windows > platform for the wildcard processing. > > I think we should add a test case for this, probably in > test/jdk/tools/launcher/Arrrghs.java. > > As I understand it, the argument validation is done in a later stage after > calling JLI_Launch. > > Cheers, > Henry > > > > On Dec 2, 2019, at 2:12 AM, Alan Bateman wrote: > > > > On 20/11/2019 19:42, Nikola Grcevski wrote: > >> : > >> > >> Please let me know if this approach is acceptable for the current bug > >> report and I'll make a webrev and include appropriate launcher tests. I > >> was thinking the tests should do extra validation on the output from > >> _JAVA_LAUNCHER_DEBUG on Windows. > >> > > I think you're in the right area but I would have expected the arg index to > > 0 here. Henry Jen (cc'ed) might have some comments on this. > > > > -Alan >
