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

[Thomas Stüfe]

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
>
>
>
>