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. 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://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 > > > >
