[BUG PROPOSAL]: C++ code that calls JNI_CreateJavaVM can be exited by java

[David Holmes]

Hi Adam,

I am still very much torn over this one. I think the idea of 
print-and-exit flags for a potentially hosted library like the JVM is 
just wrong - we should never have done that, but we did. Fixing that by 
moving the flags to the launcher is far from trivial**. Endorsing and 
encouraging these sorts of flag by adding JNI support seems to be 
sending the wrong message.

** I can envisage a "help xxx" Dcmd that can read back the info from the 
VM. The launcher can send the Dcmd, print the output and exit. The 
launcher would not need to know what the xxx values mean, but would have 
to intercept the existing ones.

Another option is just to be aware of these flags (are there more than 
jdwp and Xlog?) and deal with them specially in your custom launcher - 
either filter them out and ignore them, or else launch the VM in its own 
process to respond to them.

Any changes to the JNI specification need to go through the CSR process.

Cheers,
David

On 14/09/2017 6:26 PM, Adam Farley8 wrote:
> Hi All,
> 
> I was advised (on the OpenJDK IRC channel) that supplying a fix is 
> better than
> proposing the idea of one, so I've gone ahead and written a "silent 
> exit" fix for the
> example case:
> 
> java -agentlib:jdwp=help
> 
> This fix solves that bug, and also creates the tools for other code, VM and
> otherwise, to be able to solve their exit(0) problems as well.
> 
> I've attached the hg diffs to this email, along with a zip containing 
> the test
> I wrote to exercise this fix. The test, once unzipped, can be run via the
> TestStart.sh file, and is a bash script intended for linux execution. 
> Run it like this:
> 
> bash TestStart.sh /location/of/java
> 
> To be clear, the Java directory is the one that contains the bin directory.
> 
> Best Regards
> 
> Adam Farley
> 
> 
> 
> P.S. I debated returning a jni return code from the debugInit.c's 
> parseOptions
> method, but elected not to on the basis that (a) a seperate isHelp method
> allows us to quit the OnLoad before the agent does anything that would
> require us figuring out how to unload half an agent, and (b) I thought 
> it best to
> modify as few apis as possible. This is up for debate if people think a 
> jni return
> code is a better option here.
> 
> P.P.S. I know the files get stripped from list emails. If anyone wants 
> copies,
> email me and they're yours.
> 
> 
> ------------------ Previous email ------------------
> 
> Hi All,
> 
> I've included the full text of my reply in-line below.
> 
> A summary is: I continue to support the idea of a new return code on the 
> basis
> that, when the VM is nonusable but no error has occurred, we have no 
> suitable
> option.
> 
> Right now we can:
> - Report an error that has not occurred.
> - Die and take the user's code with us (except for any exit hook code).
> - Return a JNI_OK, and allow the user's next action to fail.
> 
> I think that if VM developers concur that the correct action is to leave 
> the VM
> in a nonusable state, but to not throw an error, that this RC gives us a 
> better option
> than exit(0).
> 
> - Adam
> 
> P.S. Apologies for the delay. I was on vacation. :)
> 
>  > Hi Alan, David, and Tom,
>  >>
>  >> First, thanks again for your efforts on this. As a new guy to OpenJDK
>  >> contributions, it means a lot to see so much progress on this so
>  >> quickly. :)
>  >
>  >All I see is discussion :) Progress would be something else entirely.
> 
> True. :)
> 
>  >
>  >>
>  >>  >On 24/08/2017 07:33, David Holmes wrote:
>  >>  >> Hi Adam,
>  >>  >>
>  >>  >> cc'ing hotspot runtime dev as runtime own JNI and the invocation 
> API -
>  >>  >> and some of the problematic code resides in the VM.
>  >>  >Yeah, the hotspot mailing list would be a better place to discuss 
> this
>  >>  >as there are several issues here and several places where HotSpot 
> aborts
>  >>  >the process when initialization fails. It's a long standing issue 
> (going
>  >>  >back 15+ years) that I think is partly because it's not easy to 
> release
>  >>  >all resources and cleanup before CreateJavaVM returns with an error.
>  >>  >
>  >>
>  >> According to the JNI spec, it is not possible (yet) to create a 
> second VM
>  >> in the same thread as the first.
>  >>
>  >> There is also a bug (dup'd against another bug I don't have the 
> access for)
>  >> which states that even a successful VM creation+destruction won't 
> permit
>  >> a second VM to be created.
>  >>
>  >> https://bugs.openjdk.java.net/browse/JDK-4712793
>  >>
>  >> Both of these seem to imply that making a new VM after a failed 
> VM-creation
>  >> (in the same thread) is unsupported behaviour.
>  >>
>  >> So is it important to release all resources and cleanup, given that we
>  >> won't
>  >> be trying to create a new VM in this thread? By "important" I mean 
> "more
>  >> important than exiting with a return code and allowing the user's code
>  >> to finish".
>  >
>  >Okay, so if there is no intention of attempting to reload the jvm again,
>  >I'm unclear what the purpose of the hosting process actually is. To me
>  >it is either a customer launcher - in which case the exit calls are
>  >"harmless" (and atexit handlers could be used if the process has its own
>  >clean up) - or it's something multi-purpose part of which is to launch a
>  >VM. In the latter case given the inability to reload a VM, and assuming
>  >the process does not what it's java launching powers to be removed, then
>  >the only real option is to filter out the problematic arguments and
>  >either ignore them or exec a separate process to handle them.
> 
> My assumption is that the user's code may be doing many things, of which
> the Java work is only a part. I'm trying not to be too specific here, as 
> I don't
> know what the user is trying to do, nor what they want their code to do if
> Java returns an error. I think we should tell the user what has 
> happened, and
> allow them to act on the information.
> 
> Right now the VM developers don't have that option. They don't have a 
> mechanism
> to tell the user that the VM is not in a usable state, but had found no 
> errors. Therefore
> the VM *must* call exit(0) to indicate "pass", but also to prevent the 
> user trying to do
> anything with the unusable VM.
> 
> I would give them that option. If they can return an RC, they should 
> have one available
> that fits this scenario.
> 
> By providing this negative return code both within and without the VM, 
> we can give future
> VM-upgrade projects the option to indicate an unusable VM with no error, 
> removing
> the need for them to call exit(0) when the VM is unusable despite no 
> error occurring.
> 
> Also, in regards to the example option: I agree that this option should 
> really be filtered
> out before we get to the exit(0)-slash-JNI_SILENT_EXIT RC. Perhaps we 
> could abstract
> the "is this a help option" logic into a shared function, and tie that 
> into the unrecognised
> options logic?
> 
>  >
>  >>  >>
>  >>  >> This specific case seems like a bug to me as the logic is 
> assuming it
>  >>  >> is only ever called by a launcher which it is okay to terminate.
>  >>  >> Though to be honest the very existence of the "help" option 
> seems to
>  >>  >> me somewhat misguided in a hosted-VM environment. That said, I see
>  >>  >> unified logging in 9 also added a terminating "help" option <sigh>.
>  >>  >The agent "help" option case is tricky and would likely need an 
> update
>  >>  >to the JVM TI spec and the Agent_OnLoad return value.
>  >>  >
>  >>
>  >> To clarify, the agent "help" option is only an example of this problem.
>  >> There are 19 locations both within and without hotspot that call 
> exit(0)
>  >> directly, plus more places where exit is passed a variable that can be
>  >> 0 (e.g. the aforementioned agent "help", which calls the forceExit 
> function
>  >> with an argument of 0, which calls exit(arg) in turn).
>  >>
>  >> I understand that your comment was intended as an effort to effect a 
> fix
>  >> for this specific instance of the problem. I wanted to make sure we 
> kept
>  >> sight of the wider problem, as ideally we'd come up with an ideal 
> solution
>  >> that could be applied to all cases.
>  >
>  >The fact there are numerous potential process termination points in the
>  >VM and JDK native code, is something we just have to live with. I'm only
>  >considering these kind of "report and terminate" flags to be the problem
>  >cases that should be handled better.
> 
> A fair statement. I posit that simply having this option available could 
> prevent
> the need for further exit(0)'s in the future.
> 
> Though I'm certainly not ruling out an entrepreneurial VM developer fixing
> these issues in the future. I'm simply agreeing that resolving all of these
> issues are outside of this proposal's scope.
> 
>  >
>  >> My thought on this was a unique return code that tells the user's code
>  >> that the VM is not in a usable state, but that no error has 
> occurred. This
>  >> should be a negative code (so the usual x<0 check will prevent the 
> user's
>  >> code from using the VM), but it shouldn't be one of the existing JNI 
> codes;
>  >> all of which seem to indicate either:
>  >>
>  >> a) The VM is fine and can be used (0).
>  >> or
>  >> b) The VM is not fine because an error occurred (-1 to -6).
>  >>
>  >> Ideally we need a c) The VM is not fine, but no error has occurred.
>  >
>  >It's somewhat debatable how to classify the case where you ask the VM to
>  >load and then perform a one-off action that effectively succeeds but
>  >leaves the VM unusable. Again ideally, to me, the VM would never do that
>  >- such actions would occur as part of VM initialization, the VM would be
>  >usable, but the launcher would do the termination because that is how
>  >the flag is specified. But that is non-trivial to untangle.
>  >
>  >David
> 
> Agreed.
> 
>  >
>  >> Or is there another solution to the exit(0) problem? Other than putting
>  >> a copy of the rest of your code on the exit hook, I mean.
>  >>
>  >>  >
>  >>  >>
>  >>  >> Options processed by the VM will be recognized, while options
>  >>  >> processed by the Java launcher will not be. "-version", "-X", 
> "-help"
>  >>  >> and numerous others are launcher options. Pure VM options are -XX
>  >>  >> options, but the VM also processes some -X flags and, as a 
> result of
>  >>  >> jigsaw, now also processes a bunch of module-related flags that are
>  >>  >> simple --foo options.
>  >>  >Right because these options need to passed to CreateJavaVM as they 
> are
>  >>  >used when initializing the VM. Using system properties would just 
> repeat
>  >>  >the issues of past (e.g. java.class.path) and require documenting 
> a slew
>  >>  >of system properties (which is complicated at repeating options).
>  >>  >
>  >>  >-Alan
> 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