RFR [8024521] (process) Async close issues with Process InputStream

[Ivan Gerasimov]

Hi Paul,

Thank you for looking at the webrev!

On 14.10.2013 12:50, Paul Sandoz wrote:
> Hi Ivan,
>
> Why do you require closeLock? is it not sufficient leave processExited as is and just add:
>
> public synchronized void close() throws IOException {
>      super.close();
> }

I want to limit the impact on the code, that's why I synchronize close() 
with only processExited().
If I made close() synchronized with all the stream's methods, then I 
think it could introduce some unpredictable drawback.

> Plus there was already some logic checking if close was asynchronously called (see BufferedInputStream.close):
>
>   372                         if (buf == null) // asynchronous close()?
>   373                             this.in = null;
>
> and this would no longer be required

Yes, you're right and these two lines can be removed.
I wouldn't remove the surrounding try {} catch (IOException) though, as 
the stream can still be closed asynchronously.

> I am somewhat struggling to understand the test (not too familiar with this area). Requires a more expert eye in this area than I.
>
> IIUC you are trying to induce the the output process fd of ExecLoop to "flip" to another fd opened by OpenLoop.

Yes.
I actually adopted a testcase provided by the reporter of the problem.
I only changed the test in such a way that it can be run in an automated 
mode.

Below is the exact comment from the original testcase.
It may be a good idea to include it in the regtest.

/* Following code is a test case that demonstrates a close race in 
ProcessPipeInputStream.
  *.
  * A snippet of the offending code within ProcessPipeInputStream:.
  *       // Called by the process reaper thread when the process exits..
  *       synchronized void processExited() {
  *          // Most BufferedInputStream methods are synchronized, but 
close()
  *           // is not, and so we have to handle concurrent racing close().
  *           try {
  *               InputStream in = this.in;
  *               if (in != null) {
  *                   byte[] stragglers = drainInputStream(in);
  *                   in.close();
  *
  * The comment even talks about the race. However, the code is not 
adequately
  * protecting against it. drainInputStream() (available() in 
particular) is not
  * threadsafe when a close happens in parallel. What's worse is that a 
subsequent
  * open() is likely to reassign the closed fd so drainInputStream ends 
up draining
  * the wrong fd! This can cause OOMs, data corruption on other fds, and 
other very
  * bad things.
  *.
  * Notes:.
  *  - Test creates a large sparse file in /tmp/bigsparsefile
  *  - Test uses "/proc/meminfo" for simple input..
  *  - Invoke using "-Xmx128m" to insure file size of 1G is greater than 
heap size
  *  - Test will run forever, if successful there will not be any OOM 
exceptions
  *  (on my test system they occur about 1 per minute)
  *  - You need multiple cores (I use 2 x Xeon E5620 = 8 cores, 16 threads).
  *..
  * The basic premise of the test is to create a 1GB sparse file. Then 
have lots
  * of threads opening and closing this file. In a loop within a 
separate thread use UNIXprocess
  * to run a simple command. We read the output of the command, make 
sure the process
  * has exited and then close the inputstream. When the bug happens, 
processExited()
  * will use FIONREAD to figure out how much data to drain BUT the fd it 
is checking
  * will be closed and reopened (now referring  to /tmp/bigsparsefile) 
so it will
  * see the size as 1GB and then attempt to allocate a buffer of that 
size and.
  * then OOM..
  */



>   The test concerns me since it is gonna use up loads of resources and proceed for 10 minutes until deciding beyond reasonable doubt that there is no issue.
When I ran the test across different machines (with the unpatched jdk) 
it reported from one to five races during 10 minutes.
We can surely decrease the timeout for the price of higher probability 
of a false positive result.


Sincerely yours,
Ivan

> Paul.
>
> On Oct 11, 2013, at 9:45 PM, Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:
>
>> Hello all!
>>
>> Would you please help review a fix?
>> http://bugs.sun.com/view_bug.do?bug_id=8024521
>>
>> As of JDK 1.7, ProcessPipeInputStream tries to drain the InputStream when the process exits.
>> However, it is racing with application code that could be closing the inputstream at the same time.
>> This can lead to processExited() operating on the wrong underlying file.
>>
>> The test included into the webrev demonstrates exactly that: Due to the race, the draining thread can suddenly start to read a huge file which will cause OutOfMemoryException.
>>
>> Here's the webrev with a simple fix, which synchronizes close() and processExited():
>> http://cr.openjdk.java.net/~igerasim/8024521/0/webrev/
>>
>> It's important to note that this simple fix isn't complete.
>> It only protects from the race when the file descriptor is closed by a call to Process#getInputStream().close().
>> The file descriptor can still be asynchronously closed in some other way, for example by calling to the native close() function.
>>
>> Sincerely yours,
>> Ivan Gerasimov
>>