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

Thu Sep 27 15:31:21 UTC 2018

Hi David,

Ah, I understand you now.

It is certainly UB, but I have the strong feeling it would just work.
Strictly speaking it is as much UB as calling fork() and a number of
other things we are doing in a signal handling context.

However, a feeling is no knowledge, and I have no time to back my
claims up with actual tests. So I support your suggestion; it makes
sense and is certainly way better than to duplicate all the code.

Thanks, Thomas

On Thu, Sep 27, 2018 at 5:02 PM, David Holmes <david.holmes at oracle.com> wrote:
> 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
>>>>>>>>
>>>>>
>>>
>