RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java

[Adam Farley8]

Bump :)



Best Regards

Adam Farley

---- Last email ----

Hi Alan

Thanks for getting back to me on this. :)

I've changed the hg_diff as described below, see the attached.

> On 27/02/2018 15:04, Adam Farley8 wrote:
>
> Resending. Bump. :) 
>
> On 14/02/2018 14:13, Adam Farley8 wrote: 
>>> Hi All, 
>>> 
>>> -- Short version -- 
>>> 
>>> Could a committer please take the fix for JDK-8190187 (full code 
included 
>>> in the bug) and: 
>>> 
>>> 1) Complete the CSR process for the new JNI Return code. 
>>> 2) Commit the changes that contain (a) the new return code, and (b) 
the 
>>> non-Hotspot code that handles the new code. 
>> The patches attached to the JIRA issue are missing the changes to the 
>> JVM TI spec (jvmti.xml). 
>
>> I'm not seeing the JNI return codes in that file. Are you after one of 
those 
>> dated updates near the bottom? 
>> e.g. 
>> ``` 
>>  <change date="14 February 2018" version="11.0.0"> 
>>      Added JNI_SILENT_EXIT return code for early exits without errors. 
>>      java.c's ParseArguments function now sets pret value to 2 for a 
NULL pwhat. 
>>         This allows us to clearly identify when no class was set, but 
no other error has occurred. 
>>         This is undone in java.c's ContinueInNewThread method, so the 
surface behaviour does not change. 
>>  </change> 
>> ```
> The "Agent Start-Up" section is the section to look at. The important 
part is:
>
> "The return value from Agent_OnLoad or Agent_OnLoad_<agent-lib-name> is 
used to indicate an error. Any value other than zero indicates an error 
and causes termination of the VM."
>
> If there is special return value to mean "VM terminates without error" 
then this part of the spec will need to be adjusted. 

Ah, that makes sense. I altered that bit and regenerated the hg_diff.

> An additional point is that you can start several agents from the 
command line, does the VM terminate after it has started all agents or 
does it terminate when the first agent returns asks the VM to terminate 
quietly?

If I'm reading the code correctly, the loop that initialises the different 
agents 
(which I believe to be the loop containing "// Invoke the Agent_OnLoad 
function")
is not interrupted by the return of a silent exit code, however it only 
takes one 
agent returning this code to cause the VM to be destroyed once startup is
complete.

>
>
>
>
>
>>> There is also text to be written for the JNI spec if this proposal 
goes 
>>> ahead. 
>
>> I assume you mean the "RETURNS" section of the JNI_CreateJavaVM 
>> bit on the invocation.html web page. Something like this? 
>
>> ``` 
>> RETURNS: 
>> Returns JNI_OK on success; returns a suitable JNI error code (a 
negative number) on failure. 
>>
>> The sole exception is a silent exit, which returns JNI error code 
JNI_SILENT_EXIT. 
>> This indicates that the VM cannot be used, but that this is the 
intended behaviour for the 
>> arguments used. E.g. -Xlog:help (which prints help output and then 
destroys the VM) 
>> ```
> Yes, this is the place that will need changes.
>

Ok. The most official-looking place for this documentation is on the 
Oracle website. I'm not sure how to go about making this change
happen though.

Is the change to this documentation something I can push through 
one of the mailing lists? Or is it perhaps part of the CSR process?

>
>
>
>
>>> 
>>> I don't agree that the launcher should be looking for 
>>> "-agentlib:jdwp=help" in the command line as it's just one of many 
ways 
>>> that the debugger agent might be started (e.g. -Xrunjdwp:<options>, 
>>> _JAVA_TOOLS_OPTIONS, ...). 
>
>> We can avoid that by finding a way around this line in 
ContinueInNewThread (java.c): 
>
>> ``` 
>> return (ret != 0) ? ret : rslt; 
>> ``` 
>
>> I have devised a means to do this, as outlined in the jvmti.xml change 
above. I put the 
>> changes into a recent clone of jdk/jdk, and attached the hg diff, along 
with an improved 
>> test. 
>
> The launcher should only need to look at the return value from JNI 
CreateJavaVM. I don't think it should do any special handling for the JDWP 
or other agents (there are just too many ways to inject command lines and 
the launcher cannot be expected to handle all of them).
>
>-Alan
>

I agree. The change I made in response to your earlier comment is not 
jdwp-specific.
Rather, it sets "ret" to "2" if the user has not specified an executable 
class in the 
command line. I did this because a ret value of "1" indicates an error, 
but we don't 
want to override a return code of JNI_SILENT_EXIT with ret's value if the 
only error
was "no class was specified".

So I set ret to "2" if no class is specified, and altered the 
aforementioned bit in
ContinueInNewThread (java.c) to pick up on a ret value of 2. If the ret 
value is 2,
and rslt is JNI_SILENT_EXIT, then we know that no error has occurred 
(other
than "no class was specified"), and that JNI_SILENT_EXIT should be 
returned,
as opposed to the current functionality, where it is the non-0 ret value 
which
is returned (erroneously, in the event of a silent exit).

I think my changeset explains this more concisely than I have. :)
Let me know if this has just made you more confused. :)

- Adam

[attachment "hg_diff.txt" deleted by Adam Farley8/UK/IBM] 

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU


Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: hg_diff.txt
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20180319/c3907b90/hg_diff-0001.txt>