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

[Thomas Stüfe]

Hi David and Matthias,

On Wed 29. Jan 2020 at 00:27, David Holmes <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.

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?

Cheers, thomas


> >
>