Jochen Wiedmann wrote:
Can anyone think of any reasons why this might cause problems? If not I'll
do the changes and submit a patch.
Please do. Not the first valuable patch from you.
Attached.
Here's what I've done: In the existing code the HttpServletRequestImpl
constructor reads the HTTP headers, and if the client doesn't send any
headers, the constructor hangs, and that hangs the thread that accepts
incoming connections. I've factored out the header reading code into a
new method (readHttpHeaders) which is called from the
ServletConnection.run method of the thread that processes the request:
public void run() throws Throwable {
try {
request.readHttpHeaders();
servlet.service(request, response);
} catch (Throwable t) {
if (!shuttingDown) {
throw t;
}
}
}
I also note that Connection class does this:
// set read timeout to 30 seconds
socket.setSoTimeout (30000);
That *should* mean that the maximum time a client can hang the server for is
30 seconds, but by observation that's clearly not happening and needs
further investigation.
Patches in that area should be welcome as well.
Also fixed. The problem was that although the Connection class set a 30
second timeout, the ServletConnection didn't - the constructor was
missing the call to setSoTimeout. The Connection class is used by the
WebServer class, ServletConnection is used by ServletWebServer.
In combination, these changes mean that a client that doesn't send any
headers can't hang the accept() thread, and that after 30 seconds, any
such misbehaving client will have its connection terminated.
--
Alan Burlison
--
Index: src/main/java/org/apache/xmlrpc/webserver/HttpServletRequestImpl.java
--- src/main/java/org/apache/xmlrpc/webserver/HttpServletRequestImpl.java Base
(BASE)
+++ src/main/java/org/apache/xmlrpc/webserver/HttpServletRequestImpl.java
Locally Modified (Based On LOCAL)
@@ -60,11 +60,11 @@
private ServletInputStream sistream;
private BufferedReader reader;
private boolean postParametersParsed;
- private final String method;
- private final String protocol;
- private final String uri;
- private final String queryString;
- private final String httpVersion;
+ private String method;
+ private String protocol;
+ private String uri;
+ private String queryString;
+ private String httpVersion;
private final Map headers = new HashMap();
private final Map attributes = new HashMap();
private Map parameters;
@@ -98,34 +98,36 @@
return c;
}
};
+ }
- /** Read the header lines, one by one. Note, that the size of
+ /**
+ * Read the header lines, one by one. Note, that the size of
* the buffer is a limitation of the maximum header length!
*/
+ public void readHttpHeaders()
+ throws IOException, ServletWebServer.Exception {
byte[] buffer = new byte[2048];
String line = readLine(buffer);
-
- StringTokenizer tokens = line!=null? new StringTokenizer(line): null;
- if (tokens==null || !tokens.hasMoreTokens()) {
- throw new ServletWebServer.Exception(400, "Bad Request",
-
"Unable to parse requests first line (should"
-
+ " be 'METHOD uri HTTP/version', was empty.");
+ StringTokenizer tokens =
+ line != null ? new StringTokenizer(line) : null;
+ if (tokens == null || !tokens.hasMoreTokens()) {
+ throw new ServletWebServer.Exception(400, "Bad Request", "Unable
to parse requests first line (should" +
+ " be 'METHOD uri HTTP/version', was empty.");
}
method = tokens.nextToken();
if (!"POST".equalsIgnoreCase(method)) {
- throw new ServletWebServer.Exception(400, "Bad Request",
-
"Expected 'POST' method, got " + method);
+ throw new ServletWebServer.Exception(400, "Bad Request", "Expected
'POST' method, got " +
+ method);
}
if (!tokens.hasMoreTokens()) {
- throw new ServletWebServer.Exception(400, "Bad Request",
-
"Unable to parse requests first line (should"
-
+ " be 'METHOD uri HTTP/version', was: " + line);
+ throw new ServletWebServer.Exception(400, "Bad Request", "Unable
to parse requests first line (should" +
+ " be 'METHOD uri HTTP/version', was: " + line);
}
String u = tokens.nextToken();
int offset = u.indexOf('?');
if (offset >= 0) {
uri = u.substring(0, offset);
- queryString = u.substring(offset+1);
+ queryString = u.substring(offset + 1);
} else {
uri = u;
queryString = null;
@@ -133,34 +135,32 @@
if (tokens.hasMoreTokens()) {
String v = tokens.nextToken().toUpperCase();
if (tokens.hasMoreTokens()) {
- throw new ServletWebServer.Exception(400, "Bad Request",
-
"Unable to parse requests first line (should"
-
+ " be 'METHOD uri HTTP/version', was: " + line);
+ throw new ServletWebServer.Exception(400, "Bad Request",
"Unable to parse requests first line (should" +
+ " be 'METHOD uri HTTP/version', was: " + line);
} else {
int index = v.indexOf('/');
if (index == -1) {
- throw new ServletWebServer.Exception(400, "Bad
Request",
-
"Unable to parse requests first line (should"
-
+ " be 'METHOD uri HTTP/version', was: " +
line);
+ throw new ServletWebServer.Exception(400, "Bad Request",
"Unable to parse requests first line (should" +
+ " be 'METHOD uri HTTP/version', was: " + line);
}
protocol = v.substring(0, index).toUpperCase();
- httpVersion = v.substring(index+1);
+ httpVersion = v.substring(index + 1);
}
} else {
httpVersion = "1.0";
protocol = "HTTP";
}
-
for (;;) {
line = HttpUtil.readLine(istream, buffer);
- if (line == null || line.length() == 0) {
+ if (line == null || line.length() == 0) {
break;
}
int off = line.indexOf(':');
if (off > 0) {
- addHeader(line.substring(0, off),
line.substring(off+1).trim());
+ addHeader(line.substring(0, off), line.substring(off +
1).trim());
} else {
- throw new ServletWebServer.Exception(400, "Bad
Request", "Unable to parse header line: " + line);
+ throw new ServletWebServer.Exception(400, "Bad Request",
"Unable to parse header line: " +
+ line);
}
}
contentBytesRemaining = getIntHeader("content-length");
Index: src/main/java/org/apache/xmlrpc/webserver/ServletConnection.java
--- src/main/java/org/apache/xmlrpc/webserver/ServletConnection.java Base (BASE)
+++ src/main/java/org/apache/xmlrpc/webserver/ServletConnection.java Locally
Modified (Based On LOCAL)
@@ -22,8 +22,6 @@
import java.net.Socket;
import javax.servlet.http.HttpServlet;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
import org.apache.xmlrpc.util.ThreadPool.InterruptableTask;
@@ -35,8 +33,8 @@
public class ServletConnection implements InterruptableTask {
private final HttpServlet servlet;
private final Socket socket;
- private final HttpServletRequest request;
- private final HttpServletResponse response;
+ private final HttpServletRequestImpl request;
+ private final HttpServletResponseImpl response;
private boolean shuttingDown;
/** Creates a new instance.
@@ -47,12 +45,15 @@
public ServletConnection(HttpServlet pServlet, Socket pSocket) throws
IOException {
servlet = pServlet;
socket = pSocket;
+ // Set read timeout to 30 seconds - same as Connection class
+ socket.setSoTimeout(30000);
request = new HttpServletRequestImpl(socket);
response = new HttpServletResponseImpl(socket);
}
public void run() throws Throwable {
try {
+ request.readHttpHeaders();
servlet.service(request, response);
} catch (Throwable t) {
if (!shuttingDown) {
