Hi Kumar,

Thank you so much for your input! You are absolutely correct, this fix doesn’t 
fix the argument expansion issue on Windows, it only fixes the crash. I made a 
similar comment in my original email, fixing the argument expansion would be a 
bit more substantial change. I’m more than happy to do it, I would just need 
some guidance on what testing needs to be done to ensure that the argument 
expansion fix doesn’t break some existing behaviour.

I also wanted to clarify that, the command line as specified below, doesn’t 
expose the issue with negative array index that was reported in JDK- 8234076, 
however a later check already present in the code prevents argument expansion 
to run on Windows.

I must apologize, I’m new to this project and I really don’t know what’s the 
appropriate team etiquette in OpenJDK for handling situations like these. I’ve 
worked on teams where when a specific issue is reported other uncovered 
problems are fixed too, and also on teams where people tend to fix the reported 
problem and then file new bug reports for other uncovered problems. I’m happy 
to do either. 

Thank you!
Nikola

From: Kumar Srinivasan <ksrini...@gmail.com> 
Sent: December 3, 2019 10:14 AM
To: Nikola Grcevski <nikola.grcev...@microsoft.com>
Cc: Henry Jen <henry....@oracle.com>; Alan Bateman <alan.bate...@oracle.com>; 
core-libs-dev@openjdk.java.net
Subject: Re: [EXTERNAL] Re: JDK-8234076 bug fix candidate

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

Reply via email to