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

[gerard ziemski]

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