hg: jdk8/tl/jdk: 8004863: Infinite Loop in KeepAliveStream

2012-12-18 Thread martinrb 
Changeset: 0fabdf676395
Author:martin
Date:  2012-12-17 18:39 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0fabdf676395

8004863: Infinite Loop in KeepAliveStream
Reviewed-by: chegar

! src/share/classes/sun/net/www/http/KeepAliveStream.java
+ test/sun/net/www/http/KeepAliveStream/InfiniteLoop.java

Re: Infinite Loop in KeepAliveStream

2012-12-18 Thread Martin Buchholz 
Pushed.

On Mon, Dec 17, 2012 at 11:18 PM, Chris Hegarty chris.hega...@oracle.comwrote:

 Thank you Martin, looks good.

 -Chris

 On 18 Dec 2012, at 02:45, Martin Buchholz marti...@google.com wrote:

 I created a webrev


 http://cr.openjdk.java.net/~martin/webrevs/openjdk8/KeepAliveStream-infloop/

 incorporating Chris' test and propose to push this change to
 ssh://mar...@hg.openjdk.java.net/jdk8/tl-gate/jdk

Re: Infinite Loop in KeepAliveStream

2012-12-17 Thread Chris Hegarty 
Thank you Martin, looks good.

-Chris

On 18 Dec 2012, at 02:45, Martin Buchholz marti...@google.com wrote:

 I created a webrev
 
 http://cr.openjdk.java.net/~martin/webrevs/openjdk8/KeepAliveStream-infloop/
 
 incorporating Chris' test and propose to push this change to 
 ssh://mar...@hg.openjdk.java.net/jdk8/tl-gate/jdk

Re: Infinite Loop in KeepAliveStream

2012-12-16 Thread Chris Hegarty 


On 15/12/2012 01:56, Martin Buchholz wrote:

...

I was trying to avoid ever calling skip with a larger argument than
available(), trying to obey the comment
// Do this ONLY if the skip won't block.
So my version feels safer, although again I don't have the full context.


OK, makes sense. I'm ok with your version. This code seems to have been 
written with thread-safety half in mind, your change is probably the 
safest.


Can you also include the test in the changeset? Or do you want me to 
push it for you?


-Chris.

Re: Infinite Loop in KeepAliveStream

