[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 < 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://grcevski.github.io/JDK-8234076/webrev/ > > 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("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 7, 2019 10:28 PM > To: Henry Jen <henry....@oracle.com> > Cc: Nikola Grcevski <nikola.grcev...@microsoft.com>; 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&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875799903&sdata=WOjzJJtIY0y4rB2liNkH4nUMNLq2uEnJ8J01gWFgt5w%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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875809897&sdata=1GLeqSPxVRhGVgE7Cxf6w5l%2F34uOHCGTq2fRIjoxaGg%3D&reserved=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 <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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875809897&sdata=4iDS4fvjQlBxrGBhaLz3lHIWM7gNvFrvz01cqw%2B6O34%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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875809897&sdata=uU7AkmEbqKgfgeONT8Cvkr0YX57x1xNmtLkqq1AMIsg%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() + "\\*.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:henry....@oracle.com> > > Sent: December 6, 2019 12:03 AM > > To: Nikola Grcevski <mailto:nikola.grcev...@microsoft.com> > > Cc: Kumar Srinivasan <mailto:ksrini...@gmail.com>; Alan Bateman <mailto: > alan.bate...@oracle.com>; 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: > 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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875819891&sdata=tijtj%2BHkXylb4Qa3liw8EetRWX8bQlsrYD%2FkAgwnrGE%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:henry....@oracle.com> > >> Sent: December 4, 2019 8:26 PM > >> To: Kumar Srinivasan <mailto:ksrini...@gmail.com>; Alan Bateman > >> <mailto:alan.bate...@oracle.com>; Nikola Grcevski > >> <mailto:nikola.grcev...@microsoft.com> > >> Cc: 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: > 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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875819891&sdata=tYr7X%2BWKsRBDhE4kk8ES09i8kU3AXUbvfjON%2B3pi3rQ%3D&reserved=0%2Fen%2Fjava%2Fjavase%2F13%2Fdocs%2Fspecs%2Fman%2Fjava.html > >> &data=02%7C01%7CNikola.Grcevski% > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875829886&sdata=3GAF3YmLHzB0dGE%2FrBgKZb6I%2FfioqRqg1quz5zEI%2F%2Fs%3D&reserved=0%7C37e38c582bac4687 > >> 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: > 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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875829886&sdata=bO71uLpbZgNDfN6uuYmhGXWBBUbc8jVpXthnzMb%2B3fw%3D&reserved=0%2FJDK-8234076%2Fwebrev%2F&data=02%7C01%7CNikola.Gr > >>> c > >>> evski% > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875839882&sdata=B9ioFEVQ9x7taxjCeY5KHvHxOhmmG0Ohapg%2BEPsynfc%3D&reserved=0%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86 > >>> f > >>> 141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=ee0dSSSJ > >>> % > >>> 2BXZogFtgPl8xFcUZ0P%2BX%2FR2G7x%2F4jjqiRIE%3D&reserved=0 > >>> > >>> Cheers, > >>> > >>> Nikola > >>> > >>> -----Original Message----- > >>> From: Henry Jen <mailto:henry....@oracle.com> > >>> Sent: December 3, 2019 11:39 AM > >>> To: Kumar Srinivasan <mailto:ksrini...@gmail.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 > >>> > >>> 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: > 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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875839882&sdata=N3%2F56AMtNgfBH%2FNYNJj8%2Fviw07WmugWhKi1Pwz11ecw%3D&reserved=0%2Fbrowse%2FJDK-7146424&data=02%7C01%7CNikola > >>>> .Grcevski% > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875849875&sdata=b2IlaMBcOlTUjNI3gG6Kxkoza8w0N4tkeFX6dUrRQl0%3D&reserved=0%7C6158f8460dcd4c39518708d7792228c5%7C72f98 > >>>> 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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875849875&sdata=MU2LIp0wJy1%2FW1B3648xH%2BpM%2F%2F3OvpYg6Bg81sREAbk%3D&reserved=0%2F~hos%2Fmike%2Ftranscripts%2Fkernighan.htm&data=0 > >>>> 2%7C01%7CNikola.Grcevski% > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875859867&sdata=tQ%2FiuszakwoOxL9dQT1NAmXCDj0qOv%2BpRmwCBTbyHaA%3D&reserved=0%7C6158f8460dcd4c39518708d77 > >>>> 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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875859867&sdata=4MYjDinmHFMeWZ3LxDoJMbsw8Fiu%2FEX6YVQnPNbn7Ag%3D&reserved=0%2Fjdk%2Fjdk13%2Ffile%2F0368f3a073a9%2Ftest%2Fjdk%2 > >>>> 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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875869862&sdata=yMmLqFu99DcB7a6GqXiOZlolyxDKD0zfNliRsz1NuuE%3D&reserved=0%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f1 > >>>> 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: > 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%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875869862&sdata=F2CPkGKG7IdCZUtCtX36eWNoCizZrOSwK1sHyQ1DqQQ%3D&reserved=0%2FJDK-8234076%2Fwebrev%2F&data=02%7C01%7CNikola > >>>> .Grcevski% > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Ca7c43b46284843df631308d77b8ea31a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637113724875869862&sdata=yMmLqFu99DcB7a6GqXiOZlolyxDKD0zfNliRsz1NuuE%3D&reserved=0%7C6158f8460dcd4c39518708d7792228c5%7C72f98 > >>>> 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:henry....@oracle.com> > >>>> Sent: December 2, 2019 12:26 PM > >>>> To: Alan Bateman <mailto:alan.bate...@oracle.com> > >>>> Cc: Nikola Grcevski <mailto:nikola.grcev...@microsoft.com>; > >>>> 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: > 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 >