[Patch] PR 31228, Fix close-on-exec race.

[Patch] PR 31228, Fix close-on-exec race.

[David Daney]

[-- 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);
-    }
 }

Re: [Patch] PR 31228, Fix close-on-exec race.

[Tom Tromey]

>>>>> "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

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

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

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

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

Re: [Patch] PR 31228, Fix close-on-exec race.

[Tom Tromey]

>>>>> "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

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

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.

Re: [Patch] PR 31228, Fix close-on-exec race.

[Andrew Haley]

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.

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

[-- 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);
-    }
 }

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

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

Re: [Patch] PR 31228, Fix close-on-exec race.

[Andrew Haley]

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.

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

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

Re: [Patch] PR 31228, Fix close-on-exec race.

[Andrew Haley]

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.

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

[-- 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);
-    }
 }

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

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.

Re: [Patch] PR 31228, Fix close-on-exec race.

[Tom Tromey]

>>>>> "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

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

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

Re: [Patch] PR 31228, Fix close-on-exec race.

[Mark Mitchell]

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

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

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

Re: [Patch] PR 31228, Fix close-on-exec race.

[Mark Mitchell]

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

Re: [Patch] PR 31228, Fix close-on-exec race.

[David Daney]

[-- 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