diff --git a/java/org/apache/catalina/connector/BadRequestException.java b/java/org/apache/catalina/connector/BadRequestException.java new file mode 100644 index 000000000..7918f04d8 --- /dev/null +++ b/java/org/apache/catalina/connector/BadRequestException.java @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.catalina.connector; + +import java.io.IOException; + +/** + * Extend IOException to identify it as being caused by a bad request from a remote client. + */ +public class BadRequestException extends IOException { + + private static final long serialVersionUID = 1L; + + + // ------------------------------------------------------------ Constructors + + /** + * Construct a new BadRequestException with no other information. + */ + public BadRequestException() { + super(); + } + + + /** + * Construct a new BadRequestException for the specified message. + * + * @param message Message describing this exception + */ + public BadRequestException(String message) { + super(message); + } + + + /** + * Construct a new BadRequestException for the specified throwable. + * + * @param throwable Throwable that caused this exception + */ + public BadRequestException(Throwable throwable) { + super(throwable); + } + + + /** + * Construct a new BadRequestException for the specified message and throwable. + * + * @param message Message describing this exception + * @param throwable Throwable that caused this exception + */ + public BadRequestException(String message, Throwable throwable) { + super(message, throwable); + } +} \ No newline at end of file diff --git a/java/org/apache/catalina/connector/InputBuffer.java b/java/org/apache/catalina/connector/InputBuffer.java index b20ddde8f..74802af91 100644 --- a/java/org/apache/catalina/connector/InputBuffer.java +++ b/java/org/apache/catalina/connector/InputBuffer.java @@ -302,10 +302,38 @@ public int realReadBytes(byte cbuf[], int off, int len) if(state == INITIAL_STATE) state = BYTE_STATE; - int result = coyoteRequest.doRead(bb); - - return result; - + try { + return coyoteRequest.doRead(bb); + } catch (BadRequestException bre) { + // Set flag used by asynchronous processing to detect errors on non-container threads + coyoteRequest.setErrorException(bre); + + // In synchronous processing, this exception may be swallowed by the application so set error flags here. + // CH: We can't use ERROR_EXCEPTION because Tomcat 6 implements Servlet 2.5 and ERROR_EXCEPTION was + // introduced in Servlet 3.0 + // coyoteRequest.setAttribute(RequestDispatcher.ERROR_EXCEPTION, bre); + + coyoteRequest.getResponse().setStatus(400); + coyoteRequest.getResponse().setErrorTrue(); + + // Make the exception visible to the application + throw bre; + } catch (IOException ioe) { + // Set flag used by asynchronous processing to detect errors on non-container threads + coyoteRequest.setErrorException(ioe); + + // In synchronous processing, this exception may be swallowed by the application so set error flags here. + // CH: We can't use ERROR_EXCEPTION because Tomcat 6 implements Servlet 2.5 and ERROR_EXCEPTION was + // introduced in Servlet 3.0 + // coyoteRequest.setAttribute(RequestDispatcher.ERROR_EXCEPTION, bre); + + coyoteRequest.getResponse().setStatus(400); + coyoteRequest.getResponse().setErrorTrue(); + + // Any other IOException on a read is almost always due to the remote client aborting the request. + // Make the exception visible to the application + throw new ClientAbortException(ioe); + } } diff --git a/java/org/apache/catalina/connector/Response.java b/java/org/apache/catalina/connector/Response.java index f39bfb8f8..c1b4160b9 100644 --- a/java/org/apache/catalina/connector/Response.java +++ b/java/org/apache/catalina/connector/Response.java @@ -214,10 +214,6 @@ public void setContext(Context context) { */ private boolean isCharacterEncodingSet = false; - /** - * The error flag. - */ - protected boolean error = false; /** @@ -264,7 +260,6 @@ public void recycle() { usingWriter = false; appCommitted = false; included = false; - error = false; isCharacterEncodingSet = false; cookies.clear(); @@ -453,7 +448,7 @@ public boolean isClosed() { * Set the error flag. */ public void setError() { - error = true; + getCoyoteResponse().setErrorTrue(); } @@ -461,7 +456,7 @@ public void setError() { * Error flag accessor. */ public boolean isError() { - return error; + return getCoyoteResponse().getErrorFlag(); } @@ -1326,6 +1321,16 @@ public void sendError(int status, String message) */ public void sendRedirect(String location) throws IOException { + sendRedirect(location, SC_FOUND); + + } + + /** + * Internal method that allows a redirect to be sent with a status other + * than {@link HttpServletResponse#SC_FOUND} (302). No attempt is made to + * validate the status code. + */ + public void sendRedirect(String location, int status) throws IOException { if (isCommitted()) throw new IllegalStateException @@ -1341,7 +1346,7 @@ public void sendRedirect(String location) // Generate a temporary redirect to the specified location try { String absolute = toAbsolute(location); - setStatus(SC_FOUND); + setStatus(status); setHeader("Location", absolute); } catch (IllegalArgumentException e) { setStatus(SC_NOT_FOUND); diff --git a/java/org/apache/catalina/core/StandardHostValve.java b/java/org/apache/catalina/core/StandardHostValve.java index 3fd4b663a..1e675170e 100644 --- a/java/org/apache/catalina/core/StandardHostValve.java +++ b/java/org/apache/catalina/core/StandardHostValve.java @@ -275,11 +275,19 @@ protected void throwable(Request request, Response response, } } } else { - // A custom error-page has not been defined for the exception - // that was thrown during request processing. Check if an - // error-page for error code 500 was specified and if so, - // send that page back as the response. - response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + + /* + * A custom error-page has not been defined for the exception that was thrown during request processing. + * Set the status to 500 if an error status has not already been set and check for custom error-page for + * the status. + */ + if (response.getStatus() < HttpServletResponse.SC_BAD_REQUEST) { + response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } else if (throwable instanceof org.apache.catalina.connector.BadRequestException) { + response.setStatus(HttpServletResponse.SC_BAD_REQUEST); + } + + // The response is an error response.setError(); diff --git a/java/org/apache/catalina/valves/ErrorReportValve.java b/java/org/apache/catalina/valves/ErrorReportValve.java index 0c90c0b7d..14c8b4b61 100644 --- a/java/org/apache/catalina/valves/ErrorReportValve.java +++ b/java/org/apache/catalina/valves/ErrorReportValve.java @@ -121,8 +121,11 @@ public void invoke(Request request, Response response) ; } - response.sendError - (HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + if (throwable instanceof org.apache.catalina.connector.BadRequestException) { + response.sendError (HttpServletResponse.SC_BAD_REQUEST); + }else{ + response.sendError (HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } } diff --git a/java/org/apache/coyote/Request.java b/java/org/apache/coyote/Request.java index c37fffe9e..eda3ede21 100644 --- a/java/org/apache/coyote/Request.java +++ b/java/org/apache/coyote/Request.java @@ -145,6 +145,13 @@ public Request() { private int available = 0; private RequestInfo reqProcessorMX=new RequestInfo(this); + + + /** + * Holds request body reading error exception. + */ + private Exception errorException = null; + // ------------------------------------------------------------- Properties @@ -431,6 +438,33 @@ public int doRead(ByteChunk chunk) return n; } + // -------------------- Error tracking -------------------- + + /** + * Set the error Exception that occurred during the writing of the response processing. + * + * @param ex The exception that occurred + */ + public void setErrorException(Exception ex) { + errorException = ex; + } + + + /** + * Get the Exception that occurred during the writing of the response. + * + * @return The exception that occurred + */ + public Exception getErrorException() { + return errorException; + } + + + public boolean isExceptionPresent() { + return errorException != null; + } + + // -------------------- debug -------------------- @@ -510,6 +544,7 @@ public void recycle() { remoteUser.recycle(); authType.recycle(); attributes.clear(); + errorException = null; } // -------------------- Info -------------------- diff --git a/java/org/apache/coyote/Response.java b/java/org/apache/coyote/Response.java index 7056ff8cb..4890a24bd 100644 --- a/java/org/apache/coyote/Response.java +++ b/java/org/apache/coyote/Response.java @@ -127,6 +127,11 @@ public Response() { protected Request req; + /** + * The error flag. + */ + protected boolean error = false; + // ------------------------------------------------------------- Properties public Request getRequest() { @@ -255,6 +260,14 @@ public boolean isExceptionPresent() { return ( errorException != null ); } + public void setErrorTrue() { + error = true; + } + + public boolean getErrorFlag(){ + return error; + } + /** * Set request URI that caused an error during @@ -551,6 +564,7 @@ public void recycle() { errorURI = null; headers.clear(); + error = false; // update counters bytesWritten=0; } diff --git a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java index 430682452..53684a8db 100644 --- a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java +++ b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.util.Locale; +import org.apache.catalina.connector.BadRequestException; import org.apache.tomcat.util.buf.ByteChunk; import org.apache.tomcat.util.buf.HexUtils; import org.apache.coyote.InputBuffer; @@ -477,7 +478,9 @@ private boolean parseHeader() throws IOException { colon = true; } else if (!HttpParser.isToken(chr)) { // Non-token characters are illegal in header names - throw new IOException(sm.getString("chunkedInputFilter.invalidTrailerHeaderName")); + throwBadRequestException(sm.getString("chunkedInputFilter.invalidTrailerHeaderName")); + } else if (trailingHeaders.getEnd() >= trailingHeaders.getLimit()) { + throwBadRequestException(sm.getString("chunkedInputFilter.maxTrailer")); } else { trailingHeaders.append(chr); } @@ -543,6 +546,8 @@ private boolean parseHeader() throws IOException { eol = true; } else if (HttpParser.isControl(chr) && chr != Constants.HT) { throw new IOException(sm.getString("chunkedInputFilter.invalidTrailerHeaderValue")); + } else if (trailingHeaders.getEnd() >= trailingHeaders.getLimit()) { + throw new IOException(sm.getString("chunkedInputFilter.maxTrailer")); } else if (chr == Constants.SP || chr == Constants.HT) { trailingHeaders.append(chr); } else { @@ -568,6 +573,8 @@ private boolean parseHeader() throws IOException { chr = buf[pos]; if ((chr != Constants.SP) && (chr != Constants.HT)) { validLine = false; + } else if (trailingHeaders.getEnd() >= trailingHeaders.getLimit()) { + throwBadRequestException(sm.getString("chunkedInputFilter.maxTrailer")); } else { eol = false; // Copying one extra space in the buffer (since there must @@ -597,6 +604,11 @@ private void throwIOException(String msg) throws IOException { throw new IOException(msg); } + private void throwBadRequestException(String msg) throws IOException { + error = true; + throw new BadRequestException(msg); + } + private void throwEOFException(String msg) throws IOException { error = true; diff --git a/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java b/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java index aa5b3272b..20937144a 100644 --- a/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java +++ b/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java @@ -155,7 +155,7 @@ private void doTestExtensionSizeLimit(int len, boolean ok) throws Exception { String extName = ";foo="; StringBuilder extValue = new StringBuilder(len); for (int i = 0; i < (len - extName.length()); i++) { - extValue.append("x"); + extValue.append('x'); } String[] request = new String[]{ @@ -347,6 +347,83 @@ private void doTestChunkSize(boolean expectPass, } } + + @Test + public void testTrailerHeaderNameNotTokenThrowException() throws Exception { + doTestTrailerHeaderNameNotToken(false); + } + + @Test + public void testTrailerHeaderNameNotTokenSwallowException() throws Exception { + doTestTrailerHeaderNameNotToken(true); + } + + private void doTestTrailerHeaderNameNotToken(boolean swallowException) throws Exception { + + // Setup Tomcat instance + Tomcat tomcat = getTomcatInstance(); + + // No file system docBase required + Context ctx = tomcat.addContext("", null); + + Tomcat.addServlet(ctx, "servlet", new SwallowBodyServlet(swallowException)); + ctx.addServletMapping("/", "servlet"); + + tomcat.start(); + + String[] request = new String[]{ + "POST / HTTP/1.1" + SimpleHttpClient.CRLF + + "Host: localhost" + SimpleHttpClient.CRLF + + "Transfer-encoding: chunked" + SimpleHttpClient.CRLF + + "Content-Type: application/x-www-form-urlencoded" + SimpleHttpClient.CRLF + + "Connection: close" + SimpleHttpClient.CRLF + + SimpleHttpClient.CRLF + + "3" + SimpleHttpClient.CRLF + + "a=0" + SimpleHttpClient.CRLF + + "4" + SimpleHttpClient.CRLF + + "&b=1" + SimpleHttpClient.CRLF + + "0" + SimpleHttpClient.CRLF + + "x@trailer: Test" + SimpleHttpClient.CRLF + + SimpleHttpClient.CRLF }; + + TrailerClient client = new TrailerClient(tomcat.getConnector().getLocalPort()); + client.setRequest(request); + + client.connect(); + client.processRequest(); + // Expected to fail because of invalid trailer header name + Assert.assertTrue(client.getResponseLine(), client.isResponse400()); + } + + private static class SwallowBodyServlet extends HttpServlet { + private static final long serialVersionUID = 1L; + + private final boolean swallowException; + + SwallowBodyServlet(boolean swallowException) { + this.swallowException = swallowException; + } + + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) + throws ServletException, IOException { + resp.setContentType("text/plain"); + PrintWriter pw = resp.getWriter(); + + // Read the body + InputStream is = req.getInputStream(); + try { + while (is.read() > -1) { + } + pw.write("OK"); + } catch (IOException ioe) { + if (!swallowException) { + throw ioe; + } + } + } + } + private static class EchoHeaderServlet extends HttpServlet { private static final long serialVersionUID = 1L; @@ -354,7 +431,7 @@ private static class EchoHeaderServlet extends HttpServlet { private final boolean expectPass; - public EchoHeaderServlet(boolean expectPass) { + EchoHeaderServlet(boolean expectPass) { this.expectPass = expectPass; } @@ -384,7 +461,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throw ioe; } - pw.write(Integer.valueOf(count).toString()); + pw.write(Integer.toString(count)); // Headers should be visible now dumpHeader("x-trailer1", req, pw); @@ -413,7 +490,7 @@ private static class BodyReadServlet extends HttpServlet { private final boolean expectPass; private final int readLimit; - public BodyReadServlet(boolean expectPass, int readLimit) { + BodyReadServlet(boolean expectPass, int readLimit) { this.expectPass = expectPass; this.readLimit = readLimit; } @@ -440,7 +517,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throw ioe; } - pw.write(Integer.valueOf(countRead).toString()); + pw.write(Integer.toString(countRead)); } public boolean getExceptionDuringRead() { @@ -454,7 +531,7 @@ public int getCountRead() { private static class TrailerClient extends SimpleHttpClient { - public TrailerClient(int port) { + TrailerClient(int port) { setPort(port); }