fopen vs. os::fopen and automatic closing of the file on exec

[Leo Korinth]

On 28/01/2020 14:06, David Holmes wrote:
> Hi Matthias,
> 
> I don't have any info of most below but one follow up ....
> 
> On 28/01/2020 10:51 pm, Baesken, Matthias wrote:
>> Hello, I noticed   while looking at   
>> https://bugs.openjdk.java.net/browse/JDK-8237830   ( support O_CLOEXEC 
>> in os::open on other OS than Linux )
>> that os::fopen  also  has  some support  for setting  FD_CLOEXEC / 
>> O_CLOEXEC  on the file opened .
>> See :
>>
>> 1253// This function is a proxy to fopen, it tries to add a non 
>> standard flag ('e' or 'N')
>> 1254// that ensures automatic closing of the file on exec. If it can 
>> not find support in
>> 1255// the underlying c library, it will make an extra system call 
>> (fcntl) to ensure automatic
>> 1256// closing of the file on exec.
>> 1257FILE* os::fopen(const char* path, const char* mode) {
>> 1258  char modified_mode[20];
>> 1259  assert(strlen(mode) + 1 < sizeof(modified_mode), "mode chars 
>> plus one extra must fit in buffer");
>> 1260  sprintf(modified_mode, "%s" LINUX_ONLY("e") BSD_ONLY("e") 
>> WINDOWS_ONLY("N"), mode);
>> 1261  FILE* file = ::fopen(path, modified_mode);
>> 1262
>> 1263#if !(defined LINUX || defined BSD || defined _WINDOWS)
>> 1264  // assume fcntl FD_CLOEXEC support as a backup solution when 'e' 
>> or 'N'
>> 1265  // is not supported as mode in fopen
>> 1266  if (file != NULL) {
>> 1267    int fd = fileno(file);
>> 1268    if (fd != -1) {
>> 1269      int fd_flags = fcntl(fd, F_GETFD);
>> 1270      if (fd_flags != -1) {
>> 1271        fcntl(fd, F_SETFD, fd_flags | FD_CLOEXEC);
>> 1272      }
>> 1273    }
>> 1274  }
>> 1275#endif
>>
>> However some questions arise here :
>>
>>    1.  Usage :        os::fopen   is  only used sometimes  in HS code 
>> ,  should most of the calls to  fopen be adjusted to os::fopen   (see 
>> list  below )
>>    2.  ::fopen vs. ::fcntl     :    is  os_linux  os::open   we try 
>> to  set  the  "closing of the file on exec"   flag  when calling 
>> ::open  but we later check  that it really  worked so we seem not to 
>> trust it fully ;
> 
> The check is for running on older Linuxes that do not support O_CLOEXEC 
> - where the flag is ignored. That is why I asked about what happens on 
> BSD/macOS and AIX in that situation.
> 
>> Should this be done here too  for Linux ? Or is that  checking in 
>> os_linux os::open  these days not needed any more ?
> 
> It's possible the most recent version of Linux without O_CLOEXEC 
> supported is no longer supported by OpenJDK, in which case we can remove 
> it. But I'm not sure what that version is. I have no idea if fopen with 
> "e" support has the same history as ::open and O_CLOEXEC.

"e" is supported since glibc 2.7, released in 2007. Any support of libc 
versions older than 2.7 today would suprise me.

Something that is not obvious is that on unix-like operating systems, 
ProcessImpl_md.c tries to close (most) open files between fork and exec. 
That is not the case for Windows (I opened 
https://bugs.openjdk.java.net/browse/JDK-8202720 for this). Thus (if I 
understand correctly) the impact on unix-like operating systems will be 
less for adding this support than it is for Windows. os::fopen was 
created to solve a specific bug on windows (logging), and was renamed to 
the more generic os::fopen during review.

I guess most uses of ::fopen /should/ use the more restricted os::fopen, 
but the gain would probably be small.

Thanks,
Leo
> David
> 
>> Best regards, Matthias
>>
>>
>>
>> Grep showed  me these fopen-calls in HS, a lot go not to os::fopen ?
>>
>> cpu/aarch64/vm_version_aarch64.cpp:171:  if (FILE *f = 
>> fopen("/proc/cpuinfo", "r")) {
>> cpu/ppc/vm_version_ppc.cpp:412:  FILE* fp = fopen(info_file, "r");
>> os/aix/os_aix.cpp:3756:  // - might cause an fopen in the subprocess 
>> to fail on a system
>> os/aix/os_perf_aix.cpp:243:  if ((f = fopen(procfile, "r")) == NULL) {
>> os/aix/os_perf_aix.cpp:666:  if ((fp = fopen(buffer, "r")) != NULL) {
>> os/aix/os_perf_aix.cpp:694:  if ((fp = fopen(buffer, "r")) != NULL) {
>> os/bsd/os_bsd.cpp:3479:  // - might cause an fopen in the subprocess 
>> to fail on a system
>> os/linux/decoder_linux.cpp:61:  FILE* file = fopen(filepath, "r");
>> os/linux/os_linux.cpp:257:  if ((fh = fopen("/proc/stat", "r")) == 
>> NULL) {
>> os/linux/os_linux.cpp:364:    FILE *fp = fopen(fname, "r");
>> os/linux/os_linux.cpp:1106:  FILE *fp = fopen("/proc/self/maps", "r");
>> os/linux/os_linux.cpp:1218:    fp = fopen("/proc/self/stat", "r");
>> os/linux/os_linux.cpp:2075:  if ((procmapsFile = 
>> fopen("/proc/self/maps", "r")) != NULL) {
>> os/linux/os_linux.cpp:2244:  FILE* fp = fopen(file, "r");
>> os/linux/os_linux.cpp:2458:  FILE *fp = fopen("/proc/cpuinfo", "r");
>> os/linux/os_linux.cpp:2515:  FILE* fp = fopen("/proc/cpuinfo", "r");
>> os/linux/os_linux.cpp:3645:    FILE *fp = fopen("/proc/self/maps", "r");
>> os/linux/os_linux.cpp:3689:  if ((f = 
>> fopen("/proc/self/coredump_filter", "r+")) == NULL) {
>> os/linux/os_linux.cpp:3741:  FILE *fp = fopen("/proc/meminfo", "r");
>> os/linux/os_linux.cpp:5572:  // - might cause an fopen in the 
>> subprocess to fail on a system
>> os/linux/os_linux.cpp:5797:  fp = fopen(proc_name, "r");
>> os/linux/os_perf_linux.cpp:238:  if ((f = fopen(procfile, "r")) == 
>> NULL) {
>> os/linux/os_perf_linux.cpp:275:  if ((f = fopen("/proc/stat", "r")) == 
>> NULL) {
>> os/linux/os_perf_linux.cpp:726:  if ((fp = fopen(buffer, "r")) != NULL) {
>> os/linux/os_perf_linux.cpp:754:  if ((fp = fopen(buffer, "r")) != NULL) {
>> os/linux/perfMemory_linux.cpp:659:  FILE *fp = fopen(fname, "r");
>> os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp:312:  FILE* const file 
>> = fopen(filename, "r");
>> os/linux/gc/z/zMountPoint_linux.cpp:72:  FILE* fd = 
>> fopen(PROC_SELF_MOUNTINFO, "r");
>> os/linux/cgroupSubsystem_linux.cpp:66:  cgroups = 
>> fopen("/proc/cgroups", "r");
>> os/linux/cgroupSubsystem_linux.cpp:121:  cgroup = 
>> fopen("/proc/self/cgroup", "r");
>> os/linux/cgroupSubsystem_linux.cpp:170:    mntinfo = 
>> fopen("/proc/self/mountinfo", "r");
>> os/linux/cgroupSubsystem_linux.cpp:224:  mntinfo = 
>> fopen("/proc/self/mountinfo", "r");
>> os/linux/cgroupSubsystem_linux.hpp:96:  fp = fopen(file, "r");
>> os/solaris/os_perf_solaris.cpp:521:      if ((fp = fopen(buffer, "r")) 
>> != NULL) {
>> os/solaris/os_perf_solaris.cpp:557:  if ((fp = fopen(psinfo_path, 
>> "r")) == NULL) {
>> os/solaris/os_solaris.cpp:1605:  FILE* fp = fopen("/etc/release", "r");
>> os/solaris/os_solaris.cpp:4091:  //   fopen must be less than 256, 
>> _even_ when the first limit above
>> os/solaris/os_solaris.cpp:4092:  //   has been raised.  This can cause 
>> calls to fopen (but not calls to
>> os/solaris/os_solaris.cpp:4094:  //   native code (although the JDK 
>> itself uses fopen).  One can hardly
>> os/solaris/os_solaris.cpp:4103:  //   stdio fopen limit by calling 
>> function enable_extended_FILE_stdio.
>> os/solaris/os_solaris.cpp:4138:  // - might cause an fopen in the 
>> subprocess to fail on a system
>> os_cpu/linux_sparc/vm_version_linux_sparc.cpp:39:    FILE* fp = 
>> fopen("/proc/cpuinfo", "r");
>> share/logging/logFileOutput.cpp:272:  _stream = os::fopen(_file_name, 
>> FileOpenMode);
>> share/logging/logFileOutput.cpp:361:  _stream = os::fopen(_file_name, 
>> FileOpenMode);
>> share/runtime/arguments.cpp:1345:  FILE* stream = fopen(file_name, "rb");
>> share/runtime/memprofiler.cpp:76:    _log_fp = fopen(log_name , "w+");
>> share/runtime/os.cpp:1253:// This function is a proxy to fopen, it 
>> tries to add a non standard flag ('e' or 'N')
>> share/runtime/os.cpp:1257:FILE* os::fopen(const char* path, const 
>> char* mode) {
>> share/runtime/os.cpp:1261:  FILE* file = ::fopen(path, modified_mode);
>> share/runtime/os.cpp:1265:  // is not supported as mode in fopen
>> share/runtime/os.hpp:510:  static FILE* fopen(const char* path, const 
>> char* mode);
>> share/runtime/abstract_vm_version.cpp:308:  FILE* fp = fopen(filename, 
>> "r");
>> share/utilities/elfFile.cpp:171:  _file = fopen(filepath, "r");
>> share/utilities/ostream.cpp:513:  _file = fopen(file_name, "w");
>> share/utilities/ostream.cpp:523:  _file = fopen(file_name, opentype);
>> share/adlc/main.cpp:363:      (ADF._fp = fopen(ADF._name, action)) == 
>> NULL) {
>> share/c1/c1_Compilation.cpp:697:  fileStream 
>> stream(fopen("c1_compile_only", "wt"));
>> share/c1/c1_Compilation.cpp:713:  fileStream 
>> stream(fopen(".hotspot_compiler", "at"));
>> share/ci/ciReplay.cpp:127:    _stream = fopen(filename, "rt");
>> share/classfile/classListParser.cpp:51:  // Use os::open() because 
>> neither fopen() nor os::fopen()
>> share/compiler/compileBroker.cpp:1891:      fp = fopen(file_name, "wt");
>> share/compiler/compilerOracle.cpp:703:  FILE* stream = 
>> fopen(cc_file(), "rt");
>> share/compiler/disassembler.cpp:272:        if ((fp = fopen(file, 
>> "r")) == NULL) {
>>