[EXTERNAL] JDK-8234076 bug fix candidate

Henry Jen henry.jen at oracle.com
Fri Dec 6 05:03:03 UTC 2019


> On Dec 5, 2019, at 5:57 PM, Nikola Grcevski <Nikola.Grcevski at microsoft.com> wrote:
> 
> Hello all again! 
> 
> Based on the suggestion by Kumar I made the following small patch to checkArg src/java.base/share/native/libjli/args.c:
> 
> --- a/src/java.base/share/native/libjli/args.c
> +++ b/src/java.base/share/native/libjli/args.c
> @@ -130,6 +130,8 @@ static void checkArg(const char *arg) {
>             }
>         } else if (JLI_StrCmp(arg, "--disable- at files") == 0) {
>             stopExpansion = JNI_TRUE;
> +        } else if (JLI_StrNCmp(arg, "--module=", 9) == 0) {

I would suggest use JLI_StrCCmp as in java.c. I think combine this fix with origin crash prevention for -1 is a safe approach and most compatible to current behavior.

BTW, we need the patch to be hosted on cr.openjdk.java.net or you can attach the patch to the review thread if you are not yet an author.

Cheers,
Henry


> +            idx = argsCount;
>         }
>     } else {
>         if (!expectingNoDashArg) {
> 
> The change is in common code and simply checks for the usage of --module= and deems the next argument as the start of the application arguments. I can confirm that using -m instead of --module doesn't allow for the = separator to be used, so we only need to check for --module= if this is a desired change.
> 
> I tested with various combinations on the command line and I couldn't find a set of arguments ordering that breaks the terminating quality of --module.
> 
> I also performed series of tests to try to break the argument expansion on Windows with this change, but it worked in all instances. The testcase is working correctly with this change, as well as the javac launcher command as proposed by Kumar (java --module-path=mods --module=jdk.compiler/com.sun.tools.javac.Main *.java).
> 
> I ran all the launcher tests on both Windows and Linux and all tests pass.
> 
> Please let me know if this is a preferred approach to address this bug or if you think there's a potential problem with the change. If this is an acceptable fix I can create new webrev with set of tests for the various edge cases I tried, and new launcher specific tests that ensure argument expansion is performed on Windows with this module usage.
> 
> Thank you,
> Nikola
> 
> -----Original Message-----
> From: Henry Jen <henry.jen at oracle.com> 
> Sent: December 4, 2019 8:26 PM
> To: Kumar Srinivasan <ksrinifmt at gmail.com>; Alan Bateman <Alan.Bateman at oracle.com>; Nikola Grcevski <Nikola.Grcevski at microsoft.com>
> Cc: core-libs-dev at openjdk.java.net
> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
> 
>> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan <ksrinifmt at gmail.com> wrote:
>> 
>> 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.
>> 
> 
> I don’t disagree, if we can decide whether —module= is allowed. Based on my understanding and the document for java[1], the —module= is not necessarily correct.
> 
> If we decided to accept it, the fix would be make sure the index set correctly as Kumar suggested, and the fix can be relatively simple.
> 
> FWIW, it’s still possible the index is NOT_FOUND if there is no main class specified, but that should never cause crash as if no main class is found, the launcher should either execute other terminal argument or display help.
> 
> I agree the fix is not complete as it only make sure no crash can happen. It doesn’t actually make —module= illegal and show help in case of that. At this point, there is a discrepancy in launcher code regard —module usage, and we need to fix that.
> 
> I’ll let Alan to comment about the —module option usage.
> 
> The webrev looks good to me, although I would like to see the test be more close to other arguments processing test, but since this can only be reproduced with —module= usage, I assume this is not bad.
> 
> [1] https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oracle.com%2Fen%2Fjava%2Fjavase%2F13%2Fdocs%2Fspecs%2Fman%2Fjava.html&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=uO2tgi1QNvXgI0wT%2FxOxB%2Bh10YpCbq37uzkKKlByUYg%3D&reserved=0
> 
>> 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrce
>> vski.github.io%2FJDK-8234076%2Fwebrev%2F&data=02%7C01%7CNikola.Grc
>> evski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f
>> 141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=ee0dSSSJ%
>> 2BXZogFtgPl8xFcUZ0P%2BX%2FR2G7x%2F4jjqiRIE%3D&reserved=0
>> 
>> 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%2Fbu
>>> gs.openjdk.java.net%2Fbrowse%2FJDK-7146424&data=02%7C01%7CNikola
>>> .Grcevski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f98
>>> 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=b
>>> 0wl3x8AVS%2BJIrHHG5ivM%2FZtfgn2IRno2AauSVcRWlg%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=0
>>> 2%7C01%7CNikola.Grcevski%40microsoft.com%7C6158f8460dcd4c39518708d77
>>> 92228c5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797
>>> 544&sdata=D1gprSmaQp1v9BB07EmYsX1%2FF4n9CGXMl8yIDJdnQ%2Bg%3D&amp
>>> ;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%2
>>> Ftools%2Flauncher%2Fmodules%2Fbasic&data=02%7C01%7CNikola.Grcevs
>>> ki%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f1
>>> 41af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=L4SzTviE
>>> WgKoBrrZ2nU%2BPJFhairYv8ZPDqW%2FNtAXEN0%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%2Fgr
>>> cevski.github.io%2FJDK-8234076%2Fwebrev%2F&data=02%7C01%7CNikola
>>> .Grcevski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f98
>>> 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=e
>>> e0dSSSJ%2BXZogFtgPl8xFcUZ0P%2BX%2FR2G7x%2F4jjqiRIE%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



More information about the core-libs-dev mailing list