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

[gerard ziemski]

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


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