Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java
Hi All, Here is the code to resolve JDK-8190187: http://cr.openjdk.java.net/~mhorie/8190187/hg_diff.txt Could a committer please take the fix and: 1) Complete the CSR process for the new JNI Return code. 2) Commit the changes that contain the Return code. And, optionally, 3) Perform 1 and 2 for the jdwp agent changes as well, so it can use this new functionality. Let me know if you need any help. :) Best Regards Adam Farley 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
Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java
Hi All, Here is the code to resolve JDK-8190187: http://cr.openjdk.java.net/~mhorie/8190187/hg_diff.txt Could a committer please take the fix and: 1) Complete the CSR process for the new JNI Return code. 2) Commit the changes that contain the Return code. And, optionally, 3) Perform 1 and 2 for the jdwp agent changes as well, so it can use this new functionality. Let me know if you need any help. :) Best Regards Adam Farley 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
Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java
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. >> ``` >> >> 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. >> >> ``` > The "Agent Start-Up" section is the section to look at. The important part is: > > "The return value from Agent_OnLoad or Agent_OnLoad_ 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:, >>> _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
Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java
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. >> ``` >> >> 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. >> >> ``` > The "Agent Start-Up" section is the section to look at. The important part is: > > "The return value from Agent_OnLoad or Agent_OnLoad_ 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:, >>> _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
Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java
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. ```
Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java
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. ``` 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. ``` > 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) ``` > > 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:, > _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. If you have Author or Committer privileges, could you add the files to the bug please? > >> Backporting would be appreciated, but is optional. > I don't think these changes are appropriate to backport as they include > specification changes/ > >-Alan This is fair. - Adam 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
Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java
On 15/02/2018 12:13 AM, 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. > > As I stated here: > > https://urldefense.proofpoint.com/v2/url?u=http-3A__mail.openjdk.java.net_pipermail_core-2Dlibs-2Ddev_2017-2DOctober_049597.html=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=P5m8KWUXJf-CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=OUKJytaKUZmaM4zCoF2GSmIrYLPEE4cSP1LkBgAIduo=DIuL0lNnE0xXAqlADCHi8uv2u6kcKcOWbob0WHHArSU= > > "I can file a bug for this, but it will take some work to see how to fit > this into the existing specifications and file CSR requests for those > changes. This won't make 18.3 (or whatever it gets called). You will > need a "champion" to help flesh this out in full and work with you on > those CSR requests. I can't volunteer to do that at this time." > > Nobody has offered to champion this. Sorry. > > David > - Yup, I remember you saying that David. I plan to work on the issues you raised in your previous email, and continue to post messages in these lists until someone who has free cycles spots my emails and agrees to commit the code. It'll happen sooner or later. :) - Adam > >> Backporting would be appreciated, but is optional. >> >> Also optional: Commit the jdwp code that serves as an example for how this >> code should be used. CSR again, unfortunately. >> >> >> -- Long Version -- >> >> We have a bug in OpenJDK where if you pass an info-only option (like >> -agentlib:jdwp=help) in through the JNI interface, it can exit your code >> with RC 0. >> >> I think this is a bug because if you planned to do anything after starting >> a Java VM, you have to do it in an exit hook. >> >> If an info-only option is used, exit(0) should not happen. Instead, the >> user's code should be told the VM cannot be used. >> >> Bug Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8190187=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=P5m8KWUXJf-CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=OUKJytaKUZmaM4zCoF2GSmIrYLPEE4cSP1LkBgAIduo=P_S35LF3YxaxKQWY7-svCzm_WvvcUU0nSx8QP9dH9O0= >> >> I have proposed the creation of a new JNI return code indicating a "silent >> exit", where the vm encountered no error, but that it is not in a fit >> state to be used. >> >> See the link for an implementation of this, along with a handy test. >> >> In short, if you agree that this is a problem, we need a champion to: >> >> 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. >> >> Optionally, if you want to commit the code containing an example of the >> fix, you will need to: >> >> 3) Commit the Hotspot code that channels the new return code. >> 4) Complete the CSR process for the jdwp behaviour change. >> 5) Commit the jdwp code. >> >> Note that all of this code has *already been written*. This should be an >> easy commit once the CSR is done with. >> >> Best Regards >> >> Adam Farley >> >> >> >> Best Regards >> >> Adam Farley >> >> 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
Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java
On 15/02/2018 12:13 AM, 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. As I stated here: http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049597.html "I can file a bug for this, but it will take some work to see how to fit this into the existing specifications and file CSR requests for those changes. This won't make 18.3 (or whatever it gets called). You will need a "champion" to help flesh this out in full and work with you on those CSR requests. I can't volunteer to do that at this time." Nobody has offered to champion this. Sorry. David - Backporting would be appreciated, but is optional. Also optional: Commit the jdwp code that serves as an example for how this code should be used. CSR again, unfortunately. -- Long Version -- We have a bug in OpenJDK where if you pass an info-only option (like -agentlib:jdwp=help) in through the JNI interface, it can exit your code with RC 0. I think this is a bug because if you planned to do anything after starting a Java VM, you have to do it in an exit hook. If an info-only option is used, exit(0) should not happen. Instead, the user's code should be told the VM cannot be used. Bug Link: https://bugs.openjdk.java.net/browse/JDK-8190187 I have proposed the creation of a new JNI return code indicating a "silent exit", where the vm encountered no error, but that it is not in a fit state to be used. See the link for an implementation of this, along with a handy test. In short, if you agree that this is a problem, we need a champion to: 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. Optionally, if you want to commit the code containing an example of the fix, you will need to: 3) Commit the Hotspot code that channels the new return code. 4) Complete the CSR process for the jdwp behaviour change. 5) Commit the jdwp code. Note that all of this code has *already been written*. This should be an easy commit once the CSR is done with. Best Regards Adam Farley Best Regards Adam Farley 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
Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java
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). There is also text to be written for the JNI spec if this proposal goes ahead. 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:, _JAVA_TOOLS_OPTIONS, ...). Backporting would be appreciated, but is optional. I don't think these changes are appropriate to backport as they include specification changes/ -Alan
