* [Patch] PR 31228, Fix close-on-exec race. @ 2007-03-19 7:03 David Daney 2007-03-20 21:25 ` Tom Tromey 0 siblings, 1 reply; 20+ messages in thread From: David Daney @ 2007-03-19 7:03 UTC (permalink / raw) To: java-patches [-- Attachment #1: Type: text/plain, Size: 1453 bytes --] 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 <ddaney@avtrex.com) PR libgcj/31228 * configure.ac: Add checks for getrlimit and sys/resource.h. * include/posix.h (_Jv_platform_close_on_exec): Remove. * include/config.h.in: Regenerate. * configure: Regenerate. * gnu/java/nio/channels/natFileChannelPosix.cc (open): Remove call to _Jv_platform_close_on_exec; * gnu/java/net/natPlainSocketImplPosix.cc (create): Likewise. (accept): Likewise. * gnu/java/net/natPlainDatagramSocketImplPosix.cc (create):Likewise. * java/lang/natPosixProcess.cc: Include sys/resource.h. (nativeSpawn): Close all file descriptors. Don't set FD_CLOEXEC on pipes. [-- Attachment #2: closeall.diff.txt --] [-- Type: text/plain, Size: 6749 bytes --] Index: configure.ac =================================================================== --- configure.ac (revision 123033) +++ configure.ac (working copy) @@ -1006,10 +1006,10 @@ else 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]) AC_CHECK_FUNCS(inet_aton inet_addr, break) - AC_CHECK_HEADERS(execinfo.h unistd.h dlfcn.h) + AC_CHECK_HEADERS(execinfo.h unistd.h dlfcn.h sys/resource.h) # Do an additional check on dld, HP-UX for example has dladdr in libdld.sl AC_CHECK_LIB(dl, dladdr, [ AC_DEFINE(HAVE_DLADDR, 1, [Define if you have dladdr()])], [ Index: include/posix.h =================================================================== --- include/posix.h (revision 123033) +++ include/posix.h (working copy) @@ -98,15 +98,6 @@ extern jlong _Jv_platform_nanotime (); extern void _Jv_platform_initialize (void); extern void _Jv_platform_initProperties (java::util::Properties*); -inline void -_Jv_platform_close_on_exec (jint fd) -{ - // Ignore errors. - ::fcntl (fd, F_SETFD, FD_CLOEXEC); -} - -#undef fcntl - #ifdef JV_HASH_SYNCHRONIZATION #ifndef HAVE_USLEEP_DECL extern "C" int usleep (useconds_t useconds); Index: include/config.h.in =================================================================== --- include/config.h.in (revision 123033) +++ include/config.h.in (working copy) @@ -127,6 +127,9 @@ /* Define to 1 if you have the `getpwuid_r' function. */ #undef HAVE_GETPWUID_R +/* Define to 1 if you have the `getrlimit' function. */ +#undef HAVE_GETRLIMIT + /* Define to 1 if you have the `gettimeofday' function. */ #undef HAVE_GETTIMEOFDAY @@ -316,6 +319,9 @@ /* Define to 1 if you have the <sys/ioctl.h> header file. */ #undef HAVE_SYS_IOCTL_H +/* Define to 1 if you have the <sys/resource.h> header file. */ +#undef HAVE_SYS_RESOURCE_H + /* Define to 1 if you have the <sys/rw_lock.h> 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 <fcntl.h> #include <sys/types.h> #include <sys/wait.h> +#ifdef HAVE_SYS_RESOURCE_H +#include <sys/resource.h> +#endif #include <signal.h> #include <string.h> #include <stdlib.h> @@ -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); - } } ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-19 7:03 [Patch] PR 31228, Fix close-on-exec race David Daney @ 2007-03-20 21:25 ` Tom Tromey 2007-03-20 21:37 ` David Daney 0 siblings, 1 reply; 20+ messages in thread From: Tom Tromey @ 2007-03-20 21:25 UTC (permalink / raw) To: David Daney; +Cc: java-patches >>>>> "David" == David Daney <ddaney@avtrex.com> writes: David> I hope someone can think of a better way to fix the race. If David> only there were some way to set the default value of the David> FD_CLOEXEC flag... We could wrap each "open + set FD_CLOEXEC" in a mutex, and do the same for the fork. Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-20 21:25 ` Tom Tromey @ 2007-03-20 21:37 ` David Daney 2007-03-20 21:57 ` David Daney 0 siblings, 1 reply; 20+ messages in thread From: David Daney @ 2007-03-20 21:37 UTC (permalink / raw) To: tromey; +Cc: java-patches Tom Tromey wrote: >>>>>> "David" == David Daney <ddaney@avtrex.com> writes: > > David> I hope someone can think of a better way to fix the race. If > David> only there were some way to set the default value of the > David> FD_CLOEXEC flag... > > We could wrap each "open + set FD_CLOEXEC" in a mutex, and do the same > for the fork. That does not work in a blocking socket accept situation. It would prevent Runtime.exec() from starting the subprocess until a connection arrived. David Daney ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-20 21:37 ` David Daney @ 2007-03-20 21:57 ` David Daney 2007-03-20 22:12 ` Tom Tromey 0 siblings, 1 reply; 20+ messages in thread From: David Daney @ 2007-03-20 21:57 UTC (permalink / raw) To: David Daney; +Cc: tromey, java-patches David Daney wrote: > Tom Tromey wrote: >>>>>>> "David" == David Daney <ddaney@avtrex.com> writes: >> >> David> I hope someone can think of a better way to fix the race. If >> David> only there were some way to set the default value of the >> David> FD_CLOEXEC flag... >> >> We could wrap each "open + set FD_CLOEXEC" in a mutex, and do the same >> for the fork. > > That does not work in a blocking socket accept situation. It would > prevent Runtime.exec() from starting the subprocess until a connection > arrived. > OK, I will respond to myself: Make the accept non-blocking, and do a select/poll on the ServerSocket. That way the accept would never block and you could use a mutex. This is workable. I think there are only 4 or 5 places file descriptors are obtained in libgcj. This makes me think that ld.so is probably broken also. When we do dlopen ld.so opens the library file and does several mmaps before closing the file descriptor. These could leak out as well. David Daney ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-20 21:57 ` David Daney @ 2007-03-20 22:12 ` Tom Tromey 2007-03-20 22:24 ` David Daney ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Tom Tromey @ 2007-03-20 22:12 UTC (permalink / raw) To: David Daney; +Cc: java-patches >>>>> "David" == David Daney <ddaney@avtrex.com> writes: David> Make the accept non-blocking, and do a select/poll on the David> ServerSocket. That way the accept would never block and you could use David> a mutex. Yeah. I hadn't thought of the accept case, thanks. David> This makes me think that ld.so is probably broken also. When we do David> dlopen ld.so opens the library file and does several mmaps before David> closing the file descriptor. These could leak out as well. Not to mention other places that libc may create fds. On some platforms fdwalk(3) can be used to make this close loop more efficient. There's also closefrom(3). Linux doesn't seem to have either of these, but Solaris does. Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-20 22:12 ` Tom Tromey @ 2007-03-20 22:24 ` David Daney 2007-03-21 10:10 ` Andrew Haley 2007-03-21 17:34 ` David Daney 2 siblings, 0 replies; 20+ messages in thread From: David Daney @ 2007-03-20 22:24 UTC (permalink / raw) To: tromey; +Cc: java-patches Tom Tromey wrote: >>>>>> "David" == David Daney <ddaney@avtrex.com> writes: > > David> Make the accept non-blocking, and do a select/poll on the > David> ServerSocket. That way the accept would never block and you could use > David> a mutex. > > Yeah. I hadn't thought of the accept case, thanks. > > David> This makes me think that ld.so is probably broken also. When we do > David> dlopen ld.so opens the library file and does several mmaps before > David> closing the file descriptor. These could leak out as well. > > Not to mention other places that libc may create fds. > > On some platforms fdwalk(3) can be used to make this close loop more > efficient. There's also closefrom(3). Linux doesn't seem to have > either of these, but Solaris does. > > Tom It would be fairly simple to write a linux version of closefrom(3) by reading /proc/self/fd. I think that may be the best choice. David Daney. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-20 22:12 ` Tom Tromey 2007-03-20 22:24 ` David Daney @ 2007-03-21 10:10 ` Andrew Haley 2007-03-21 17:34 ` David Daney 2 siblings, 0 replies; 20+ messages in thread From: Andrew Haley @ 2007-03-21 10:10 UTC (permalink / raw) To: Tom Tromey; +Cc: David Daney, java-patches Tom Tromey writes: > >>>>> "David" == David Daney <ddaney@avtrex.com> writes: > > David> Make the accept non-blocking, and do a select/poll on the > David> ServerSocket. That way the accept would never block and you could use > David> a mutex. > > Yeah. I hadn't thought of the accept case, thanks. > > David> This makes me think that ld.so is probably broken also. When we do > David> dlopen ld.so opens the library file and does several mmaps before > David> closing the file descriptor. These could leak out as well. > > Not to mention other places that libc may create fds. And native code such as GUI toolkits that we don't control. Maybe they mark their file descriptors FD_CLOEXEC, maybe they don't. > On some platforms fdwalk(3) can be used to make this close loop more > efficient. There's also closefrom(3). Linux doesn't seem to have > either of these, but Solaris does. Closing all file descriptors on exec() is a normal idiom. Only measurement will tell us whether this is a big time hog, and we shouldn't do any optimization until we know it's needed. Experience has shown that we (well me, anyway) are almost always wrong about where the time goes. Andrew. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-20 22:12 ` Tom Tromey 2007-03-20 22:24 ` David Daney 2007-03-21 10:10 ` Andrew Haley @ 2007-03-21 17:34 ` David Daney 2007-03-21 17:39 ` David Daney 2007-03-21 17:54 ` Andrew Haley 2 siblings, 2 replies; 20+ messages in thread From: David Daney @ 2007-03-21 17:34 UTC (permalink / raw) To: java-patches; +Cc: tromey [-- Attachment #1: Type: text/plain, Size: 1776 bytes --] Tom Tromey wrote: >>>>>> "David" == David Daney <ddaney@avtrex.com> writes: >>>>>> > > David> Make the accept non-blocking, and do a select/poll on the > David> ServerSocket. That way the accept would never block and you could use > David> a mutex. > > Yeah. I hadn't thought of the accept case, thanks. > > David> This makes me think that ld.so is probably broken also. When we do > David> dlopen ld.so opens the library file and does several mmaps before > David> closing the file descriptor. These could leak out as well. > > Not to mention other places that libc may create fds. > > On some platforms fdwalk(3) can be used to make this close loop more > efficient. There's also closefrom(3). Linux doesn't seem to have > either of these, but Solaris does. > > Tom > How about this version? I only tested it on x86_64-pc-linux-gnu, so there may be bugs in the parts of the patch that are targeted at platforms that have fdwalk. 2007-03-21 David Daney <ddaney@avtrex.com) PR libgcj/31228 * configure.ac: Add checks for getrlimit, fdwalk, readdir, /proc/self/fd, and sys/resource.h. * include/posix.h (_Jv_platform_close_on_exec): Remove. * include/config.h.in: Regenerate. * configure: Regenerate. * gnu/java/nio/channels/natFileChannelPosix.cc (open): Remove call to _Jv_platform_close_on_exec; * gnu/java/net/natPlainSocketImplPosix.cc (create): Likewise. (accept): Likewise. * gnu/java/net/natPlainDatagramSocketImplPosix.cc (create):Likewise. * java/lang/natPosixProcess.cc: Include dirent.h and sys/resource.h. (closer_function): New function. (close_all_files): New function with two alternate implementations. (nativeSpawn): Call close_all_files. Don't set FD_CLOEXEC on pipes. [-- Attachment #2: closeall.diff.txt --] [-- Type: text/plain, Size: 13937 bytes --] Index: configure.ac =================================================================== --- configure.ac (revision 123033) +++ configure.ac (working copy) @@ -1009,7 +1009,8 @@ else fork execvp pipe sigaction ftruncate mmap \ getifaddrs]) AC_CHECK_FUNCS(inet_aton inet_addr, break) - AC_CHECK_HEADERS(execinfo.h unistd.h dlfcn.h) + AC_CHECK_HEADERS(execinfo.h unistd.h dlfcn.h sys/resource.h) + AC_CHECK_FUNCS(fdwalk readdir getrlimit) # Do an additional check on dld, HP-UX for example has dladdr in libdld.sl AC_CHECK_LIB(dl, dladdr, [ AC_DEFINE(HAVE_DLADDR, 1, [Define if you have dladdr()])], [ @@ -1022,11 +1023,15 @@ else AC_CHECK_FILES(/proc/self/maps, [ AC_DEFINE(HAVE_PROC_SELF_MAPS, 1, [Define if you have /proc/self/maps])]) + AC_CHECK_FILES(/proc/self/fd, [ + AC_DEFINE(HAVE_PROC_SELF_FD, 1, + [Define if you have /proc/self/fd])]) else case $host in *-linux*) AC_DEFINE(HAVE_PROC_SELF_EXE, 1, [Define if you have /proc/self/exe]) AC_DEFINE(HAVE_PROC_SELF_MAPS, 1, [Define if you have /proc/self/maps]) + AC_DEFINE(HAVE_PROC_SELF_FD, 1, [Define if you have /proc/self/fd]) ;; esac fi Index: include/posix.h =================================================================== --- include/posix.h (revision 123033) +++ include/posix.h (working copy) @@ -98,15 +98,6 @@ extern jlong _Jv_platform_nanotime (); extern void _Jv_platform_initialize (void); extern void _Jv_platform_initProperties (java::util::Properties*); -inline void -_Jv_platform_close_on_exec (jint fd) -{ - // Ignore errors. - ::fcntl (fd, F_SETFD, FD_CLOEXEC); -} - -#undef fcntl - #ifdef JV_HASH_SYNCHRONIZATION #ifndef HAVE_USLEEP_DECL extern "C" int usleep (useconds_t useconds); Index: include/config.h.in =================================================================== --- include/config.h.in (revision 123033) +++ include/config.h.in (working copy) @@ -88,6 +88,9 @@ /* Define to 1 if you have the <fcntl.h> header file. */ #undef HAVE_FCNTL_H +/* Define to 1 if you have the `fdwalk' function. */ +#undef HAVE_FDWALK + /* Define to 1 if you have the `fork' function. */ #undef HAVE_FORK @@ -127,6 +130,9 @@ /* Define to 1 if you have the `getpwuid_r' function. */ #undef HAVE_GETPWUID_R +/* Define to 1 if you have the `getrlimit' function. */ +#undef HAVE_GETRLIMIT + /* Define to 1 if you have the `gettimeofday' function. */ #undef HAVE_GETTIMEOFDAY @@ -232,6 +238,9 @@ /* Define if you have /proc/self/exe */ #undef HAVE_PROC_SELF_EXE +/* Define if you have /proc/self/fd */ +#undef HAVE_PROC_SELF_FD + /* Define if you have /proc/self/maps */ #undef HAVE_PROC_SELF_MAPS @@ -247,6 +256,9 @@ /* Define to 1 if you have the <pwd.h> header file. */ #undef HAVE_PWD_H +/* Define to 1 if you have the `readdir' function. */ +#undef HAVE_READDIR + /* Define to 1 if you have the `readdir_r' function. */ #undef HAVE_READDIR_R @@ -316,6 +328,9 @@ /* Define to 1 if you have the <sys/ioctl.h> header file. */ #undef HAVE_SYS_IOCTL_H +/* Define to 1 if you have the <sys/resource.h> header file. */ +#undef HAVE_SYS_RESOURCE_H + /* Define to 1 if you have the <sys/rw_lock.h> header file. */ #undef HAVE_SYS_RW_LOCK_H @@ -379,6 +394,9 @@ #undef HAVE__PROC_SELF_EXE /* Define to 1 if you have the file `AC_File'. */ +#undef HAVE__PROC_SELF_FD + +/* Define to 1 if you have the file `AC_File'. */ #undef HAVE__PROC_SELF_MAPS /* Define as const if the declaration of iconv() needs const. */ Index: configure =================================================================== --- configure (revision 123033) +++ configure (working copy) @@ -10063,7 +10063,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 @@ -10212,6 +10213,115 @@ fi done + + + +for ac_func in fdwalk readdir getrlimit +do +as_ac_var=`echo "ac_cv_func_$ac_func" | $as_tr_sh` +echo "$as_me:$LINENO: checking for $ac_func" >&5 +echo $ECHO_N "checking for $ac_func... $ECHO_C" >&6 +if eval "test \"\${$as_ac_var+set}\" = set"; then + echo $ECHO_N "(cached) $ECHO_C" >&6 +else + if test x$gcc_no_link = xyes; then + { { echo "$as_me:$LINENO: error: Link tests are not allowed after GCC_NO_EXECUTABLES." >&5 +echo "$as_me: error: Link tests are not allowed after GCC_NO_EXECUTABLES." >&2;} + { (exit 1); exit 1; }; } +fi +cat >conftest.$ac_ext <<_ACEOF +/* confdefs.h. */ +_ACEOF +cat confdefs.h >>conftest.$ac_ext +cat >>conftest.$ac_ext <<_ACEOF +/* end confdefs.h. */ +/* Define $ac_func to an innocuous variant, in case <limits.h> declares $ac_func. + For example, HP-UX 11i <limits.h> declares gettimeofday. */ +#define $ac_func innocuous_$ac_func + +/* System header to define __stub macros and hopefully few prototypes, + which can conflict with char $ac_func (); below. + Prefer <limits.h> to <assert.h> if __STDC__ is defined, since + <limits.h> exists even on freestanding compilers. */ + +#ifdef __STDC__ +# include <limits.h> +#else +# include <assert.h> +#endif + +#undef $ac_func + +/* Override any gcc2 internal prototype to avoid an error. */ +#ifdef __cplusplus +extern "C" +{ +#endif +/* We use char because int might match the return type of a gcc2 + builtin and then its argument prototype would still apply. */ +char $ac_func (); +/* The GNU C library defines this for functions which it implements + to always fail with ENOSYS. Some functions are actually named + something starting with __ and the normal name is an alias. */ +#if defined (__stub_$ac_func) || defined (__stub___$ac_func) +choke me +#else +char (*f) () = $ac_func; +#endif +#ifdef __cplusplus +} +#endif + +int +main () +{ +return f != $ac_func; + ; + return 0; +} +_ACEOF +rm -f conftest.$ac_objext conftest$ac_exeext +if { (eval echo "$as_me:$LINENO: \"$ac_link\"") >&5 + (eval $ac_link) 2>conftest.er1 + ac_status=$? + grep -v '^ *+' conftest.er1 >conftest.err + rm -f conftest.er1 + cat conftest.err >&5 + echo "$as_me:$LINENO: \$? = $ac_status" >&5 + (exit $ac_status); } && + { ac_try='test -z "$ac_c_werror_flag" + || test ! -s conftest.err' + { (eval echo "$as_me:$LINENO: \"$ac_try\"") >&5 + (eval $ac_try) 2>&5 + ac_status=$? + echo "$as_me:$LINENO: \$? = $ac_status" >&5 + (exit $ac_status); }; } && + { ac_try='test -s conftest$ac_exeext' + { (eval echo "$as_me:$LINENO: \"$ac_try\"") >&5 + (eval $ac_try) 2>&5 + ac_status=$? + echo "$as_me:$LINENO: \$? = $ac_status" >&5 + (exit $ac_status); }; }; then + eval "$as_ac_var=yes" +else + echo "$as_me: failed program was:" >&5 +sed 's/^/| /' conftest.$ac_ext >&5 + +eval "$as_ac_var=no" +fi +rm -f conftest.err conftest.$ac_objext \ + conftest$ac_exeext conftest.$ac_ext +fi +echo "$as_me:$LINENO: result: `eval echo '${'$as_ac_var'}'`" >&5 +echo "${ECHO_T}`eval echo '${'$as_ac_var'}'`" >&6 +if test `eval echo '${'$as_ac_var'}'` = yes; then + cat >>confdefs.h <<_ACEOF +#define `echo "HAVE_$ac_func" | $as_tr_cpp` 1 +_ACEOF + +fi +done + # Do an additional check on dld, HP-UX for example has dladdr in libdld.sl echo "$as_me:$LINENO: checking for dladdr in -ldl" >&5 echo $ECHO_N "checking for dladdr in -ldl... $ECHO_C" >&6 @@ -10435,6 +10545,37 @@ _ACEOF fi + echo "$as_me:$LINENO: checking for /proc/self/fd" >&5 +echo $ECHO_N "checking for /proc/self/fd... $ECHO_C" >&6 +if test "${ac_cv_file__proc_self_fd+set}" = set; then + echo $ECHO_N "(cached) $ECHO_C" >&6 +else + test "$cross_compiling" = yes && + { { echo "$as_me:$LINENO: error: cannot check for file existence when cross compiling" >&5 +echo "$as_me: error: cannot check for file existence when cross compiling" >&2;} + { (exit 1); exit 1; }; } +if test -r "/proc/self/fd"; then + ac_cv_file__proc_self_fd=yes +else + ac_cv_file__proc_self_fd=no +fi +fi +echo "$as_me:$LINENO: result: $ac_cv_file__proc_self_fd" >&5 +echo "${ECHO_T}$ac_cv_file__proc_self_fd" >&6 +if test $ac_cv_file__proc_self_fd = yes; then + +cat >>confdefs.h <<_ACEOF +#define HAVE__PROC_SELF_FD 1 +_ACEOF + + + +cat >>confdefs.h <<\_ACEOF +#define HAVE_PROC_SELF_FD 1 +_ACEOF + +fi + else case $host in *-linux*) @@ -10448,6 +10589,11 @@ cat >>confdefs.h <<\_ACEOF #define HAVE_PROC_SELF_MAPS 1 _ACEOF + +cat >>confdefs.h <<\_ACEOF +#define HAVE_PROC_SELF_FD 1 +_ACEOF + ;; esac fi 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,12 @@ details. */ #include <fcntl.h> #include <sys/types.h> #include <sys/wait.h> +#ifdef HAVE_SYS_RESOURCE_H +#include <sys/resource.h> +#endif +#ifdef HAVE_DIRENT_H +#include <dirent.h> +#endif #include <signal.h> #include <string.h> #include <stdlib.h> @@ -87,6 +93,93 @@ myclose (int &fd) fd = -1; } +#ifdef HAVE_FDWALK + +static int +closer_function(void *arg, int fd) +{ + int *le = (int *)arg; + if (fd >= le[0] && fd != le[1]) + close (fd); + return 0; +} + + +static void +close_all_files(int lowfd, int exclude) +{ + int le[2]; + le[0] = lowfd; + le[1] = exclude; + + fdwalk (closer_function, le); +} +#else +namespace +{ + struct fd_list_elt + { + fd_list_elt *next; + int fd; + }; +} + +static void +close_all_files(int lowfd, int exclude) +{ +# if defined(HAVE_PROC_SELF_FD) && defined(HAVE_READDIR) \ + && defined(HAVE_OPENDIR) + DIR *dir = opendir ("/proc/self/fd"); + dirent *dep; + fd_list_elt *list_head = NULL; + + while ((dep = readdir (dir)) != NULL) + { + if (dep->d_name[0] != '.') + { + int fd = atoi (dep->d_name); + if (fd >= lowfd && fd != exclude) + { +# ifdef HAVE_ALLOCA + fd_list_elt *elt = (fd_list_elt *)alloca(sizeof(fd_list_elt)); +# else + fd_list_elt *elt = new fd_list_elt; +# endif + elt->next = list_head; + elt->fd = fd; + } + } + } + closedir (dir); + while (list_head != NULL) + { + close (list_head->fd); + list_head = list_head->next; + // Don't bother freeing list_head. We are either going to exec + // or _exit so the memory will get cleaned up. + } +# else // Cannot read /proc/self/fd + int max_fd; +# ifdef(HAVE_GETRLIMIT) + rlimit rl; + int rv = getrlimit(RLIMIT_NOFILE, &rl); + if (rv == 0) + max_fd = rl.rlim_max - 1; + else + max_fd = 1024 - 1; +# else // Cannot determine maximum number of open files. + max_fd = 1024 - 1; +# endif + while (max_fd >= lowfd) + { + if (max_fd != exclude) + close (max_fd); + max_fd--; + } +# endif +} +#endif + // There has to be a signal handler in order to be able to // sigwait() on SIGCHLD. The information passed is ignored as it // will be recovered by the waitpid() call. @@ -352,6 +445,15 @@ 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. + close_all_files (3, msgp[1]); // Make sure that SIGCHLD is unblocked for the new process. sigset_t mask; @@ -438,12 +540,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); - } } ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-21 17:34 ` David Daney @ 2007-03-21 17:39 ` David Daney 2007-03-21 17:54 ` Andrew Haley 1 sibling, 0 replies; 20+ messages in thread From: David Daney @ 2007-03-21 17:39 UTC (permalink / raw) To: java-patches; +Cc: tromey David Daney wrote: > +static void > +close_all_files(int lowfd, int exclude) > +{ > +# if defined(HAVE_PROC_SELF_FD) && defined(HAVE_READDIR) \ > + && defined(HAVE_OPENDIR) > + DIR *dir = opendir ("/proc/self/fd"); > + dirent *dep; > + fd_list_elt *list_head = NULL; > + > + while ((dep = readdir (dir)) != NULL) > + { > + if (dep->d_name[0] != '.') > + { > + int fd = atoi (dep->d_name); > + if (fd >= lowfd && fd != exclude) > + { > +# ifdef HAVE_ALLOCA > + fd_list_elt *elt = (fd_list_elt *)alloca(sizeof(fd_list_elt)); > +# else > + fd_list_elt *elt = new fd_list_elt; > +# endif > + elt->next = list_head; > + elt->fd = fd; > + } > + } > + } > + closedir (dir); > + while (list_head != NULL) > + { > + close (list_head->fd); > + list_head = list_head->next; > + // Don't bother freeing list_head. We are either going to exec > + // or _exit so the memory will get cleaned up. > + } Well this is just a proof of concept. There should probably be some error checking in there too. David Daney ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-21 17:34 ` David Daney 2007-03-21 17:39 ` David Daney @ 2007-03-21 17:54 ` Andrew Haley 2007-03-21 22:31 ` David Daney 1 sibling, 1 reply; 20+ messages in thread From: Andrew Haley @ 2007-03-21 17:54 UTC (permalink / raw) To: David Daney; +Cc: java-patches, tromey David Daney writes: > Tom Tromey wrote: > >>>>>> "David" == David Daney <ddaney@avtrex.com> writes: > >>>>>> > > > > David> Make the accept non-blocking, and do a select/poll on the > > David> ServerSocket. That way the accept would never block and you could use > > David> a mutex. > > > > Yeah. I hadn't thought of the accept case, thanks. > > > > David> This makes me think that ld.so is probably broken also. When we do > > David> dlopen ld.so opens the library file and does several mmaps before > > David> closing the file descriptor. These could leak out as well. > > > > Not to mention other places that libc may create fds. > > > > On some platforms fdwalk(3) can be used to make this close loop more > > efficient. There's also closefrom(3). Linux doesn't seem to have > > either of these, but Solaris does. > > > > Tom > > > How about this version? This is impressively heroic, but I really think you should measure the performance improvement before committing this. :-) Andrew. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-21 17:54 ` Andrew Haley @ 2007-03-21 22:31 ` David Daney 2007-03-22 10:46 ` Andrew Haley 0 siblings, 1 reply; 20+ messages in thread From: David Daney @ 2007-03-21 22:31 UTC (permalink / raw) To: Andrew Haley; +Cc: java-patches, tromey Andrew Haley wrote: > David Daney writes: > > Tom Tromey wrote: > > >>>>>> "David" == David Daney <ddaney@avtrex.com> writes: > > >>>>>> > > > > > > David> Make the accept non-blocking, and do a select/poll on the > > > David> ServerSocket. That way the accept would never block and you could use > > > David> a mutex. > > > > > > Yeah. I hadn't thought of the accept case, thanks. > > > > > > David> This makes me think that ld.so is probably broken also. When we do > > > David> dlopen ld.so opens the library file and does several mmaps before > > > David> closing the file descriptor. These could leak out as well. > > > > > > Not to mention other places that libc may create fds. > > > > > > On some platforms fdwalk(3) can be used to make this close loop more > > > efficient. There's also closefrom(3). Linux doesn't seem to have > > > either of these, but Solaris does. > > > > > > Tom > > > > > How about this version? > > This is impressively heroic, but I really think you should measure the > performance improvement before committing this. :-) > > Andrew. > Hard data takes the fun out of things :-( Here is the test program: I open 50 files and then Runtime.exec("/bin/true") 10,000 times. ------------------------------------------------------------------------ import java.io.*; import java.util.*; public class ExecTest { public static void main(String args[]) { try { ArrayList<FileOutputStream> l = new ArrayList<FileOutputStream>(); for (int i = 0; i < 50; i++) { FileOutputStream fos = new FileOutputStream("junk" + i); l.add(fos); } Runtime r = Runtime.getRuntime(); long startTime = System.nanoTime(); for (int i = 0; i < 10000; i++) { Process p = r.exec("/bin/true"); if(false) { InputStream is = p.getInputStream(); byte buffer[] = new byte[1024]; for (;;) { int nr = is.read(buffer); if (-1 == nr) break; System.out.write(buffer, 0, nr); } } p.waitFor(); } long endTime = System.nanoTime(); System.out.println("Elapsed: " + (endTime - startTime)); } catch (Exception ex) { ex.printStackTrace(); } } } ------------------------------------------------------ I did three test runs of each of the following: 1) ulimit -n 1024; A corrected version of my last patch that gets a directory of /proc/self/fd and closes all of the descriptors found. 19158418000 19570065000 19396004000 2) ulimit -n 1024; close *all* descriptors up to the ulimit -n value. 16650209000 17505032000 16257549000 3) ulimit -n 10240; same code as case 2. 28571639000 28423899000 27640995000 4) ulimit -n 100; same code as case 2. 14759709000 16140129000 15765422000 What does all this mean? Well on my x86_64-pc-linux-gnu notebook, it takes about 1 second to close 10,000,000 times an invalid file descriptor. The savings of only closing open files would appear to make sense only if the default ulimit -n value is raised to a value greater than about 3000. The only reason to increase ulimit -n, from its default value of 1024, is if you expect to have a lot of open files. In this case This is all moot as the time to close the open files would be much greater than the overhead of closing invalid descriptors. So if we want to fix the bug, I would recommend my original patch. David Daney ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-21 22:31 ` David Daney @ 2007-03-22 10:46 ` Andrew Haley 2007-03-23 0:12 ` David Daney 0 siblings, 1 reply; 20+ messages in thread From: Andrew Haley @ 2007-03-22 10:46 UTC (permalink / raw) To: David Daney; +Cc: java-patches, tromey David Daney writes: > Andrew Haley wrote: > > David Daney writes: > > > How about this version? > > > > This is impressively heroic, but I really think you should measure the > > performance improvement before committing this. :-) > > Hard data takes the fun out of things :-( Haha! I am *such* a scientist! :-) > What does all this mean? Well on my x86_64-pc-linux-gnu notebook, > it takes about 1 second to close 10,000,000 times an invalid file > descriptor. The savings of only closing open files would appear to > make sense only if the default ulimit -n value is raised to a value > greater than about 3000. > > The only reason to increase ulimit -n, from its default value of > 1024, is if you expect to have a lot of open files. In this case > This is all moot as the time to close the open files would be much > greater than the overhead of closing invalid descriptors. > > So if we want to fix the bug, I would recommend my original patch. Yeah, we want to fix the bug. This is OK. Andrew. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-22 10:46 ` Andrew Haley @ 2007-03-23 0:12 ` David Daney 2007-03-23 1:16 ` David Daney 0 siblings, 1 reply; 20+ messages in thread From: David Daney @ 2007-03-23 0:12 UTC (permalink / raw) To: java-patches; +Cc: Andrew Haley, tromey [-- Attachment #1: Type: text/plain, Size: 2225 bytes --] Andrew Haley wrote: > David Daney writes: > > Andrew Haley wrote: > > > David Daney writes: > > > > > How about this version? > > > > > > This is impressively heroic, but I really think you should measure the > > > performance improvement before committing this. :-) > > > > Hard data takes the fun out of things :-( > > Haha! I am *such* a scientist! :-) > > > What does all this mean? Well on my x86_64-pc-linux-gnu notebook, > > it takes about 1 second to close 10,000,000 times an invalid file > > descriptor. The savings of only closing open files would appear to > > make sense only if the default ulimit -n value is raised to a value > > greater than about 3000. > > > > The only reason to increase ulimit -n, from its default value of > > 1024, is if you expect to have a lot of open files. In this case > > This is all moot as the time to close the open files would be much > > greater than the overhead of closing invalid descriptors. > > > > So if we want to fix the bug, I would recommend my original patch. > > Yeah, we want to fix the bug. This is OK. > > Andrew. > Attached is what I ended up committing. The only difference from the original patch is that I use the rlim_max value instead of the flim_cur value. This will yield the same result unless someone has been doing funny things with the limit. In which case we should use the rlim_max value in case there are any vestigial descriptors left open above the current limit. Tested on x86_64-pc-linux-gnu (FC6) with no regressions found. 2007-03-22 David Daney <ddaney@avtrex.com> PR libgcj/31228 * configure.ac: Add checks for getrlimit and sys/resource.h. * include/posix.h (_Jv_platform_close_on_exec): Remove. * include/config.h.in: Regenerate. * configure: Regenerate. * gnu/java/nio/channels/natFileChannelPosix.cc (open): Remove call to _Jv_platform_close_on_exec; * gnu/java/net/natPlainSocketImplPosix.cc (create): Likewise. (accept): Likewise. * gnu/java/net/natPlainDatagramSocketImplPosix.cc (create):Likewise. * java/lang/natPosixProcess.cc: Include sys/resource.h. (nativeSpawn): Close all file descriptors. Don't set FD_CLOEXEC on pipes. [-- Attachment #2: closeall.diff.txt --] [-- Type: text/plain, Size: 6749 bytes --] Index: configure.ac =================================================================== --- configure.ac (revision 123033) +++ configure.ac (working copy) @@ -1006,10 +1006,10 @@ else 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]) AC_CHECK_FUNCS(inet_aton inet_addr, break) - AC_CHECK_HEADERS(execinfo.h unistd.h dlfcn.h) + AC_CHECK_HEADERS(execinfo.h unistd.h dlfcn.h sys/resource.h) # Do an additional check on dld, HP-UX for example has dladdr in libdld.sl AC_CHECK_LIB(dl, dladdr, [ AC_DEFINE(HAVE_DLADDR, 1, [Define if you have dladdr()])], [ Index: include/posix.h =================================================================== --- include/posix.h (revision 123033) +++ include/posix.h (working copy) @@ -98,15 +98,6 @@ extern jlong _Jv_platform_nanotime (); extern void _Jv_platform_initialize (void); extern void _Jv_platform_initProperties (java::util::Properties*); -inline void -_Jv_platform_close_on_exec (jint fd) -{ - // Ignore errors. - ::fcntl (fd, F_SETFD, FD_CLOEXEC); -} - -#undef fcntl - #ifdef JV_HASH_SYNCHRONIZATION #ifndef HAVE_USLEEP_DECL extern "C" int usleep (useconds_t useconds); Index: include/config.h.in =================================================================== --- include/config.h.in (revision 123033) +++ include/config.h.in (working copy) @@ -127,6 +127,9 @@ /* Define to 1 if you have the `getpwuid_r' function. */ #undef HAVE_GETPWUID_R +/* Define to 1 if you have the `getrlimit' function. */ +#undef HAVE_GETRLIMIT + /* Define to 1 if you have the `gettimeofday' function. */ #undef HAVE_GETTIMEOFDAY @@ -316,6 +319,9 @@ /* Define to 1 if you have the <sys/ioctl.h> header file. */ #undef HAVE_SYS_IOCTL_H +/* Define to 1 if you have the <sys/resource.h> header file. */ +#undef HAVE_SYS_RESOURCE_H + /* Define to 1 if you have the <sys/rw_lock.h> 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 <fcntl.h> #include <sys/types.h> #include <sys/wait.h> +#ifdef HAVE_SYS_RESOURCE_H +#include <sys/resource.h> +#endif #include <signal.h> #include <string.h> #include <stdlib.h> @@ -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_max - 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); - } } ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-23 0:12 ` David Daney @ 2007-03-23 1:16 ` David Daney 2007-03-23 20:01 ` Tom Tromey 0 siblings, 1 reply; 20+ messages in thread From: David Daney @ 2007-03-23 1:16 UTC (permalink / raw) To: Andrew Haley, tromey; +Cc: java-patches David Daney wrote: > Attached is what I ended up committing. > > The only difference from the original patch is that I use the rlim_max > value instead of the flim_cur value. This will yield the same result > unless someone has been doing funny things with the limit. In which > case we should use the rlim_max value in case there are any vestigial > descriptors left open above the current limit. > > Tested on x86_64-pc-linux-gnu (FC6) with no regressions found. > > 2007-03-22 David Daney <ddaney@avtrex.com> > > PR libgcj/31228 > * configure.ac: Add checks for getrlimit and sys/resource.h. > * include/posix.h (_Jv_platform_close_on_exec): Remove. > * include/config.h.in: Regenerate. > * configure: Regenerate. > * gnu/java/nio/channels/natFileChannelPosix.cc (open): Remove call to > _Jv_platform_close_on_exec; > * gnu/java/net/natPlainSocketImplPosix.cc (create): Likewise. > (accept): Likewise. > * gnu/java/net/natPlainDatagramSocketImplPosix.cc (create):Likewise. > * java/lang/natPosixProcess.cc: Include sys/resource.h. > (nativeSpawn): Close all file descriptors. Don't set FD_CLOEXEC on > pipes. Should I put this on the 4.2 branch as well? If you think it is a good idea, I will. David Daney. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-23 1:16 ` David Daney @ 2007-03-23 20:01 ` Tom Tromey 2007-03-23 20:22 ` David Daney 0 siblings, 1 reply; 20+ messages in thread From: Tom Tromey @ 2007-03-23 20:01 UTC (permalink / raw) To: David Daney; +Cc: Andrew Haley, java-patches >>>>> "David" == David Daney <ddaney@avtrex.com> writes: David> Should I put this on the 4.2 branch as well? If you think it is a David> good idea, I will. It seems like a good bug fix, so yeah. I didn't look, though, to see what state 4.2 is in. Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-23 20:01 ` Tom Tromey @ 2007-03-23 20:22 ` David Daney 2007-03-26 0:10 ` Mark Mitchell 0 siblings, 1 reply; 20+ messages in thread From: David Daney @ 2007-03-23 20:22 UTC (permalink / raw) To: Mark Mitchell; +Cc: tromey, Andrew Haley, java-patches Mark, I would like to put the patch for PR 31228, a libgcj bug, on the 4.2 branch. The libgcj maintainer (Tom Tromey) is in agreement. As release manager, do you have an objections? Thanks, David Daney Tom Tromey wrote: >>>>>> "David" == David Daney <ddaney@avtrex.com> writes: > > David> Should I put this on the 4.2 branch as well? If you think it is a > David> good idea, I will. > > It seems like a good bug fix, so yeah. > I didn't look, though, to see what state 4.2 is in. > > Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-23 20:22 ` David Daney @ 2007-03-26 0:10 ` Mark Mitchell 2007-03-26 3:37 ` David Daney 0 siblings, 1 reply; 20+ messages in thread From: Mark Mitchell @ 2007-03-26 0:10 UTC (permalink / raw) To: David Daney; +Cc: tromey, Andrew Haley, java-patches David Daney wrote: > I would like to put the patch for PR 31228, a libgcj bug, on the 4.2 > branch. The libgcj maintainer (Tom Tromey) is in agreement. > > As release manager, do you have an objections? Bugzilla doesn't indicate that this is a regression; is there a compelling reason that this needs to go on 4.2? -- Mark Mitchell CodeSourcery mark@codesourcery.com (650) 331-3385 x713 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-26 0:10 ` Mark Mitchell @ 2007-03-26 3:37 ` David Daney 2007-03-26 5:41 ` Mark Mitchell 0 siblings, 1 reply; 20+ messages in thread From: David Daney @ 2007-03-26 3:37 UTC (permalink / raw) To: Mark Mitchell; +Cc: tromey, Andrew Haley, java-patches Mark Mitchell wrote: > David Daney wrote: > > >> I would like to put the patch for PR 31228, a libgcj bug, on the 4.2 >> branch. The libgcj maintainer (Tom Tromey) is in agreement. >> >> As release manager, do you have an objections? >> > > Bugzilla doesn't indicate that this is a regression; is there a > compelling reason that this needs to go on 4.2? > > It is not a regression. The bug is a race condition for which there is no evidence that it has ever actually caused undesirable behavior or even occurred. It is however a potential security hole in libgcj. If this were a patch for a Primary GCC language, it might not be suitable 4.2 at this point. However, the libgcj maintainers sometimes like to fix problems even at this late stage. I have bootstrapped and tested the patch on the 4.2 branch. So it is ready to go. I guess I don't really have strong feelings about it one way or the other. If the Mark or the java maintainers have a preference that the patch be committed or not, I will follow their recommendations. David Daney ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-26 3:37 ` David Daney @ 2007-03-26 5:41 ` Mark Mitchell 2007-03-26 6:11 ` David Daney 0 siblings, 1 reply; 20+ messages in thread From: Mark Mitchell @ 2007-03-26 5:41 UTC (permalink / raw) To: David Daney; +Cc: tromey, Andrew Haley, java-patches David Daney wrote: > It is not a regression. The bug is a race condition for which there is > no evidence that it has ever actually caused undesirable behavior or > even occurred. It is however a potential security hole in libgcj. Security issues are a big deal; go ahead and apply the patch. > If this were a patch for a Primary GCC language, it might not be > suitable 4.2 at this point. However, the libgcj maintainers sometimes > like to fix problems even at this late stage. Yes, but that's something which we should be avoiding. Fortran and Java are both grown-up now, and have significant userbases; all the same considerations which apply to C and C++ apply to them as well, at this point. Thanks, -- Mark Mitchell CodeSourcery mark@codesourcery.com (650) 331-3385 x713 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch] PR 31228, Fix close-on-exec race. 2007-03-26 5:41 ` Mark Mitchell @ 2007-03-26 6:11 ` David Daney 0 siblings, 0 replies; 20+ messages in thread From: David Daney @ 2007-03-26 6:11 UTC (permalink / raw) To: java-patches; +Cc: Mark Mitchell, tromey, Andrew Haley [-- Attachment #1: Type: text/plain, Size: 1085 bytes --] Mark Mitchell wrote: > David Daney wrote: > > >> It is not a regression. The bug is a race condition for which there is >> no evidence that it has ever actually caused undesirable behavior or >> even occurred. It is however a potential security hole in libgcj. >> > > Security issues are a big deal; go ahead and apply the patch. > Thanks Mark. Committed as revision 123208 to the gcc-4_2-branch. 2007-03-25 David Daney <ddaney@avtrex.com> PR libgcj/31228 * configure.ac: Add checks for getrlimit and sys/resource.h. * include/posix.h (_Jv_platform_close_on_exec): Remove. * include/config.h.in: Regenerate. * configure: Regenerate. * gnu/java/nio/channels/natFileChannelPosix.cc (open): Remove call to _Jv_platform_close_on_exec; * gnu/java/net/natPlainSocketImplPosix.cc (create): Likewise. (accept): Likewise. * gnu/java/net/natPlainDatagramSocketImplPosix.cc (create):Likewise. * java/lang/natPosixProcess.cc: Include sys/resource.h. (nativeSpawn): Close all file descriptors. Don't set FD_CLOEXEC on pipes. [-- Attachment #2: closeall.4_2.diff.txt --] [-- Type: text/plain, Size: 7156 bytes --] Index: configure =================================================================== --- configure (revision 123164) +++ configure (working copy) @@ -7687,7 +7687,7 @@ if test "$ac_x_libraries" = no; then # See if we find them without any special options. # Don't add to $LIBS permanently. ac_save_LIBS=$LIBS - LIBS="-lXt $LIBS" + LIBS="-lX11 $LIBS" if test x$gcc_no_link = xyes; then { { echo "$as_me:$LINENO: error: Link tests are not allowed after GCC_NO_EXECUTABLES." >&5 echo "$as_me: error: Link tests are not allowed after GCC_NO_EXECUTABLES." >&2;} @@ -9414,12 +9414,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` @@ -9875,7 +9876,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 123164) +++ 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 123164) +++ 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 123164) +++ 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: configure.ac =================================================================== --- configure.ac (revision 123164) +++ configure.ac (working copy) @@ -906,10 +906,10 @@ else 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]) AC_CHECK_FUNCS(inet_aton inet_addr, break) - AC_CHECK_HEADERS(execinfo.h unistd.h dlfcn.h) + AC_CHECK_HEADERS(execinfo.h unistd.h dlfcn.h sys/resource.h) # Do an additional check on dld, HP-UX for example has dladdr in libdld.sl AC_CHECK_LIB(dl, dladdr, [ AC_DEFINE(HAVE_DLADDR, 1, [Define if you have dladdr()])], [ Index: java/lang/natPosixProcess.cc =================================================================== --- java/lang/natPosixProcess.cc (revision 123164) +++ java/lang/natPosixProcess.cc (working copy) @@ -17,6 +17,9 @@ details. */ #include <fcntl.h> #include <sys/types.h> #include <sys/wait.h> +#ifdef HAVE_SYS_RESOURCE_H +#include <sys/resource.h> +#endif #include <signal.h> #include <string.h> #include <stdlib.h> @@ -341,7 +344,31 @@ java::lang::ConcreteProcess::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_max - 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); @@ -425,11 +452,4 @@ java::lang::ConcreteProcess::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); - fcntl (errp[0], F_SETFD, FD_CLOEXEC); - } } Index: include/posix.h =================================================================== --- include/posix.h (revision 123164) +++ include/posix.h (working copy) @@ -91,15 +91,6 @@ extern jlong _Jv_platform_nanotime (); extern void _Jv_platform_initialize (void); extern void _Jv_platform_initProperties (java::util::Properties*); -inline void -_Jv_platform_close_on_exec (jint fd) -{ - // Ignore errors. - ::fcntl (fd, F_SETFD, FD_CLOEXEC); -} - -#undef fcntl - #ifdef JV_HASH_SYNCHRONIZATION #ifndef HAVE_USLEEP_DECL extern "C" int usleep (useconds_t useconds); Index: include/config.h.in =================================================================== --- include/config.h.in (revision 123164) +++ include/config.h.in (working copy) @@ -124,6 +124,9 @@ /* Define to 1 if you have the `getpwuid_r' function. */ #undef HAVE_GETPWUID_R +/* Define to 1 if you have the `getrlimit' function. */ +#undef HAVE_GETRLIMIT + /* Define to 1 if you have the `gettimeofday' function. */ #undef HAVE_GETTIMEOFDAY @@ -304,6 +307,9 @@ /* Define to 1 if you have the <sys/ioctl.h> header file. */ #undef HAVE_SYS_IOCTL_H +/* Define to 1 if you have the <sys/resource.h> header file. */ +#undef HAVE_SYS_RESOURCE_H + /* Define to 1 if you have the <sys/rw_lock.h> header file. */ #undef HAVE_SYS_RW_LOCK_H ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2007-03-26 6:11 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-03-19 7:03 [Patch] PR 31228, Fix close-on-exec race David Daney 2007-03-20 21:25 ` Tom Tromey 2007-03-20 21:37 ` David Daney 2007-03-20 21:57 ` David Daney 2007-03-20 22:12 ` Tom Tromey 2007-03-20 22:24 ` David Daney 2007-03-21 10:10 ` Andrew Haley 2007-03-21 17:34 ` David Daney 2007-03-21 17:39 ` David Daney 2007-03-21 17:54 ` Andrew Haley 2007-03-21 22:31 ` David Daney 2007-03-22 10:46 ` Andrew Haley 2007-03-23 0:12 ` David Daney 2007-03-23 1:16 ` David Daney 2007-03-23 20:01 ` Tom Tromey 2007-03-23 20:22 ` David Daney 2007-03-26 0:10 ` Mark Mitchell 2007-03-26 3:37 ` David Daney 2007-03-26 5:41 ` Mark Mitchell 2007-03-26 6:11 ` David Daney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).