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
