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://bugs.openjdk.java.net/browse/JDK-7146424 > Please see all the related bugs in the above JIRA issue. > > Paragraph #6 in this interview surmises the wild card behavior on Windows: > https://www.princeton.edu/~hos/mike/transcripts/kernighan.htm > > 2. Though the arguments related tests are in Aaarghs.java the modules related > tests for the launcher are at: > https://hg.openjdk.java.net/jdk/jdk13/file/0368f3a073a9/test/jdk/tools/launcher/modules/basic > 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://grcevski.github.io/JDK-8234076/webrev/ > > 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 >
