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

[Baesken, Matthias]

Hi David, thanks for looking into it .

While checking other  open(64) calls, I found  the fileOpen / handleOpen   calls  in java.base .
Looks like  they  miss  so far  setting  FD_CLOEXEC /   O_CLOEXEC  , it isn’t done in the callers of those functions  and it is also not (silently) added in the fileOpen/handleOpen function itself  (like HS does in os::open ).
See :

73FD
74handleOpen(const char *path, int oflag, int mode) {
75    FD fd;
76    RESTARTABLE(open64(path, oflag, mode), fd);
77    if (fd != -1) {
78        struct stat64 buf64;
79        int result;
80        RESTARTABLE(fstat64(fd, &buf64), result);
81        if (result != -1) {
82            if (S_ISDIR(buf64.st_mode)) {
83                close(fd);
84                errno = EISDIR;
85                fd = -1;
86            }
87        } else {
88            close(fd);
89            fd = -1;
90        }
91    }
92    return fd;
93}

95void
96fileOpen(JNIEnv *env, jobject this, jstring path, jfieldID fid, int flags)
97{
.....
107        fd = handleOpen(ps, flags, 0666);
....
122}

56JNIEXPORT void JNICALL
57Java_java_io_FileOutputStream_open0(JNIEnv *env, jobject this,
58                                    jstring path, jboolean append) {
59    fileOpen(env, this, path, fos_fd,
60             O_WRONLY | O_CREAT | (append ? O_APPEND : O_TRUNC));
61}

59JNIEXPORT void JNICALL
60Java_java_io_FileInputStream_open0(JNIEnv *env, jobject this, jstring path) {
61    fileOpen(env, this, path, fis_fd, O_RDONLY);
62}


Is this something that should be changed too ? 

Best regards, Matthias


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