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

Thu Sep 27 15:02:51 UTC 2018

Hi Thomas,

I think you may be missing my point. We take a signal that will 
terminate the VM, and from the signal handler context start the error 
reporting and as part of that try to fork_and_exec the onError command 
requested by the user. My recollection from reading all this stuff is 
that vfork may not be safe to call from a signal context.

That's why I suggested we only switch this for the OnOutOfMemoryError 
case as it's not (normally?) executed from a signal context.

Cheers,
David

On 27/09/2018 10:33 AM, Thomas Stüfe wrote:
> 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
>>>>>>>
>>>>
>>