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

[Thomas Stüfe]

Sorry, and thanks for your patience!

..Thomas

On Wed, May 27, 2020 at 5:57 PM gerard ziemski <gerard.ziemski at oracle.com>
wrote:

> Thank you all for the feedback.
>
> I am going to withdraw the issue for now. I'm hearing different
> requirements and I think it's possible to meet them all, but I need to
> consider whether it's worth the effort.
>
>
> cheers
>
> On 5/24/20 1:58 PM, Thomas Stüfe wrote:
>
> Hi Gerard,
>
> I would really prefer we keep the first output line as it is.
>
> "Core dump will be written. Default location: ..."
>
> unless there is a really pressing need to change it. This output has been
> there for ages and gets used by us and customers both in scripts to grep
> for core file locations.
>
> Also, your proposal would make the line very long. I'd prefer if we try
> any output in the 80 char range. Note that the string following "Default
> location: " may not be a path but, as Yasumasa pointed out, a very lengthy
> description of where to find the core when apport is enabled on certain
> Linux distros.
>
> --
>
> One important fact to keep in mind is that this runs when the VM is in a
> fragile state. The less code we execute the better. We want to do the bare
> minimum needed to get a core file and optionally an hs-err file. I recently
> had several customer issues in a row where I had to advice the customer to
> switch writing the error file completely off with
> -XX:+SuppressFatalErrorMessage because we execute just too much code when
> writing those hs-err files and it completely obfuscated the issue in the
> core file or even prevented writing them.
>
> Especially no unnecessary File IO. File IO may hang, and we should avoid
> time outs in error reporting if at all possible.
>
> Thanks, Thomas
>
>
> On Fri, May 22, 2020 at 6:15 PM gerard ziemski <gerard.ziemski at oracle.com>
> 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
>>
>>
>> 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
>>
>>
>>
>>
>