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://grcevski.github.io/JDK-8234076/webrev/ > > 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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824589979&sdata=XcriPrQ%2B9rypJN6IShAKU2%2Fz5sZE%2BMCWT3gHyVg3Y3s%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824589979&sdata=kgi%2B5Ob%2FqX45oTtLSTalwXeQqIct1EUD1k30KOddm5c%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824599960&sdata=P3l7Y932pcZ14WwbszC0%2BIN%2BUcS2QrAizwT0Idqa6yQ%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824599960&sdata=iaKDsz3aVTdEumV4eOMj5SbA9WtSHT6ADA4KEMoqwgw%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824609890&sdata=KrUiucM4CY4G1aADtTMhHwJxr3e7ATJDMMxX5uG3b14%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824609890&sdata=3FmeinWv8eFAn8IWUozJWE2OKKaOP9QUFV75TIpu%2Bjk%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824619848&sdata=iuIbGg8zzbN7EPOUkKuXfzAUknZDXckOdwBX2XhwGrE%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824619848&sdata=362LaE5nipNO0eVmKa%2BmTTwhhBCFfAGdFMFHd%2Bumx4A%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824629806&sdata=WMlvgPM2Ixt0WaZiD61ZLjRmSgRL3UDmrNITh7H%2Ft38%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824639758&sdata=QyeJDvywLhjIe02uwSYhV2mI2ERcgOhObjk2lDWF8oQ%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824649719&sdata=K1mAbE9L7km1jXGixjKlWpUFsEpSKxwpsi%2Bqby35nlo%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824659675&sdata=ED1VtZLjyMmo3uRMRQ02rMCeeHrk0HIlQO8mv2tnPKA%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%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824669630&sdata=fi870VNLFmOgySkWY1LJJ2qBSKZkdTaIEW6ytg%2FMHDA%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
