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

[Yasumasa Suenaga]

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.


Thanks,

Yasumasa


> Thanks,
> David
> -----
> 
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2020/05/19 5:06, gerard ziemski wrote:
>>> hi all,
>>>
>>> Please review this enhancement, which adds explanation to the user when we can not dump core file (i.e. "The core files directory does not exist or it has no write permissions.") because either the core folder does not exist or the process has no write permission in that folder (i.e. default behavior on recent macOS). We already warn the users when "ulimit" is not set, so we are just trying to be consistent and cover all cases that can cause the core dump not to happen.
>>>
>>> Even though we are adding more code to the signal handler here, it shouldn't affect it negatively. I couldn't measure (using Xcode profiler) any memory impact of the new code (i.e. fopen/close/remove APIs) Moreover we already use "fopen()" and "fclose()" when writing the hs_err log, so the only new API used is "remove()".
>>>
>>> We could conceivably move this code to the startup and cache the answer to be later used in signal handler (it does impact the the startup by -0.6% for "hello world" test case), but I don't like the idea of adding more code to the startup, when it's not going to be used majority of the time.
>>>
>>> Please notice that I had to move the "Default location:" part of the message around, so that it gets printed after the new "The core files directory does not exist or it has no write permissions." message. That's because some tests expect the core location immediately after "Default location:".
>>>
>>>
>>> Before this change a usual message would look like this:
>>>
>>> # No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
>>>
>>> or
>>>
>>> # Core dump will be written. Default location: /cores/core.8201
>>>
>>>
>>> Now we can print:
>>>
>>> # No core dump will be written. The core files directory does not exist or it has no write permissions. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
>>>
>>> or
>>>
>>> # Core dump will be requested from the system. The core files directory does not exist or it has no write permissions. Default location: /cores/core.8171
>>>
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8237775
>>> webrev: http://cr.openjdk.java.net/~gziemski/8237775_rev1
>>> testing: passes Mach5 hs-tier1,2,3,4,5
>>>
>>>
>>> cheers