From: David Daney <ddaney@avtrex.com>
To: java-patches@gcc.gnu.org
Cc: Mark Mitchell <mark@codesourcery.com>,
tromey@redhat.com, Andrew Haley <aph@redhat.com>
Subject: Re: [Patch] PR 31228, Fix close-on-exec race.
Date: Mon, 26 Mar 2007 06:11:00 -0000 [thread overview]
Message-ID: <460763EE.1030908@avtrex.com> (raw)
In-Reply-To: <46075CDF.9080304@codesourcery.com>
[-- 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
prev parent reply other threads:[~2007-03-26 6:11 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-03-19 7:03 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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=460763EE.1030908@avtrex.com \
--to=ddaney@avtrex.com \
--cc=aph@redhat.com \
--cc=java-patches@gcc.gnu.org \
--cc=mark@codesourcery.com \
--cc=tromey@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).