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://grcevski.github.io/JDK-8234076/webrev/ 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%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-7146424&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293&sdata=9KSksL8%2BCmXSscF8oGGn5piLz2wApQ9paJUyZWbKWCw%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=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293&sdata=l20J1AN4vBmT19gzBxLOktBsdv260F2rMWRvCLeVb84%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%2Ftools%2Flauncher%2Fmodules%2Fbasic&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293&sdata=jsOjS1rgX4tfzJwE8Xif3NARZPRHb39Y64LvSdz1Jic%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%2Fgrcevski.github.io%2FJDK-8234076%2Fwebrev%2F&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293&sdata=lx%2FFVo5UOw3uhxgttVm2RKkoFPu8tmQtx0OwMvbTwJs%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 >
