RFR (S): 8237775: Core file generation will silently fail if /cores directory has wrong permissions set

[Yasumasa Suenaga]

Hi Gerard,

On 2020/05/23 1:13, gerard ziemski wrote:
> Thank you David, Yasumasa for the review and your suggestions.
> 
> On 5/18/20 11:40 PM, Yasumasa Suenaga wrote:
>> On 2020/05/19 10:10, David Holmes wrote:
>>> Hi Gerard,
>>>
>>> On 19/05/2020 10:40 am, Yasumasa Suenaga wrote:
>>>> Hi Gerard,
>>>>
>>>> os_posix.cpp:
>>>>
>>>>    I think we should not create any file in have_file_write_permission() because it might remain without being removed.
>>>>    Can you check the permission of parent directory?
>>>
>>> I agree with Yasumasa that we should not test permissions by creating a file in a directory that has nothing directly to do with the VM. Even if we ensure it is removed, that's just not the right way to check for permission.
>>>
>>> But I also think it is not necessarily a valid check anyway. A core file is not created by the process like a normal file, and so the process itself need not have write permission to the core file location for the core file to be successfully created by the "system".
>>
>> Core dump was failed when I set "/core" to /proc/sys/kernel/core_pattern on Fedora 32 (Linux Kernel 5.6.11).
>>
>> I looked at a glance, core dump in Linux seems to be written as user granted (do_coredump() in Linux kernel), so it might be failed when core file would be written to the directory which is not permitted to process user.
>>
>>
>>> I really think this is being over engineered. The core dump process is outside of the JVM and the more we try and second guess how it might work and how it might fail the bigger the problem we create for ourselves. I think all we should be doing (if we really think we need do something) is give a general message that states:
>>> - a core dump has been requested
>>> - the location we think the core dump may be if actually created
>>> - reasons the request may be denied: rlimit, missing directory, no permissions
>>
>> I agree with David.
> 
> I would prefer to see a more sophisticated approach here, but I'd rather see an extra note here, than not doing anything at all.
> 
> I added a note to check the existence of the core files folder and the write permission as suggested. I left rlimit check and related messages as is.
> 
> This particular case currently only affects macOS, but I think we should leave it in for other platforms as well, in case they follow the security minded approach of macOS in the future.
> 
> 
> With this change instead of:
> 
> # Core dump will be written. Default location: /cores/core.1234
> 
> we will see:
> 
> # Core dump will be requested. Make sure that the core files folder exists and that it has write permissions. Default location: /cores/core.1234

In Linux, if coredump would be proceeded in other process (core_pattern starts with '|'), this new note is odd to me.
(Coredump might not be written to specified directory.)

You can pass the result of the check to VMError::record_coredump_status(). I guess you can do you want if you pass the result of permission check to 2nd argument of record_coredump_status().
(I think it is not over engineering.)


Thanks,

Yasumasa


> bug: https://bugs.openjdk.java.net/browse/JDK-8237775
> webrev: http://cr.openjdk.java.net/~gziemski/8237775_rev2
> testing: passes Mach5 hs-tier1,2,3,4,5
> 
> 
> cheers
> 
> 
>