RFR 8027434: "-XX:OnOutOfMemoryError" uses fork instead of vfork

[Thomas Stüfe]

On Thu, Sep 27, 2018 at 4:15 PM, David Holmes <david.holmes at oracle.com> wrote:
> Hi Thomas,
>
> On 27/09/2018 9:35 AM, Thomas Stüfe wrote:
>>
>> Hi,
>>
>> I think there is no reason at all to not always to use vfork() here
>> (in the linux implementation for fork_and_exec()).
>>
>> The concerns with vfork() I outlined in
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055333.html
>> do not apply here. This is a very simple case, no stdio redirection,
>> no environment change, nothing. So, I would just exchange fork() for
>> vfork().
>
>
> What about signal-safety if called from the signal handler in the onError
> case?
>
> Thanks,
> David

I do not see a large issue.

The code section in question is this:

5784   } else if (pid == 0) {
5785     // child process
5786
5787     execve("/bin/sh", (char* const*)argv, environ);
5788
5789     // execve failed
5790     _exit(-1);

So we call execve, failing that, immediately _exit. This is as good as it gets.

The risk we run is that any signal hitting the child in the time it
takes to execute this tiny section will affect and maybe kill the
parent. But:

- the chance is astronomically small
- we live with a much larger risk in Runtime.exec() - as I explained
in my mail to core-libs - and even there I do not know of any real
world issues (still worth fixing though).
- In contrast to Runtime.exec, if the parent dies here the
consequences are not that dire. We are already dying, after all.

I would think that this is one of the few places where vfork may be
actually fine. Its tiny risk offset by the advantage of being able to
call tools from a VM with a large memory footprint.

(Or am I missing your point? I may be missing your point... :)

..Thomas

>
>
>> Best Regards, Thomas
>>
>> On Thu, Sep 27, 2018 at 3:22 PM, David Holmes <david.holmes at oracle.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 27/09/2018 8:18 AM, Muthusamy Chinnathambi wrote:
>>>>
>>>>
>>>> Hi David,
>>>>
>>>>> That doesn't work.
>>>>
>>>>
>>>> I get your point. My bad.
>>>>
>>>>> Only the caller knows why it is being called.
>>>>
>>>>
>>>> Please find the updated webrev at
>>>> http://cr.openjdk.java.net/~mchinnathamb/8027434/webrev.02/
>>>> This now uses a static helper to decide.
>>>
>>>
>>>
>>> Sorry but I really don't like this version using is_forkmode_vfork. It's
>>> awkward to introduce a Linux specific feature into a shared API. But I
>>> think
>>> the best way to do this would be to introduce a new default parameter to
>>> os::fork_and_exec
>>>
>>> static int fork_and_exec(char *cmd, bool use_vfork_if_available = false);
>>>
>>> then the impl in o_linux.cpp which use the arg to select fork or vfork.
>>> And
>>> vmError will do:
>>>
>>> if (os::fork_and_exec(cmd, true) < 0) {
>>>
>>> Thanks,
>>> David
>>>
>>>
>>>> Regards,
>>>> Muthusamy C
>>>>
>>>> -----Original Message-----
>>>> From: David Holmes
>>>> Sent: Friday, September 21, 2018 7:27 PM
>>>> To: Muthusamy Chinnathambi <muthusamy.chinnathambi at oracle.com>;
>>>> hotspot-runtime-dev at openjdk.java.net
>>>> Subject: Re: RFR 8027434: "-XX:OnOutOfMemoryError" uses fork instead of
>>>> vfork
>>>>
>>>> Sorry for the delay.
>>>>
>>>> On 19/09/2018 7:48 AM, Muthusamy Chinnathambi wrote:
>>>>>
>>>>>
>>>>> Hi David,
>>>>>
>>>>> Please find the updated webrev at
>>>>> http://cr.openjdk.java.net/~mchinnathamb/8027434/webrev.01/
>>>>> Instead of creating a static helper, I have moved the decision to the
>>>>> fork_and_exec routine itself.
>>>>
>>>>
>>>>
>>>> That doesn't work. Just because we have an OnOutOfMemoryError handler
>>>> set it doesn't mean the call to os::fork_and_exec was made to run that
>>>> handler. Only the caller knows why it is being called.
>>>>
>>>> David
>>>>
>>>>> Tested with mach5 hs-tier1-4.
>>>>>
>>>>> Regards,
>>>>> Muthusamy C
>>>>>
>>>>> -----Original Message-----
>>>>> From: Muthusamy Chinnathambi
>>>>> Sent: Friday, September 14, 2018 12:03 PM
>>>>> To: David Holmes <david.holmes at oracle.com>;
>>>>> hotspot-runtime-dev at openjdk.java.net
>>>>> Subject: RE: RFR 8027434: "-XX:OnOutOfMemoryError" uses fork instead of
>>>>> vfork
>>>>>
>>>>> Hi David,
>>>>>
>>>>>> please refactor into a static helper function
>>>>>
>>>>>
>>>>> Sure, I will change and get back after testing.
>>>>>
>>>>> Regards,
>>>>> Muthusamy C
>>>>>
>>>>> -----Original Message-----
>>>>> From: David Holmes
>>>>> Sent: Friday, September 14, 2018 11:16 AM
>>>>> To: Muthusamy Chinnathambi <muthusamy.chinnathambi at oracle.com>;
>>>>> hotspot-runtime-dev at openjdk.java.net
>>>>> Subject: Re: RFR 8027434: "-XX:OnOutOfMemoryError" uses fork instead of
>>>>> vfork
>>>>>
>>>>> Hi Muthusamy,
>>>>>
>>>>> Instead of duplicating all the code - which I think only differs in the
>>>>> fork versus vfork line - please refactor into a static helper function
>>>>> that takes a boolean vfork and call that from
>>>>> fork_and_exec/vfork_and_exec selecting fork or vfork as appropriate.
>>>>>
>>>>> I'm still mulling over whether there is a cleaner way to do this other
>>>>> than introducing a linux only entry to the os class.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>
>>>>> On 14/09/2018 1:55 PM, Muthusamy Chinnathambi wrote:
>>>>>>
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> Could you review this fix for JDK-8027434 - to use vfork instead of
>>>>>> fork
>>>>>> for -XX:OnOutOfMemoryError.
>>>>>>
>>>>>> See bug comments for more details.
>>>>>>
>>>>>> Bug URL: https://bugs.openjdk.java.net/browse/JDK-8027434
>>>>>> Webrev URL:
>>>>>> http://cr.openjdk.java.net/~mchinnathamb/8027434/webrev.00/
>>>>>>
>>>>>> Tested with mach5 hs-tier1-4.
>>>>>>
>>>>>> Regards,
>>>>>> Muthusamy C
>>>>>>
>>>
>