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

[David Holmes]

Hi Matthias,

On 28/01/2020 11:44 pm, Baesken, Matthias wrote:
> 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 ?

I think not. Unless those public API's are specified to open a file in a 
specific mode, like close-on-exec, then they should not do that by default.

We should only be doing close-on-exec on internally used files.

David
-----

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