This is the mail archive of the
java-patches@gcc.gnu.org
mailing list for the Java project.
Re: [Patch] PR 31228, Fix close-on-exec race.
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.
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);
- }
}