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 <kusriniva...@vmware.com> Sent: December 12, 2019 8:32 AM To: Henry Jen <henry....@oracle.com> Cc: core-libs-dev@openjdk.java.net; Nikola Grcevski <nikola.grcev...@microsoft.com> 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 <henry....@oracle.com> 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 <ksrini...@gmail.com> 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 >> <nikola.grcev...@microsoft.com> 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%2F&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086603237&sdata=i9pxLqzp2zCmsMbFDB56Aimdq1VVbYxjeaP%2ByBtNA3w%3D&reserved=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 <sys/stat.h> >> #include <wtypes.h> >> #include <commctrl.h> >> +#include <assert.h> >> >> #include <jni.h> >> #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 ArgsEnvVar extends TestHelper { >> private static File testJar = null; >> @@ -153,6 +155,7 @@ >> List.of("-jar", "test.jar"), >> List.of("-m", "test/Foo"), >> List.of("--module", "test/Foo"), >> + List.of("--module=test/Foo"), >> List.of("--dry-run"), >> List.of("-h"), >> List.of("-?"), >> @@ -241,6 +244,69 @@ >> verifyOptions(List.of("--add-exports", >> "java.base/jdk.internal.misc=ALL-UNNAMED", "-jar", "test.jar"), tr); >> } >> >> + >> + @Test >> + // That that we can correctly handle the module longform argument option >> + // when supplied in an argument file >> + public void modulesInArgsFile() throws IOException { >> + File cwd = new File("."); >> + File testModuleDir = new File(cwd, "modules_test"); >> + >> + createEchoArgumentsModule(testModuleDir); >> + >> + Path SRC_DIR = Paths.get(testModuleDir.getAbsolutePath(), "src"); >> + Path MODS_DIR = Paths.get(testModuleDir.getAbsolutePath(), "mods"); >> + >> + // test module / main class >> + String MODULE_OPTION = "--module=test/launcher.Main"; >> + String TEST_MODULE = "test"; >> + >> + // javac -d mods/test src/test/** >> + TestResult tr = doExec( >> + javacCmd, >> + "-d", MODS_DIR.toString(), >> + "--module-source-path", SRC_DIR.toString(), >> + "--module", TEST_MODULE); >> + >> + if (!tr.isOK()) { >> + System.out.println("test did not compile"); >> + throw new RuntimeException("Error: modules test did not >> compile"); >> + } >> + >> + // verify the terminating ability of --module= through environment >> variables >> + File argFile = createArgFile("cmdargs", List.of("--module-path", >> MODS_DIR.toString(), MODULE_OPTION, "--hello")); >> + env.put(JDK_JAVA_OPTIONS, "@cmdargs"); >> + tr = doExec(env, javaCmd); >> + tr.checkNegative(); >> + tr.contains("Error: Option " + MODULE_OPTION + " in @cmdargs is not >> allowed in environment variable JDK_JAVA_OPTIONS"); >> + if (!tr.testStatus) { >> + System.out.println(tr); >> + throw new RuntimeException("test fails"); >> + } >> + >> + // check that specifying --module and --module-path with file works >> + tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs\""); >> + tr.contains("[--hello]"); >> + if (!tr.testStatus) { >> + System.out.println(tr); >> + throw new RuntimeException("test fails"); >> + } >> + >> + // check with reversed --module-path and --module in the arguments >> file, this will fail, --module= is terminating >> + File argFile1 = createArgFile("cmdargs1", List.of(MODULE_OPTION, >> "--module-path", MODS_DIR.toString(), "--hello")); >> + tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs1\""); >> + tr.checkNegative(); >> + if (!tr.testStatus) { >> + System.out.println(tr); >> + throw new RuntimeException("test fails"); >> + } >> + >> + // clean-up >> + argFile.delete(); >> + argFile1.delete(); >> + recursiveDelete(testModuleDir); >> + } >> + >> public static void main(String... args) throws Exception { >> init(); >> ArgsEnvVar a = new ArgsEnvVar(); >> diff -r bd436284147d test/jdk/tools/launcher/TestHelper.java >> --- a/test/jdk/tools/launcher/TestHelper.java Wed Nov 20 08:12:14 2019 >> +0800 >> +++ b/test/jdk/tools/launcher/TestHelper.java Wed Dec 11 12:09:29 2019 >> -0500 >> @@ -500,6 +500,40 @@ >> return Locale.getDefault().getLanguage().equals("en"); >> } >> >> + /** >> + * Helper method to initialize a simple module that just prints the >> passed in arguments >> + */ >> + static void createEchoArgumentsModule(File modulesDir) throws >> IOException { >> + if (modulesDir.exists()) { >> + recursiveDelete(modulesDir); >> + } >> + >> + modulesDir.mkdirs(); >> + >> + File srcDir = new File(modulesDir, "src"); >> + File modsDir = new File(modulesDir, "mods"); >> + File testDir = new File(srcDir, "test"); >> + File launcherTestDir = new File(testDir, "launcher"); >> + srcDir.mkdir(); >> + modsDir.mkdir(); >> + testDir.mkdir(); >> + launcherTestDir.mkdir(); >> + >> + String[] moduleInfoCode = { "module test {}" }; >> + createFile(new File(testDir, "module-info.java"), >> Arrays.asList(moduleInfoCode)); >> + >> + String[] moduleCode = { >> + "package launcher;", >> + "import java.util.Arrays;", >> + "public class Main {", >> + "public static void main(String[] args) {", >> + "System.out.println(Arrays.toString(args));", >> + "}", >> + "}" >> + }; >> + createFile(new File(launcherTestDir, "Main.java"), >> Arrays.asList(moduleCode)); >> + } >> + >> /* >> * A class to encapsulate the test results and stuff, with some ease >> * of use methods to check the test results. >> diff -r bd436284147d test/jdk/tools/launcher/TestSpecialArgs.java >> --- a/test/jdk/tools/launcher/TestSpecialArgs.java Wed Nov 20 08:12:14 >> 2019 +0800 >> +++ b/test/jdk/tools/launcher/TestSpecialArgs.java Wed Dec 11 12:09: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 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 <ksrini...@gmail.com> >> Sent: December 10, 2019 8:43 PM >> To: Nikola Grcevski <nikola.grcev...@microsoft.com> >> Cc: Henry Jen <henry....@oracle.com>; Alan Bateman >> <alan.bate...@oracle.com>; 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<String> 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;", >> .... >> .... >> } >> createFile(new File(launcherTestDir, "Main.java"), Arrays.asList(a2)); >> >> This might make the code neater. >> >> Besides that, looks very good. >> >> Thanks >> Kumar >> >> >> On Mon, Dec 9, 2019 at 1:58 PM Nikola Grcevski >> <mailto:nikola.grcev...@microsoft.com> wrote: >> Hi Henry and Kumar, >> >> Thank you again for the review! I have added the fix to isTerminalOpt and >> used both of your suggestions to make new tests. >> With native memory tracking enabled, I could actually see a crash on both >> Linux and Windows without the suggested fix. >> >> I tested the changes again on both Linux and Windows, and the new unit tests >> fail if isTerminalOpt doesn’t check for –module= >> as per Henry's suggestion. >> >> I'm attaching the new patch (my apologies for the size) at the bottom of >> this email after my signature. If I haven't covered >> certain aspects in the new tests please let me know, I'm more than happy to >> extend them further. I've updated the webrev >> to reflect the latest patch if it's easier to read: >> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrcevski.github.io%2FJDK-8234076%2Fwebrev%2F&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086613230&sdata=s0720HUSIGiCLzUS3t4PceseBVGJ30iyO%2F4QLqGKCuI%3D&reserved=0 >> >> Thanks again! >> 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 Mon Dec 09 16:08:54 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 Mon Dec 09 16:08:54 >> 2019 -0500 >> @@ -34,6 +34,7 @@ >> #include <sys/stat.h> >> #include <wtypes.h> >> #include <commctrl.h> >> +#include <assert.h> >> >> #include <jni.h> >> #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 Mon Dec 09 16:08:54 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 ArgsEnvVar extends TestHelper { >> private static File testJar = null; >> @@ -153,6 +155,7 @@ >> List.of("-jar", "test.jar"), >> List.of("-m", "test/Foo"), >> List.of("--module", "test/Foo"), >> + List.of("--module=test/Foo"), >> List.of("--dry-run"), >> List.of("-h"), >> List.of("-?"), >> @@ -241,6 +244,101 @@ >> verifyOptions(List.of("--add-exports", >> "java.base/jdk.internal.misc=ALL-UNNAMED", "-jar", "test.jar"), tr); >> } >> >> + /** >> + * Helper method to initialize a simple module that just prints the >> passed in arguments >> + */ >> + private void initModulesDir(File modulesDir) throws IOException { >> + if (modulesDir.exists()) { >> + recursiveDelete(modulesDir); >> + } >> + >> + modulesDir.mkdirs(); >> + >> + File srcDir = new File(modulesDir, "src"); >> + File modsDir = new File(modulesDir, "mods"); >> + File testDir = new File(srcDir, "test"); >> + File launcherTestDir = new File(testDir, "launcher"); >> + srcDir.mkdir(); >> + modsDir.mkdir(); >> + testDir.mkdir(); >> + launcherTestDir.mkdir(); >> + >> + ArrayList<String> 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); >> + } >> + >> + @Test >> + // That that we can correctly handle the module longform argument option >> + // when supplied in an argument file >> + public void modulesInArgsFile() throws IOException { >> + File cwd = new File("."); >> + File testModuleDir = new File(cwd, "modules_test"); >> + >> + initModulesDir(testModuleDir); >> + >> + Path SRC_DIR = Paths.get(testModuleDir.getAbsolutePath(), "src"); >> + Path MODS_DIR = Paths.get(testModuleDir.getAbsolutePath(), "mods"); >> + >> + // test module / main class >> + String MODULE_OPTION = "--module=test/launcher.Main"; >> + String TEST_MODULE = "test"; >> + >> + // javac -d mods/test src/test/** >> + TestResult tr = doExec( >> + javacCmd, >> + "-d", MODS_DIR.toString(), >> + "--module-source-path", SRC_DIR.toString(), >> + "--module", TEST_MODULE); >> + >> + if (!tr.isOK()) { >> + System.out.println("test did not compile"); >> + throw new RuntimeException("Error: modules test did not >> compile"); >> + } >> + >> + // verify the terminating ability of --module= through environment >> variables >> + File argFile = createArgFile("cmdargs", List.of("--module-path", >> MODS_DIR.toString(), MODULE_OPTION, "--hello")); >> + env.put(JDK_JAVA_OPTIONS, "@cmdargs"); >> + tr = doExec(env, javaCmd); >> + tr.checkNegative(); >> + tr.contains("Error: Option " + MODULE_OPTION + " in @cmdargs is not >> allowed in environment variable JDK_JAVA_OPTIONS"); >> + if (!tr.testStatus) { >> + System.out.println(tr); >> + throw new RuntimeException("test fails"); >> + } >> + >> + // check that specifying --module and --module-path with file works >> + tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs\""); >> + tr.contains("[--hello]"); >> + if (!tr.testStatus) { >> + System.out.println(tr); >> + throw new RuntimeException("test fails"); >> + } >> + >> + // check with reversed --module-path and --module in the arguments >> file, this will fail, --module= is terminating >> + File argFile1 = createArgFile("cmdargs1", List.of(MODULE_OPTION, >> "--module-path", MODS_DIR.toString(), "--hello")); >> + tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs1\""); >> + tr.checkNegative(); >> + if (!tr.testStatus) { >> + System.out.println(tr); >> + throw new RuntimeException("test fails"); >> + } >> + >> + // clean-up >> + argFile.delete(); >> + argFile1.delete(); >> + recursiveDelete(testModuleDir); >> + } >> + >> public static void main(String... args) throws Exception { >> init(); >> ArgsEnvVar a = new ArgsEnvVar(); >> diff -r bd436284147d test/jdk/tools/launcher/TestSpecialArgs.java >> --- a/test/jdk/tools/launcher/TestSpecialArgs.java Wed Nov 20 08:12:14 >> 2019 +0800 >> +++ b/test/jdk/tools/launcher/TestSpecialArgs.java Mon Dec 09 16:08:54 >> 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 Mon Dec 09 >> 16:08:54 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("https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fos.name&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086613230&sdata=voXq4xFGp38yvWJruKJslQY%2FOCiJXAxoBJz%2FWpkgM80%3D&reserved=0";, >> "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() + "file://*.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 <mailto:ksrini...@gmail.com> >> Sent: December 7, 2019 10:28 PM >> To: Henry Jen <mailto:henry....@oracle.com> >> Cc: Nikola Grcevski <mailto:nikola.grcev...@microsoft.com>; Alan Bateman >> <mailto:alan.bate...@oracle.com>; mailto: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&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086613230&sdata=vmbejj3xW9nZ%2FO9PiQByxWi1CBBnnqos%2Fm6EyJsuy3M%3D&reserved=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&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086613230&sdata=K2YKHqi7mOejpa8B8RTQhFxOg2gtuTRunx8%2Bv3pDmso%3D&reserved=0 >> >> Kumar Srinivasan >> >> On Fri, Dec 6, 2019 at 2:44 PM Henry Jen >> <mailto: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 >>> <mailto:mailto: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&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086613230&sdata=s0720HUSIGiCLzUS3t4PceseBVGJ30iyO%2F4QLqGKCuI%3D&reserved=0) >>> >>> Thanks again to everyone for your help with this bug. If there are any >>> additional comments or suggestions please let me know. >>> 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 Fri Dec 06 15:11:36 >>> 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) { >>> 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 Fri Dec 06 15:11:36 >>> 2019 -0500 >>> @@ -34,6 +34,7 @@ >>> #include <sys/stat.h> >>> #include <wtypes.h> >>> #include <commctrl.h> >>> +#include <assert.h> >>> >>> #include <jni.h> >>> #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/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 Fri Dec 06 >>> 15:11:36 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("https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fos.name&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086613230&sdata=voXq4xFGp38yvWJruKJslQY%2FOCiJXAxoBJz%2FWpkgM80%3D&reserved=0";, >>> "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() + "file://*.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 <mailto:mailto:henry....@oracle.com> >>> Sent: December 6, 2019 12:03 AM >>> To: Nikola Grcevski <mailto:mailto:nikola.grcev...@microsoft.com> >>> Cc: Kumar Srinivasan <mailto:mailto:ksrini...@gmail.com>; Alan Bateman >>> <mailto:mailto:alan.bate...@oracle.com>; >>> mailto:mailto:core-libs-dev@openjdk.java.net >>> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate >>> >>> >>>> On Dec 5, 2019, at 5:57 PM, Nikola Grcevski >>>> <mailto:mailto:nikola.grcev...@microsoft.com> 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 >>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fcr.openjdk.java.net&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086613230&sdata=QfNlx4YAu51NF%2BracTH2pN4T1NHuYCZUkvl2nSMB9C0%3D&reserved=0 >>> 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 <mailto:mailto:henry....@oracle.com> >>>> Sent: December 4, 2019 8:26 PM >>>> To: Kumar Srinivasan <mailto:mailto:ksrini...@gmail.com>; Alan Bateman >>>> <mailto:mailto:alan.bate...@oracle.com>; Nikola Grcevski >>>> <mailto:mailto:nikola.grcev...@microsoft.com> >>>> Cc: mailto:mailto:core-libs-dev@openjdk.java.net >>>> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate >>>> >>>>> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan >>>>> <mailto:mailto:ksrini...@gmail.com> 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 >>>> .https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Foracle.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086613230&sdata=OtYRav6tKD%2BthebvIhi376aRMVyPPRkrXnT8CDz9oWQ%3D&reserved=0 >>>> &data=02%7C01%7CNikola.Grcevski%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824629806&sdata=7VMZnWzyp9ZSG%2FzVaxUMFUuqeikdR%2FIvDGg2p0AqRTU%3D&reserved=0 >>>> e9b608d77a0999a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63711205 >>>> 3962510892&sdata=bh5Phj2Ti%2B%2FWKLK9VfWXIFXVfTRDBOjSkYTkrQ5k%2FvY >>>> %3D&reserved=0 >>>> >>>>> Kumar >>>>> >>>>> On Tue, Dec 3, 2019 at 5:29 PM Nikola Grcevski >>>>> <mailto:mailto: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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrc >>>>> e >>>>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvski.github.io&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086623229&sdata=FSUfXSdZEk%2F%2BRVJQ9eUN3iA1Sibo9DZ5BCi5GmDHv6Y%3D&reserved=0 >>>>> c >>>>> evski%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824629806&sdata=7VMZnWzyp9ZSG%2FzVaxUMFUuqeikdR%2FIvDGg2p0AqRTU%3D&reserved=0 >>>>> f >>>>> 141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=ee0dSSSJ >>>>> % >>>>> 2BXZogFtgPl8xFcUZ0P%2BX%2FR2G7x%2F4jjqiRIE%3D&reserved=0 >>>>> >>>>> Cheers, >>>>> >>>>> Nikola >>>>> >>>>> -----Original Message----- >>>>> From: Henry Jen <mailto:mailto:henry....@oracle.com> >>>>> Sent: December 3, 2019 11:39 AM >>>>> To: Kumar Srinivasan <mailto:mailto:ksrini...@gmail.com> >>>>> Cc: Nikola Grcevski <mailto:mailto:nikola.grcev...@microsoft.com>; Alan >>>>> Bateman >>>>> <mailto:mailto:alan.bate...@oracle.com>; >>>>> mailto:mailto: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 >>>>>> <mailto:mailto:ksrini...@gmail.com> 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%2Fbu >>>>>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgs.openjdk.java.net&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086623229&sdata=QqUa6h6WjyW2%2Fp9SktM7nLZzEnv0K1%2BAdFXkDQ8dI5s%3D&reserved=0 >>>>>> .Grcevski%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824639758&sdata=mH19URBhYu0rW%2Bpsl91V9e3TcwPD%2BDX0YSNbXao5H8o%3D&reserved=0 >>>>>> 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=b >>>>>> 0wl3x8AVS%2BJIrHHG5ivM%2FZtfgn2IRno2AauSVcRWlg%3D&reserved=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. >>>>>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fprinceton.edu&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086623229&sdata=cdMin3jUqzpvkWSzRgaSKWqYW8mhXx570UeetH7AbSY%3D&reserved=0 >>>>>> 2%7C01%7CNikola.Grcevski%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824649719&sdata=%2FUxEEDQrTgIk2Y%2Bd%2FPz67ocYVqRXncW%2FsITD%2BWpemSY%3D&reserved=0 >>>>>> 92228c5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797 >>>>>> 544&sdata=D1gprSmaQp1v9BB07EmYsX1%2FF4n9CGXMl8yIDJdnQ%2Bg%3D& >>>>>> ;reserved=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 >>>>>> .https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopenjdk.java.net&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086623229&sdata=NwkNTOf2qiPKtUctl7wecn0Oxu75cMubH1HzSMq2vFM%3D&reserved=0 >>>>>> Ftools%2Flauncher%2Fmodules%2Fbasic&data=02%7C01%7CNikola.Grcevs >>>>>> ki%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824659675&sdata=HvTbDg9uHe5WDu0OVcH%2BPF5mgYHWv2OUHYAWY2EheFo%3D&reserved=0 >>>>>> 41af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=L4SzTviE >>>>>> WgKoBrrZ2nU%2BPJFhairYv8ZPDqW%2FNtAXEN0%3D&reserved=0 >>>>>> Using the modules test framework may make the test simpler. >>>>>> >>>>>> Kumar Srinivasan >>>>>> >>>>>> >>>>>> On Mon, Dec 2, 2019 at 11:34 AM Nikola Grcevski >>>>>> <mailto:mailto: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%2Fgr >>>>>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fcevski.github.io&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C5f3bed3f0f724724dd3608d77f07a29f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637117543086623229&sdata=0Xpbx25XswIubMpWSiVzGRIisFvP2dNh6z8h%2F0kFm7Q%3D&reserved=0 >>>>>> .Grcevski%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824669630&sdata=XX0qBV1e5ucqPSL%2B4YciHNXR29evREuIRDFwTSxaqHU%3D&reserved=0 >>>>>> 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=e >>>>>> e0dSSSJ%2BXZogFtgPl8xFcUZ0P%2BX%2FR2G7x%2F4jjqiRIE%3D&reserved=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 <mailto:mailto:henry....@oracle.com> >>>>>> Sent: December 2, 2019 12:26 PM >>>>>> To: Alan Bateman <mailto:mailto:alan.bate...@oracle.com> >>>>>> Cc: Nikola Grcevski <mailto:mailto:nikola.grcev...@microsoft.com>; >>>>>> mailto:mailto: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 >>>>>>> <mailto:mailto:alan.bate...@oracle.com> 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 >
