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. http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/37d1442d53bc/test/tools/launcher/TestSpecialArgs.java#l243 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, http://hg.openjdk.java.net/jdk/jdk/file/31882abe1494/test/jdk/tools/launcher/TestHelper.java Kumar Srinivasan On Fri, Dec 6, 2019 at 2:44 PM Henry Jen <henry....@oracle.com> wrote: > Thanks for the work, the test case covers that when —module= is used > before other options, which is good. > > But we need another to make sure that when —module= is put in an argument > file or environment variable, that should not be allowed. The fix is > simple, we need to add that to isTerminalOp() method. > > Cheers, > Henry > > > On Dec 6, 2019, at 12:28 PM, Nikola Grcevski < > nikola.grcev...@microsoft.com> wrote: > > > > Thank you Henry! > > > > I have modified the fix in checkArg to use JLI_StrCCmp as suggested > below and I have extended the BasicTest.java (in > test/jdk/tools/launcher/modules/basic) with few additional tests. > > > > I don't have author status yet, therefore I'm attaching the patch right > after my signature. I also updated my external webrev if that's easier to > review (https://grcevski.github.io/JDK-8234076/webrev/) > > > > 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("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); > > + } > > } > > > > > > -----Original Message----- > > From: Henry Jen <henry....@oracle.com> > > Sent: December 6, 2019 12:03 AM > > To: Nikola Grcevski <nikola.grcev...@microsoft.com> > > Cc: Kumar Srinivasan <ksrini...@gmail.com>; Alan Bateman < > alan.bate...@oracle.com>; core-libs-dev@openjdk.java.net > > Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate > > > > > >> On Dec 5, 2019, at 5:57 PM, Nikola Grcevski < > 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 cr.openjdk.java.net or you can > attach the patch to the review thread if you are not yet an author. > > > > Cheers, > > Henry > > > > > >> + idx = argsCount; > >> } > >> } else { > >> if (!expectingNoDashArg) { > >> > >> The change is in common code and simply checks for the usage of > --module= and deems the next argument as the start of the application > arguments. I can confirm that using -m instead of --module doesn't allow > for the = separator to be used, so we only need to check for --module= if > this is a desired change. > >> > >> I tested with various combinations on the command line and I couldn't > find a set of arguments ordering that breaks the terminating quality of > --module. > >> > >> I also performed series of tests to try to break the argument expansion > on Windows with this change, but it worked in all instances. The testcase > is working correctly with this change, as well as the javac launcher > command as proposed by Kumar (java --module-path=mods > --module=jdk.compiler/com.sun.tools.javac.Main *.java). > >> > >> I ran all the launcher tests on both Windows and Linux and all tests > pass. > >> > >> Please let me know if this is a preferred approach to address this bug > or if you think there's a potential problem with the change. If this is an > acceptable fix I can create new webrev with set of tests for the various > edge cases I tried, and new launcher specific tests that ensure argument > expansion is performed on Windows with this module usage. > >> > >> Thank you, > >> Nikola > >> > >> -----Original Message----- > >> From: Henry Jen <henry....@oracle.com> > >> Sent: December 4, 2019 8:26 PM > >> To: Kumar Srinivasan <ksrini...@gmail.com>; Alan Bateman > >> <alan.bate...@oracle.com>; Nikola Grcevski > >> <nikola.grcev...@microsoft.com> > >> Cc: core-libs-dev@openjdk.java.net > >> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate > >> > >>> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan <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 > >> .oracle.com%2Fen%2Fjava%2Fjavase%2F13%2Fdocs%2Fspecs%2Fman%2Fjava.html > >> &data=02%7C01%7CNikola.Grcevski%40microsoft.com%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 < > 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 > >>> vski.github.io%2FJDK-8234076%2Fwebrev%2F&data=02%7C01%7CNikola.Gr > >>> c > >>> evski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86 > >>> f > >>> 141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=ee0dSSSJ > >>> % > >>> 2BXZogFtgPl8xFcUZ0P%2BX%2FR2G7x%2F4jjqiRIE%3D&reserved=0 > >>> > >>> Cheers, > >>> > >>> Nikola > >>> > >>> -----Original Message----- > >>> From: Henry Jen <henry....@oracle.com> > >>> Sent: December 3, 2019 11:39 AM > >>> To: Kumar Srinivasan <ksrini...@gmail.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 > >>> > >>> 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 <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 > >>>> gs.openjdk.java.net%2Fbrowse%2FJDK-7146424&data=02%7C01%7CNikola > >>>> .Grcevski%40microsoft.com%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. > >>>> princeton.edu%2F~hos%2Fmike%2Ftranscripts%2Fkernighan.htm&data=0 > >>>> 2%7C01%7CNikola.Grcevski%40microsoft.com%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 > >>>> .openjdk.java.net%2Fjdk%2Fjdk13%2Ffile%2F0368f3a073a9%2Ftest%2Fjdk%2 > >>>> Ftools%2Flauncher%2Fmodules%2Fbasic&data=02%7C01%7CNikola.Grcevs > >>>> ki%40microsoft.com%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 < > 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 > >>>> cevski.github.io%2FJDK-8234076%2Fwebrev%2F&data=02%7C01%7CNikola > >>>> .Grcevski%40microsoft.com%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 <henry....@oracle.com> > >>>> Sent: December 2, 2019 12:26 PM > >>>> To: Alan Bateman <alan.bate...@oracle.com> > >>>> Cc: Nikola Grcevski <nikola.grcev...@microsoft.com>; > >>>> 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 <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 > >
