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.
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.
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
- References:
- [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.
- Re: [Patch] PR 31228, Fix close-on-exec race.