From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30980 invoked by alias); 19 Mar 2007 07:03:38 -0000 Received: (qmail 30943 invoked by uid 22791); 19 Mar 2007 07:03:36 -0000 X-Spam-Check-By: sourceware.org Received: from smtp1.dnsmadeeasy.com (HELO smtp1.dnsmadeeasy.com) (205.234.170.134) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 19 Mar 2007 07:03:33 +0000 Received: from smtp1.dnsmadeeasy.com (localhost [127.0.0.1]) by smtp1.dnsmadeeasy.com (Postfix) with ESMTP id 37E2D299DF1 for ; Mon, 19 Mar 2007 07:03:31 +0000 (GMT) X-Authenticated-Name: js.dnsmadeeasy X-Transit-System: In case of SPAM please contact abuse@dnsmadeeasy.com Received: from avtrex.com (unknown [67.116.42.147]) by smtp1.dnsmadeeasy.com (Postfix) with ESMTP for ; Mon, 19 Mar 2007 07:03:31 +0000 (GMT) Received: from [192.168.7.225] ([192.168.7.225]) by avtrex.com with Microsoft SMTPSVC(6.0.3790.1830); Mon, 19 Mar 2007 00:03:30 -0700 Message-ID: <45FE35C1.9060805@avtrex.com> Date: Mon, 19 Mar 2007 07:03:00 -0000 From: David Daney User-Agent: Thunderbird 1.5.0.10 (X11/20070302) MIME-Version: 1.0 To: java-patches@gcc.gnu.org Subject: [Patch] PR 31228, Fix close-on-exec race. Content-Type: multipart/mixed; boundary="------------060602050801070702050906" X-IsSubscribed: yes Mailing-List: contact java-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: java-patches-owner@gcc.gnu.org X-SW-Source: 2007-q1/txt/msg00693.txt.bz2 This is a multi-part message in MIME format. --------------060602050801070702050906 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 1453 This is an ugly problem: If Runtime.exec() forks between when a file descriptor is created and when we can set FD_CLOEXEC on it, the descriptor will leak to the subprocess. The fix is to close all possible file descriptors before doing the exec system call. I don't really like the fix, as it typically adds over 1000 system calls for each execed process. The only bright side is that it is no longer necessary to call _Jv_platform_close_on_exec, so I removed it. I am a little uneasy about that also, but thought what the heck... Tested on x86_64-pc-linux (FC6) with no regressions in make -k check in libjava. OK to commit? I hope someone can think of a better way to fix the race. If only there were some way to set the default value of the FD_CLOEXEC flag... 2007-03-19 David Daney header file. */ #undef HAVE_SYS_IOCTL_H +/* Define to 1 if you have the header file. */ +#undef HAVE_SYS_RESOURCE_H + /* Define to 1 if you have the header file. */ #undef HAVE_SYS_RW_LOCK_H Index: configure =================================================================== --- configure (revision 123033) +++ configure (working copy) @@ -9602,12 +9602,13 @@ else + for ac_func in strerror ioctl select fstat open fsync sleep opendir \ gmtime_r localtime_r readdir_r getpwuid_r getcwd \ access stat lstat mkdir rename rmdir unlink utime chmod readlink \ nl_langinfo setlocale \ inet_pton uname inet_ntoa \ - fork execvp pipe sigaction ftruncate mmap \ + fork execvp getrlimit pipe sigaction ftruncate mmap \ getifaddrs do as_ac_var=`echo "ac_cv_func_$ac_func" | $as_tr_sh` @@ -10063,7 +10064,8 @@ done -for ac_header in execinfo.h unistd.h dlfcn.h + +for ac_header in execinfo.h unistd.h dlfcn.h sys/resource.h do as_ac_Header=`echo "ac_cv_header_$ac_header" | $as_tr_sh` if eval "test \"\${$as_ac_Header+set}\" = set"; then Index: gnu/java/nio/channels/natFileChannelPosix.cc =================================================================== --- gnu/java/nio/channels/natFileChannelPosix.cc (revision 123033) +++ gnu/java/nio/channels/natFileChannelPosix.cc (working copy) @@ -178,8 +178,6 @@ FileChannelImpl::open (jstring path, jin throw new ::java::io::FileNotFoundException (msg->toString ()); } - _Jv_platform_close_on_exec (fd); - return fd; } Index: gnu/java/net/natPlainSocketImplPosix.cc =================================================================== --- gnu/java/net/natPlainSocketImplPosix.cc (revision 123033) +++ gnu/java/net/natPlainSocketImplPosix.cc (working copy) @@ -72,8 +72,6 @@ gnu::java::net::PlainSocketImpl::create throw new ::java::io::IOException (JvNewStringUTF (strerr)); } - _Jv_platform_close_on_exec (sock); - // We use native_fd in place of fd here. From leaving fd null we avoid // the double close problem in FileDescriptor.finalize. native_fd = sock; @@ -285,8 +283,6 @@ gnu::java::net::PlainSocketImpl::accept if (new_socket < 0) goto error; - _Jv_platform_close_on_exec (new_socket); - jbyteArray raddr; jint rport; if (u.address.sin_family == AF_INET) Index: gnu/java/net/natPlainDatagramSocketImplPosix.cc =================================================================== --- gnu/java/net/natPlainDatagramSocketImplPosix.cc (revision 123033) +++ gnu/java/net/natPlainDatagramSocketImplPosix.cc (working copy) @@ -83,8 +83,6 @@ gnu::java::net::PlainDatagramSocketImpl: throw new ::java::net::SocketException (JvNewStringUTF (strerr)); } - _Jv_platform_close_on_exec (sock); - // We use native_fd in place of fd here. From leaving fd null we avoid // the double close problem in FileDescriptor.finalize. native_fd = sock; Index: java/lang/natPosixProcess.cc =================================================================== --- java/lang/natPosixProcess.cc (revision 123033) +++ java/lang/natPosixProcess.cc (working copy) @@ -17,6 +17,9 @@ details. */ #include #include #include +#ifdef HAVE_SYS_RESOURCE_H +#include +#endif #include #include #include @@ -352,7 +355,31 @@ java::lang::PosixProcess::nativeSpawn () _exit (127); } } - + // Make sure all file descriptors are closed. In + // multi-threaded programs, there is a race between when a + // descriptor is obtained, when we can set FD_CLOEXEC, and + // fork(). If the fork occurs before FD_CLOEXEC is set, the + // descriptor would leak to the execed process if we did not + // manually close it. So that is what we do. Since we + // close all the descriptors, it is redundant to set + // FD_CLOEXEC on them elsewhere. + int max_fd; +#ifdef HAVE_GETRLIMIT + rlimit rl; + int rv = getrlimit(RLIMIT_NOFILE, &rl); + if (rv == 0) + max_fd = rl.rlim_cur - 1; + else + max_fd = 1024 - 1; +#else + max_fd = 1024 - 1; +#endif + while(max_fd > 2) + { + if (max_fd != msgp[1]) + close (max_fd); + max_fd--; + } // Make sure that SIGCHLD is unblocked for the new process. sigset_t mask; sigemptyset (&mask); @@ -438,12 +465,4 @@ java::lang::PosixProcess::nativeSpawn () myclose (msgp[0]); cleanup (args, env, path); - - if (exception == NULL) - { - fcntl (outp[1], F_SETFD, FD_CLOEXEC); - fcntl (inp[0], F_SETFD, FD_CLOEXEC); - if (! redirect) - fcntl (errp[0], F_SETFD, FD_CLOEXEC); - } } --------------060602050801070702050906--