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

[David Daney <ddaney at avtrex dot com>]

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