2012-12-14 Thread Martin Buchholz 
On Thu, Dec 13, 2012 at 7:26 AM, Chris Hegarty chris.hega...@oracle.comwrote:

 I think what you suggested should work. But how about a simpler version?



  if (expected  count) {
  long nskip = expected - count;
  if (nskip = available()) {
 -long n = 0;
 -while (n  nskip) {
 -nskip = nskip - n;
 -n = skip(nskip);
 +while (nskip  0  available()  0) {

 +skip(nskip);
 +nskip = expected - count;
  }


I was trying to avoid ever calling skip with a larger argument than
available(), trying to obey the comment
// Do this ONLY if the skip won't block.
So my version feels safer, although again I don't have the full context.



 -Chris


 On 12/13/2012 12:49 AM, Martin Buchholz wrote:



 On Tue, Dec 11, 2012 at 7:40 AM, Chris Hegarty chris.hega...@oracle.com
 mailto:chris.hegarty@oracle.**com chris.hega...@oracle.com wrote:

 Hi Martin,

 Thank you for reporting this issue. I filed 8004863: Infinite Loop
 in KeepAliveStream, to track it.

 I put together a small test to reproduce the problem (inline below).
 It is racey, but shows the problem most of the time on my machine.

 I tried your suggested patch, but found that there were cases where
 not enough data was being read off the stream, when it was being
 closed. This causes a problem for subsequent requests on that
 stream. The suggested patch below resolves this, and should also
 resolve the problem you are seeing ( patch against jdk8 ).

 Is this something that you want to run with, or would you prefer for
 me to get it into jdk8?

 diff -r fda257689786
 src/share/classes/sun/net/www/**__http/KeepAliveStream.java
 --- a/src/share/classes/sun/net/__**www/http/KeepAliveStream.java

 Mon Dec 10 10:52:11 2012 +0900
 +++ b/src/share/classes/sun/net/__**www/http/KeepAliveStream.java

 Tue Dec 11 15:30:50 2012 +
 @@ -83,10 +83,9 @@ class KeepAliveStream extends MeteredStr
   if (expected  count) {
   long nskip = expected - count;

   if (nskip = available()) {
 -long n = 0;
 -while (n  nskip) {
 -nskip = nskip - n;
 -n = skip(nskip);
 +while (nskip  0) {
 +skip(nskip);
 +nskip = expected - count;
   }


 I'm still having a hard time understanding the code.  It looks to me
 like the above still has issues if there is an asynchronous close in
 this loop.  In that case skip() will make no progress and always return
 0, and we will continue to see nskip  0.

 How about (completely untested)

   if (expected  count) {
   long nskip = (long) (expected - count);
   if (nskip = available()) {
 -long n = 0;
 -while (n  nskip) {
 -nskip = nskip - n;
 -n = skip(nskip);
 -}
 +do {} while ((nskip = (long) (expected - count))  0L
 +  skip(Math.min(nskip, available()))
   0L);
   } else if (expected =
 KeepAliveStreamCleaner.MAX_**DATA_REMAINING  !hurried) {
   //put this KeepAliveStream on the queue so that
 the data remaining
   //on the socket can be cleanup asyncronously.

Re: Infinite Loop in KeepAliveStream

2012-12-13 Thread Chris Hegarty 

I think what you suggested should work. But how about a simpler version?


 if (expected  count) {
 long nskip = expected - count;
 if (nskip = available()) {
-long n = 0;
-while (n  nskip) {
-nskip = nskip - n;
-n = skip(nskip);
+while (nskip  0  available()  0) {
+skip(nskip);
+nskip = expected - count;
 }


-Chris

On 12/13/2012 12:49 AM, Martin Buchholz wrote:



On Tue, Dec 11, 2012 at 7:40 AM, Chris Hegarty chris.hega...@oracle.com
mailto:chris.hega...@oracle.com wrote:

Hi Martin,

Thank you for reporting this issue. I filed 8004863: Infinite Loop
in KeepAliveStream, to track it.

I put together a small test to reproduce the problem (inline below).
It is racey, but shows the problem most of the time on my machine.

I tried your suggested patch, but found that there were cases where
not enough data was being read off the stream, when it was being
closed. This causes a problem for subsequent requests on that
stream. The suggested patch below resolves this, and should also
resolve the problem you are seeing ( patch against jdk8 ).

Is this something that you want to run with, or would you prefer for
me to get it into jdk8?

diff -r fda257689786
src/share/classes/sun/net/www/__http/KeepAliveStream.java
--- a/src/share/classes/sun/net/__www/http/KeepAliveStream.java
Mon Dec 10 10:52:11 2012 +0900
+++ b/src/share/classes/sun/net/__www/http/KeepAliveStream.java
Tue Dec 11 15:30:50 2012 +
@@ -83,10 +83,9 @@ class KeepAliveStream extends MeteredStr
  if (expected  count) {
  long nskip = expected - count;

  if (nskip = available()) {
-long n = 0;
-while (n  nskip) {
-nskip = nskip - n;
-n = skip(nskip);
+while (nskip  0) {
+skip(nskip);
+nskip = expected - count;
  }


I'm still having a hard time understanding the code.  It looks to me
like the above still has issues if there is an asynchronous close in
this loop.  In that case skip() will make no progress and always return
0, and we will continue to see nskip  0.

How about (completely untested)

  if (expected  count) {
  long nskip = (long) (expected - count);
  if (nskip = available()) {
-long n = 0;
-while (n  nskip) {
-nskip = nskip - n;
-n = skip(nskip);
-}
+do {} while ((nskip = (long) (expected - count))  0L
+  skip(Math.min(nskip, available()))
  0L);
  } else if (expected =
KeepAliveStreamCleaner.MAX_DATA_REMAINING  !hurried) {
  //put this KeepAliveStream on the queue so that
the data remaining
  //on the socket can be cleanup asyncronously.

Re: Infinite Loop in KeepAliveStream

2012-12-12 Thread Martin Buchholz 
On Tue, Dec 11, 2012 at 7:40 AM, Chris Hegarty chris.hega...@oracle.comwrote:

 Hi Martin,

 Thank you for reporting this issue. I filed 8004863: Infinite Loop in
 KeepAliveStream, to track it.

 I put together a small test to reproduce the problem (inline below). It is
 racey, but shows the problem most of the time on my machine.

 I tried your suggested patch, but found that there were cases where not
 enough data was being read off the stream, when it was being closed. This
 causes a problem for subsequent requests on that stream. The suggested
 patch below resolves this, and should also resolve the problem you are
 seeing ( patch against jdk8 ).

 Is this something that you want to run with, or would you prefer for me to
 get it into jdk8?

 diff -r fda257689786 src/share/classes/sun/net/www/**
 http/KeepAliveStream.java
 --- a/src/share/classes/sun/net/**www/http/KeepAliveStream.java   Mon Dec
 10 10:52:11 2012 +0900
 +++ b/src/share/classes/sun/net/**www/http/KeepAliveStream.java   Tue Dec
 11 15:30:50 2012 +
 @@ -83,10 +83,9 @@ class KeepAliveStream extends MeteredStr
  if (expected  count) {
  long nskip = expected - count;

  if (nskip = available()) {
 -long n = 0;
 -while (n  nskip) {
 -nskip = nskip - n;
 -n = skip(nskip);
 +while (nskip  0) {
 +skip(nskip);
 +nskip = expected - count;
  }


I'm still having a hard time understanding the code.  It looks to me like
the above still has issues if there is an asynchronous close in this loop.
 In that case skip() will make no progress and always return 0, and we will
continue to see nskip  0.

How about (completely untested)

 if (expected  count) {
 long nskip = (long) (expected - count);
 if (nskip = available()) {
-long n = 0;
-while (n  nskip) {
-nskip = nskip - n;
-n = skip(nskip);
-}
+do {} while ((nskip = (long) (expected - count))  0L
+  skip(Math.min(nskip, available())) 
0L);
 } else if (expected =
KeepAliveStreamCleaner.MAX_DATA_REMAINING  !hurried) {
 //put this KeepAliveStream on the queue so that the
data remaining
 //on the socket can be cleanup asyncronously.

Re: Infinite Loop in KeepAliveStream

2012-12-11 Thread Chris Hegarty 

Hi Martin,

Thank you for reporting this issue. I filed 8004863: Infinite Loop in 
KeepAliveStream, to track it.


I put together a small test to reproduce the problem (inline below). It 
is racey, but shows the problem most of the time on my machine.


I tried your suggested patch, but found that there were cases where not 
enough data was being read off the stream, when it was being closed. 
This causes a problem for subsequent requests on that stream. The 
suggested patch below resolves this, and should also resolve the problem 
you are seeing ( patch against jdk8 ).


Is this something that you want to run with, or would you prefer for me 
to get it into jdk8?


diff -r fda257689786 src/share/classes/sun/net/www/http/KeepAliveStream.java
--- a/src/share/classes/sun/net/www/http/KeepAliveStream.java	Mon Dec 10 
10:52:11 2012 +0900
+++ b/src/share/classes/sun/net/www/http/KeepAliveStream.java	Tue Dec 11 
15:30:50 2012 +

@@ -83,10 +83,9 @@ class KeepAliveStream extends MeteredStr
 if (expected  count) {
 long nskip = expected - count;
 if (nskip = available()) {
-long n = 0;
-while (n  nskip) {
-nskip = nskip - n;
-n = skip(nskip);
+while (nskip  0) {
+skip(nskip);
+nskip = expected - count;
 }

---
/*
 * Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License 
version

 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/*
 * @test
 * @bug XX
 * @summary 
 */

import com.sun.net.httpserver.HttpExchange;
import com.sun.net.httpserver.HttpHandler;
import com.sun.net.httpserver.HttpServer;
import java.io.*;
import java.net.*;
import java.util.concurrent.Phaser;

// Racey test, will not always fail, but if it does then we have a problem.

public class InfiniteLoop {

public static void main(String[] args) throws Exception {
HttpServer server = HttpServer.create(new InetSocketAddress(0), 0);
server.createContext(/test/InfiniteLoop, new RespHandler());
server.start();
try {
InetSocketAddress address = server.getAddress();
URL url = new URL(http://localhost:; + address.getPort()
  + /test/InfiniteLoop);
final Phaser phaser = new Phaser(2);
for (int i=0; i10; i++) {
HttpURLConnection uc = 
(HttpURLConnection)url.openConnection();

final InputStream is = uc.getInputStream();
final Thread thread = new Thread() {
public void run() {
try {
phaser.arriveAndAwaitAdvance();
while (is.read() != -1)
Thread.sleep(50);
} catch (Exception x) { x.printStackTrace(); }
}};
thread.start();
phaser.arriveAndAwaitAdvance();
is.close();
System.out.println(returned from close);
thread.join();
}
} finally {
server.stop(0);
}
}

static class RespHandler implements HttpHandler {
static final int RESP_LENGTH = 32 * 1024;
@Override
public void handle(HttpExchange t) throws IOException {
InputStream is  = t.getRequestBody();
byte[] ba = new byte[8192];
while(is.read(ba) != -1);

t.sendResponseHeaders(200, RESP_LENGTH);
try (OutputStream os = t.getResponseBody()) {
os.write(new byte[RESP_LENGTH]);
}
t.close();
}
}
}
---

-Chris.



On 12/11/2012 01:21 AM, Martin Buchholz wrote:

Hi sun/net/www maintainers,

Here at Google we have observed an infinite loop
in jdk/src/share/classes/sun/net/www/http/KeepAliveStream.java

  85: if (nskip = available()) {
  86

Infinite Loop in KeepAliveStream

2012-12-10 Thread Martin Buchholz 
Hi sun/net/www maintainers,

Here at Google we have observed an infinite loop
in jdk/src/share/classes/sun/net/www/http/KeepAliveStream.java

 85: if (nskip = available()) {
 86: long n = 0;
 87: while (n  nskip) {
 88: nskip = nskip - n;
 89: n = skip(nskip);
 90: }

If this stream is asynchronously closed (or perhaps, read, or skipped),
then skip will always return 0 (see MeteredStream)

public synchronized long skip(long n) throws IOException {

// REMIND: what does skip do on EOF
if (closed) {
return 0;
}

and you can clearly see an infinite loop.

Unfortunately, we are too clueless about this code to be able to reproduce
this infloop at will, but we are guessing you may be able to.

Here's an infloop stack trace snippet (line numbers from openjdk6 sources)

 sun.net.www.http.KeepAliveStream.close(KeepAliveStream.java:93)
 java.io.FilterInputStream.close(FilterInputStream.java:172)
 
sun.net.www.protocol.http.HttpURLConnection$HttpInputStream.close(HttpURLConnection.java:2625)
 org.apache.xerces.impl.XMLEntityManager$RewindableInputStream.close(Unknown
Source)
 org.apache.xerces.impl.io.UTF8Reader.close(Unknown Source)
 org.apache.xerces.impl.XMLEntityManager.closeReaders(Unknown Source)
 org.apache.xerces.parsers.XML11Configuration.cleanup(Unknown Source)
 org.apache.xerces.parsers.XML11Configuration.parse(Unknown Source)

Here's a possible patch that seems to make the code more correct in the
presence of asynchronous close, although guaranteeing completely correct
synchronization here seems difficult.

@@ -83,10 +83,8 @@
 if (expected  count) {
 long nskip = (long) (expected - count);
 if (nskip = available()) {
-long n = 0;
-while (n  nskip) {
-nskip = nskip - n;
-n = skip(nskip);
+while (nskip  0  nskip = available()) {
+nskip -= skip(nskip);
 }
 } else if (expected =
KeepAliveStreamCleaner.MAX_DATA_REMAINING  !hurried) {
 //put this KeepAliveStream on the queue so that the
data remaining

It's very likely you can come up with a better patch.

see also http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4392195