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

[David Holmes]

Hi Thomas,

On 29/01/2020 8:35 pm, Thomas Stüfe wrote:
> Hi David and Matthias,
> 
> On Wed 29. Jan 2020 at 00:27, David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     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
>     -----
> 
> 
> Sorry for chiming in sideways, but I am not so sure. I think it makes 
> sense to specify close on exec here.

It may well make sense but it isn't specified behaviour so would require 
an API specification change.

> When the Vm forks via Runtime.exec we take pains to manually close all 
> open file handles in the child process. That means to me that this is 
> the desired behavior. Leaking file descriptors to child processes is not 
> wanted.
> 
> However, we cannot close file descriptors if someone does a naked fork. 
> I’d think this is a design problem we’d like to fix. The chance that 
> someone deliberately relies on nakedly forked children inheriting 
> descriptors opened in the parent vm is smaller than the chance that 
> leaked file descriptors in child’s cause trouble. These errors are also 
> hard to find.
> 
> Btw, maybe that would be better discusses in core libs?

Yes this is a topic for core-libs or nio-dev. I'm certain this has been 
considered at some point.

Cheers,
David

> Cheers, thomas
> 
> 
>      >
>