[EXTERNAL] JDK-8234076 bug fix candidate

Wed Dec 4 21:15:20 UTC 2019

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.Grcevski at 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.jen at oracle.com>
> Sent: December 3, 2019 11:39 AM
> To: Kumar Srinivasan <ksrinifmt at gmail.com>
> Cc: Nikola Grcevski <Nikola.Grcevski at microsoft.com>; Alan Bateman <
> Alan.Bateman at oracle.com>; core-libs-dev at 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 <ksrinifmt at 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.Grcevski at 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.jen at oracle.com>
> > Sent: December 2, 2019 12:26 PM
> > To: Alan Bateman <Alan.Bateman at oracle.com>
> > Cc: Nikola Grcevski <Nikola.Grcevski at microsoft.com>;
> core-libs-dev at 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.Bateman at 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
> >
>
>