From 11ad5f515986a5e7f3e6c4c6a9e2cb51b689aab8 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 13:55:37 +0000 Subject: [PATCH 01/16] Handle errors consistently No functional change as trailer headers are read after the body. --- java/org/apache/coyote/http11/filters/ChunkedInputFilter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java index e2fdb0ae2e63..2b0ebe951fa7 100644 --- a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java +++ b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java @@ -500,7 +500,7 @@ 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")); + throwIOException(sm.getString("chunkedInputFilter.invalidTrailerHeaderName")); } else { trailingHeaders.append(chr); } From 7f9aab4b330cc74b64f08b7979a094d56fe48c64 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 08:59:41 -0600 Subject: [PATCH 02/16] Differentiate request cancellation from a bad request Introduce BadRequestException as a sub-class of IOException and the super class of ClientAbortException. This allows Tomcat to differentiate between an invalid request (e.g. invalid HTTP headers) and a request that the user cancelled before it completed. --- .../connector/BadRequestException.java | 68 +++++++++++++++++++ .../connector/ClientAbortException.java | 4 +- .../catalina/connector/InputBuffer.java | 6 +- .../catalina/core/ApplicationDispatcher.java | 10 +-- .../catalina/core/StandardWrapperValve.java | 12 ++-- 5 files changed, 83 insertions(+), 17 deletions(-) create mode 100644 java/org/apache/catalina/connector/BadRequestException.java diff --git a/java/org/apache/catalina/connector/BadRequestException.java b/java/org/apache/catalina/connector/BadRequestException.java new file mode 100644 index 000000000000..71a792d7fbd0 --- /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); + } +} diff --git a/java/org/apache/catalina/connector/ClientAbortException.java b/java/org/apache/catalina/connector/ClientAbortException.java index fa469f96eeea..a3ba607366e3 100644 --- a/java/org/apache/catalina/connector/ClientAbortException.java +++ b/java/org/apache/catalina/connector/ClientAbortException.java @@ -16,15 +16,13 @@ */ package org.apache.catalina.connector; -import java.io.IOException; - /** * Extend IOException to identify it as being caused by an abort of a request by * a remote client. * * @author Glenn L. Nielsen */ -public final class ClientAbortException extends IOException { +public final class ClientAbortException extends BadRequestException { private static final long serialVersionUID = 1L; diff --git a/java/org/apache/catalina/connector/InputBuffer.java b/java/org/apache/catalina/connector/InputBuffer.java index ac1b816207ae..9c6b94a13307 100644 --- a/java/org/apache/catalina/connector/InputBuffer.java +++ b/java/org/apache/catalina/connector/InputBuffer.java @@ -314,10 +314,12 @@ public int realReadBytes() throws IOException { try { return coyoteRequest.doRead(this); + } catch (BadRequestException bre) { + coyoteRequest.setErrorException(bre); + throw bre; } catch (IOException ioe) { coyoteRequest.setErrorException(ioe); - // An IOException on a read is almost always due to - // the remote client aborting the request. + // Any other IOException on a read is almost always due to the remote client aborting the request. throw new ClientAbortException(ioe); } } diff --git a/java/org/apache/catalina/core/ApplicationDispatcher.java b/java/org/apache/catalina/core/ApplicationDispatcher.java index f54ea6f13c83..4374ed792742 100644 --- a/java/org/apache/catalina/core/ApplicationDispatcher.java +++ b/java/org/apache/catalina/core/ApplicationDispatcher.java @@ -41,7 +41,7 @@ import org.apache.catalina.Context; import org.apache.catalina.Globals; import org.apache.catalina.Wrapper; -import org.apache.catalina.connector.ClientAbortException; +import org.apache.catalina.connector.BadRequestException; import org.apache.catalina.connector.Request; import org.apache.catalina.connector.RequestFacade; import org.apache.catalina.connector.Response; @@ -691,7 +691,7 @@ private void invoke(ServletRequest request, ServletResponse response, filterChain.doFilter(request, response); } // Servlet Service Method is called by the FilterChain - } catch (ClientAbortException e) { + } catch (BadRequestException e) { ioException = e; } catch (IOException e) { wrapper.getLogger().error(sm.getString("applicationDispatcher.serviceException", @@ -704,9 +704,9 @@ private void invoke(ServletRequest request, ServletResponse response, wrapper.unavailable(e); } catch (ServletException e) { Throwable rootCause = StandardWrapper.getRootCause(e); - if (!(rootCause instanceof ClientAbortException)) { - wrapper.getLogger().error(sm.getString("applicationDispatcher.serviceException", - wrapper.getName()), rootCause); + if (!(rootCause instanceof BadRequestException)) { + wrapper.getLogger().error(sm.getString("applicationDispatcher.serviceException", wrapper.getName()), + rootCause); } servletException = e; } catch (RuntimeException e) { diff --git a/java/org/apache/catalina/core/StandardWrapperValve.java b/java/org/apache/catalina/core/StandardWrapperValve.java index bc7e06783734..8455040ad650 100644 --- a/java/org/apache/catalina/core/StandardWrapperValve.java +++ b/java/org/apache/catalina/core/StandardWrapperValve.java @@ -31,7 +31,7 @@ import org.apache.catalina.Context; import org.apache.catalina.Globals; import org.apache.catalina.LifecycleException; -import org.apache.catalina.connector.ClientAbortException; +import org.apache.catalina.connector.BadRequestException; import org.apache.catalina.connector.Request; import org.apache.catalina.connector.Response; import org.apache.catalina.valves.ValveBase; @@ -199,7 +199,7 @@ public final void invoke(Request request, Response response) } } - } catch (ClientAbortException | CloseNowException e) { + } catch (BadRequestException | CloseNowException e) { if (container.getLogger().isDebugEnabled()) { container.getLogger().debug(sm.getString( "standardWrapper.serviceException", wrapper.getName(), @@ -235,11 +235,9 @@ public final void invoke(Request request, Response response) // do not want to do exception(request, response, e) processing } catch (ServletException e) { Throwable rootCause = StandardWrapper.getRootCause(e); - if (!(rootCause instanceof ClientAbortException)) { - container.getLogger().error(sm.getString( - "standardWrapper.serviceExceptionRoot", - wrapper.getName(), context.getName(), e.getMessage()), - rootCause); + if (!(rootCause instanceof BadRequestException)) { + container.getLogger().error(sm.getString("standardWrapper.serviceExceptionRoot", wrapper.getName(), + context.getName(), e.getMessage()), rootCause); } throwable = e; exception(request, response, e); From 4e2732a53d23215c4bd242a895076d5034a2761d Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 09:09:44 -0600 Subject: [PATCH 03/16] Use a 400 response for bad requests rather than a 500 response --- .../catalina/core/StandardWrapperValve.java | 9 +- .../coyote/http2/TestHttp2UpgradeHandler.java | 91 +++++++++++++------ webapps/docs/changelog.xml | 4 + 3 files changed, 73 insertions(+), 31 deletions(-) diff --git a/java/org/apache/catalina/core/StandardWrapperValve.java b/java/org/apache/catalina/core/StandardWrapperValve.java index 8455040ad650..d85ecb140363 100644 --- a/java/org/apache/catalina/core/StandardWrapperValve.java +++ b/java/org/apache/catalina/core/StandardWrapperValve.java @@ -199,7 +199,14 @@ public final void invoke(Request request, Response response) } } - } catch (BadRequestException | CloseNowException e) { + } catch (BadRequestException e) { + if (container.getLogger().isDebugEnabled()) { + container.getLogger().debug( + sm.getString("standardWrapper.serviceException", wrapper.getName(), context.getName()), e); + } + throwable = e; + exception(request, response, e, HttpServletResponse.SC_BAD_REQUEST); + } catch (CloseNowException e) { if (container.getLogger().isDebugEnabled()) { container.getLogger().debug(sm.getString( "standardWrapper.serviceException", wrapper.getName(), diff --git a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java index f96895980ad8..6f4e91a92647 100644 --- a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java +++ b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java @@ -58,15 +58,9 @@ public void testLargeHeader() throws Exception { // Body parser.readFrame(); - Assert.assertEquals( - "3-HeadersStart\n" + - "3-Header-[:status]-[200]\n" + - "3-Header-[x-ignore]-[...]\n" + - "3-Header-[content-type]-[text/plain;charset=UTF-8]\n" + - "3-Header-[content-length]-[2]\n" + - "3-Header-[date]-[Wed, 11 Nov 2015 19:18:42 GMT]\n" + - "3-HeadersEnd\n" + - "3-Body-2\n" + + Assert.assertEquals("3-HeadersStart\n" + "3-Header-[:status]-[200]\n" + "3-Header-[x-ignore]-[...]\n" + + "3-Header-[content-type]-[text/plain;charset=UTF-8]\n" + "3-Header-[content-length]-[2]\n" + + "3-Header-[date]-[Wed, 11 Nov 2015 19:18:42 GMT]\n" + "3-HeadersEnd\n" + "3-Body-2\n" + "3-EndOfStream\n", output.getTrace()); } @@ -136,14 +130,9 @@ private void doTestUpgradeWithRequestBody(boolean usePost, boolean useReader, bo openClientConnection(); - byte[] upgradeRequest = ((usePost ? "POST" : "GET") + - " /" + (useReader ? "?useReader=true " : " ") + "HTTP/1.1\r\n" + - "Host: localhost:" + getPort() + "\r\n" + - "Content-Length: 18\r\n" + - "Connection: Upgrade,HTTP2-Settings\r\n" + - "Upgrade: h2c\r\n" + - EMPTY_HTTP2_SETTINGS_HEADER + - "\r\n" + + byte[] upgradeRequest = ((usePost ? "POST" : "GET") + " /" + (useReader ? "?useReader=true " : " ") + + "HTTP/1.1\r\n" + "Host: localhost:" + getPort() + "\r\n" + "Content-Length: 18\r\n" + + "Connection: Upgrade,HTTP2-Settings\r\n" + "Upgrade: h2c\r\n" + EMPTY_HTTP2_SETTINGS_HEADER + "\r\n" + "Small request body").getBytes(StandardCharsets.ISO_8859_1); os.write(upgradeRequest); os.flush(); @@ -171,19 +160,61 @@ private void doTestUpgradeWithRequestBody(boolean usePost, boolean useReader, bo parser.readFrame(); parser.readFrame(); - Assert.assertEquals("0-Settings-[3]-[200]\n" + - "0-Settings-End\n" + - "0-Settings-Ack\n" + - "0-Ping-[0,0,0,0,0,0,0,1]\n" + - "1-HeadersStart\n" + - "1-Header-[:status]-[200]\n" + - "1-Header-[content-type]-[text/plain;charset=UTF-8]\n" + - "1-Header-[content-length]-[39]\n" + - "1-Header-[date]-[" + DEFAULT_DATE + "]\n" + - "1-HeadersEnd\n" + - "1-Body-39\n" + - "1-EndOfStream\n" - , output.getTrace()); + Assert.assertEquals("0-Settings-[3]-[200]\n" + "0-Settings-End\n" + "0-Settings-Ack\n" + + "0-Ping-[0,0,0,0,0,0,0,1]\n" + "1-HeadersStart\n" + "1-Header-[:status]-[200]\n" + + "1-Header-[content-type]-[text/plain;charset=UTF-8]\n" + "1-Header-[content-length]-[39]\n" + + "1-Header-[date]-[" + DEFAULT_DATE + "]\n" + "1-HeadersEnd\n" + "1-Body-39\n" + "1-EndOfStream\n", + output.getTrace()); + } + } + + + @Test + public void testActiveConnectionCountAndClientTimeout() throws Exception { + + enableHttp2(2, false, 10000, 10000, 4000, 2000, 2000); + + Tomcat tomcat = getTomcatInstance(); + + Context ctxt = tomcat.addContext("", null); + Tomcat.addServlet(ctxt, "simple", new SimpleServlet()); + ctxt.addServletMappingDecoded("/simple", "simple"); + + tomcat.start(); + + openClientConnection(); + doHttpUpgrade(); + sendClientPreface(); + validateHttp2InitialResponse(2); + + byte[] frameHeader = new byte[9]; + ByteBuffer headersPayload = ByteBuffer.allocate(128); + + byte[] dataFrameHeader = new byte[9]; + ByteBuffer dataFramePayload = ByteBuffer.allocate(128); + + // Should be able to make more than 2 requests even if they timeout + // since they should be removed from active connections once they + // timeout + for (int stream = 3; stream < 8; stream += 2) { + // Don't write the body. Allow the read to timeout. + buildPostRequest(frameHeader, headersPayload, false, dataFrameHeader, dataFramePayload, null, stream); + writeFrame(frameHeader, headersPayload); + + // 400 response (triggered by IOException trying to read body that never arrived) + parser.readFrame(); + Assert.assertTrue(output.getTrace(), + output.getTrace().startsWith(stream + "-HeadersStart\n" + stream + "-Header-[:status]-[400]\n")); + output.clearTrace(); + + // reset frame + parser.readFrame(); + Assert.assertEquals(stream + "-RST-[11]\n", output.getTrace()); + output.clearTrace(); + + // Prepare buffers for re-use + headersPayload.clear(); + dataFramePayload.clear(); } } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index bd62a89331e3..df1f22dc5835 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -151,6 +151,10 @@ Improve handling of failures within recycle() methods. (markt) + + Use a 400 status code to report an error due to a bad request (e.g. an + invalid trailer header) rather than a 500 status code. (markt) + From 77a147ebbc804eaa24c6578a2fe0ba1173bbd0a1 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 09:25:17 -0600 Subject: [PATCH 04/16] Ensure IOException on request read always triggers error handling --- .../catalina/connector/InputBuffer.java | 13 ++++ .../filters/TestChunkedInputFilter.java | 77 +++++++++++++++++++ webapps/docs/changelog.xml | 5 ++ 3 files changed, 95 insertions(+) diff --git a/java/org/apache/catalina/connector/InputBuffer.java b/java/org/apache/catalina/connector/InputBuffer.java index 9c6b94a13307..4e330f679bc7 100644 --- a/java/org/apache/catalina/connector/InputBuffer.java +++ b/java/org/apache/catalina/connector/InputBuffer.java @@ -29,6 +29,7 @@ import java.util.concurrent.ConcurrentHashMap; import jakarta.servlet.ReadListener; +import jakarta.servlet.RequestDispatcher; import org.apache.catalina.security.SecurityUtil; import org.apache.coyote.ActionCode; @@ -315,11 +316,23 @@ public int realReadBytes() throws IOException { try { return coyoteRequest.doRead(this); } 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. + coyoteRequest.setAttribute(RequestDispatcher.ERROR_EXCEPTION, bre); + coyoteRequest.getResponse().setStatus(400); + coyoteRequest.getResponse().setError(); + // 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. + coyoteRequest.setAttribute(RequestDispatcher.ERROR_EXCEPTION, ioe); + coyoteRequest.getResponse().setStatus(400); + coyoteRequest.getResponse().setError(); // 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/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java b/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java index 471e5ac75980..b25f5e7c33f8 100644 --- a/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java +++ b/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java @@ -428,6 +428,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.addServletMappingDecoded("/", "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; diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index df1f22dc5835..6f184f2ddfb5 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -155,6 +155,11 @@ Use a 400 status code to report an error due to a bad request (e.g. an invalid trailer header) rather than a 500 status code. (markt) + + Ensure that an IOException during the reading of the + request triggers always error handling, regardless of whether the + application swallows the exception. (markt) + From a6add28a404251eac88240587337ca6ab5a68da3 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 10:03:34 -0600 Subject: [PATCH 05/16] Fix back-port --- .../catalina/connector/InputBuffer.java | 1 + .../catalina/core/StandardWrapperValve.java | 159 +++++++----------- 2 files changed, 62 insertions(+), 98 deletions(-) diff --git a/java/org/apache/catalina/connector/InputBuffer.java b/java/org/apache/catalina/connector/InputBuffer.java index 4e330f679bc7..c818a21d9187 100644 --- a/java/org/apache/catalina/connector/InputBuffer.java +++ b/java/org/apache/catalina/connector/InputBuffer.java @@ -300,6 +300,7 @@ boolean isBlocking() { * * @throws IOException An underlying IOException occurred */ + @SuppressWarnings("deprecation") @Override public int realReadBytes() throws IOException { if (closed) { diff --git a/java/org/apache/catalina/core/StandardWrapperValve.java b/java/org/apache/catalina/core/StandardWrapperValve.java index d85ecb140363..8920ae87890a 100644 --- a/java/org/apache/catalina/core/StandardWrapperValve.java +++ b/java/org/apache/catalina/core/StandardWrapperValve.java @@ -42,8 +42,7 @@ import org.apache.tomcat.util.res.StringManager; /** - * Valve that implements the default basic behavior for the - * StandardWrapper container implementation. + * Valve that implements the default basic behavior for the StandardWrapper container implementation. * * @author Craig R. McClanahan */ @@ -52,9 +51,9 @@ final class StandardWrapperValve extends ValveBase { private static final StringManager sm = StringManager.getManager(StandardWrapperValve.class); - //------------------------------------------------------ Constructor + // ------------------------------------------------------ Constructor - public StandardWrapperValve() { + StandardWrapperValve() { super(true); } @@ -74,24 +73,22 @@ public StandardWrapperValve() { // --------------------------------------------------------- Public Methods /** - * Invoke the servlet we are managing, respecting the rules regarding - * servlet lifecycle and SingleThreadModel support. + * Invoke the servlet we are managing, respecting the rules regarding servlet lifecycle support. * - * @param request Request to be processed + * @param request Request to be processed * @param response Response to be produced * - * @exception IOException if an input/output error occurred + * @exception IOException if an input/output error occurred * @exception ServletException if a servlet error occurred */ @Override - public final void invoke(Request request, Response response) - throws IOException, ServletException { + public void invoke(Request request, Response response) throws IOException, ServletException { // Initialize local variables we may need boolean unavailable = false; Throwable throwable = null; // This should be a Request attribute... - long t1=System.currentTimeMillis(); + long t1 = System.currentTimeMillis(); requestCount.incrementAndGet(); StandardWrapper wrapper = (StandardWrapper) getContainer(); Servlet servlet = null; @@ -100,25 +97,14 @@ public final void invoke(Request request, Response response) // Check for the application being marked unavailable if (!context.getState().isAvailable()) { response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE, - sm.getString("standardContext.isUnavailable")); + sm.getString("standardContext.isUnavailable")); unavailable = true; } // Check for the servlet being marked unavailable if (!unavailable && wrapper.isUnavailable()) { - container.getLogger().info(sm.getString("standardWrapper.isUnavailable", - wrapper.getName())); - long available = wrapper.getAvailable(); - if ((available > 0L) && (available < Long.MAX_VALUE)) { - response.setDateHeader("Retry-After", available); - response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE, - sm.getString("standardWrapper.isUnavailable", - wrapper.getName())); - } else if (available == Long.MAX_VALUE) { - response.sendError(HttpServletResponse.SC_NOT_FOUND, - sm.getString("standardWrapper.notFound", - wrapper.getName())); - } + container.getLogger().info(sm.getString("standardWrapper.isUnavailable", wrapper.getName())); + checkWrapperAvailable(response, wrapper); unavailable = true; } @@ -128,29 +114,16 @@ public final void invoke(Request request, Response response) servlet = wrapper.allocate(); } } catch (UnavailableException e) { - container.getLogger().error( - sm.getString("standardWrapper.allocateException", - wrapper.getName()), e); - long available = wrapper.getAvailable(); - if ((available > 0L) && (available < Long.MAX_VALUE)) { - response.setDateHeader("Retry-After", available); - response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE, - sm.getString("standardWrapper.isUnavailable", - wrapper.getName())); - } else if (available == Long.MAX_VALUE) { - response.sendError(HttpServletResponse.SC_NOT_FOUND, - sm.getString("standardWrapper.notFound", - wrapper.getName())); - } + container.getLogger().error(sm.getString("standardWrapper.allocateException", wrapper.getName()), e); + checkWrapperAvailable(response, wrapper); } catch (ServletException e) { - container.getLogger().error(sm.getString("standardWrapper.allocateException", - wrapper.getName()), StandardWrapper.getRootCause(e)); + container.getLogger().error(sm.getString("standardWrapper.allocateException", wrapper.getName()), + StandardWrapper.getRootCause(e)); throwable = e; exception(request, response, e); } catch (Throwable e) { ExceptionUtils.handleThrowable(e); - container.getLogger().error(sm.getString("standardWrapper.allocateException", - wrapper.getName()), e); + container.getLogger().error(sm.getString("standardWrapper.allocateException", wrapper.getName()), e); throwable = e; exception(request, response, e); servlet = null; @@ -158,15 +131,13 @@ public final void invoke(Request request, Response response) MessageBytes requestPathMB = request.getRequestPathMB(); DispatcherType dispatcherType = DispatcherType.REQUEST; - if (request.getDispatcherType()==DispatcherType.ASYNC) { + if (request.getDispatcherType() == DispatcherType.ASYNC) { dispatcherType = DispatcherType.ASYNC; } - request.setAttribute(Globals.DISPATCHER_TYPE_ATTR,dispatcherType); - request.setAttribute(Globals.DISPATCHER_REQUEST_PATH_ATTR, - requestPathMB); + request.setAttribute(Globals.DISPATCHER_TYPE_ATTR, dispatcherType); + request.setAttribute(Globals.DISPATCHER_REQUEST_PATH_ATTR, requestPathMB); // Create the filter chain for this request - ApplicationFilterChain filterChain = - ApplicationFilterFactory.createFilterChain(request, wrapper, servlet); + ApplicationFilterChain filterChain = ApplicationFilterFactory.createFilterChain(request, wrapper, servlet); // Call the filter chain for this request // NOTE: This also calls the servlet's service() method @@ -180,8 +151,7 @@ public final void invoke(Request request, Response response) if (request.isAsyncDispatching()) { request.getAsyncContextInternal().doInternalDispatch(); } else { - filterChain.doFilter(request.getRequest(), - response.getResponse()); + filterChain.doFilter(request.getRequest(), response.getResponse()); } } finally { String log = SystemLogHandler.stopCapture(); @@ -193,8 +163,7 @@ public final void invoke(Request request, Response response) if (request.isAsyncDispatching()) { request.getAsyncContextInternal().doInternalDispatch(); } else { - filterChain.doFilter - (request.getRequest(), response.getResponse()); + filterChain.doFilter(request.getRequest(), response.getResponse()); } } @@ -208,36 +177,21 @@ public final void invoke(Request request, Response response) exception(request, response, e, HttpServletResponse.SC_BAD_REQUEST); } catch (CloseNowException e) { if (container.getLogger().isDebugEnabled()) { - container.getLogger().debug(sm.getString( - "standardWrapper.serviceException", wrapper.getName(), - context.getName()), e); + container.getLogger().debug( + sm.getString("standardWrapper.serviceException", wrapper.getName(), context.getName()), e); } throwable = e; exception(request, response, e); } catch (IOException e) { - container.getLogger().error(sm.getString( - "standardWrapper.serviceException", wrapper.getName(), - context.getName()), e); + container.getLogger() + .error(sm.getString("standardWrapper.serviceException", wrapper.getName(), context.getName()), e); throwable = e; exception(request, response, e); } catch (UnavailableException e) { - container.getLogger().error(sm.getString( - "standardWrapper.serviceException", wrapper.getName(), - context.getName()), e); - // throwable = e; - // exception(request, response, e); + container.getLogger() + .error(sm.getString("standardWrapper.serviceException", wrapper.getName(), context.getName()), e); wrapper.unavailable(e); - long available = wrapper.getAvailable(); - if ((available > 0L) && (available < Long.MAX_VALUE)) { - response.setDateHeader("Retry-After", available); - response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE, - sm.getString("standardWrapper.isUnavailable", - wrapper.getName())); - } else if (available == Long.MAX_VALUE) { - response.sendError(HttpServletResponse.SC_NOT_FOUND, - sm.getString("standardWrapper.notFound", - wrapper.getName())); - } + checkWrapperAvailable(response, wrapper); // Do not save exception in 'throwable', because we // do not want to do exception(request, response, e) processing } catch (ServletException e) { @@ -250,9 +204,8 @@ public final void invoke(Request request, Response response) exception(request, response, e); } catch (Throwable e) { ExceptionUtils.handleThrowable(e); - container.getLogger().error(sm.getString( - "standardWrapper.serviceException", wrapper.getName(), - context.getName()), e); + container.getLogger() + .error(sm.getString("standardWrapper.serviceException", wrapper.getName(), context.getName()), e); throwable = e; exception(request, response, e); } finally { @@ -268,8 +221,7 @@ public final void invoke(Request request, Response response) } } catch (Throwable e) { ExceptionUtils.handleThrowable(e); - container.getLogger().error(sm.getString("standardWrapper.deallocateException", - wrapper.getName()), e); + container.getLogger().error(sm.getString("standardWrapper.deallocateException", wrapper.getName()), e); if (throwable == null) { throwable = e; exception(request, response, e); @@ -279,49 +231,60 @@ public final void invoke(Request request, Response response) // If this servlet has been marked permanently unavailable, // unload it and release this instance try { - if ((servlet != null) && - (wrapper.getAvailable() == Long.MAX_VALUE)) { + if ((servlet != null) && (wrapper.getAvailable() == Long.MAX_VALUE)) { wrapper.unload(); } } catch (Throwable e) { ExceptionUtils.handleThrowable(e); - container.getLogger().error(sm.getString("standardWrapper.unloadException", - wrapper.getName()), e); + container.getLogger().error(sm.getString("standardWrapper.unloadException", wrapper.getName()), e); if (throwable == null) { exception(request, response, e); } } - long t2=System.currentTimeMillis(); + long t2 = System.currentTimeMillis(); long time=t2-t1; processingTime += time; if( time > maxTime) { maxTime=time; } - if( time < minTime) { - minTime=time; + if (time < minTime) { + minTime = time; } } } + private void checkWrapperAvailable(Response response, StandardWrapper wrapper) throws IOException { + long available = wrapper.getAvailable(); + if ((available > 0L) && (available < Long.MAX_VALUE)) { + response.setDateHeader("Retry-After", available); + response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE, + sm.getString("standardWrapper.isUnavailable", wrapper.getName())); + } else if (available == Long.MAX_VALUE) { + response.sendError(HttpServletResponse.SC_NOT_FOUND, + sm.getString("standardWrapper.notFound", wrapper.getName())); + } + } + // -------------------------------------------------------- Private Methods /** - * Handle the specified ServletException encountered while processing - * the specified Request to produce the specified Response. Any - * exceptions that occur during generation of the exception report are - * logged and swallowed. + * Handle the specified ServletException encountered while processing the specified Request to produce the specified + * Response. Any exceptions that occur during generation of the exception report are logged and swallowed. * - * @param request The request being processed - * @param response The response being generated - * @param exception The exception that occurred (which possibly wraps - * a root cause exception + * @param request The request being processed + * @param response The response being generated + * @param exception The exception that occurred (which possibly wraps a root cause exception */ - private void exception(Request request, Response response, - Throwable exception) { + private void exception(Request request, Response response, Throwable exception) { + exception(request, response, exception, HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } + + @SuppressWarnings("deprecation") + private void exception(Request request, Response response, Throwable exception, int errorCode) { request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, exception); - response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + response.setStatus(errorCode); response.setError(); } From bcd90a1a5b81e3cb2715a0ddd2579c334bc42715 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 16:36:41 +0000 Subject: [PATCH 06/16] Use sendError() rather than working directly with Coyote response The primary benefit is that the standard error page mechanism is invoked. --- .../catalina/connector/InputBuffer.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/java/org/apache/catalina/connector/InputBuffer.java b/java/org/apache/catalina/connector/InputBuffer.java index c818a21d9187..bdcb0665a00b 100644 --- a/java/org/apache/catalina/connector/InputBuffer.java +++ b/java/org/apache/catalina/connector/InputBuffer.java @@ -300,7 +300,6 @@ boolean isBlocking() { * * @throws IOException An underlying IOException occurred */ - @SuppressWarnings("deprecation") @Override public int realReadBytes() throws IOException { if (closed) { @@ -317,21 +316,11 @@ public int realReadBytes() throws IOException { try { return coyoteRequest.doRead(this); } 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. - coyoteRequest.setAttribute(RequestDispatcher.ERROR_EXCEPTION, bre); - coyoteRequest.getResponse().setStatus(400); - coyoteRequest.getResponse().setError(); // Make the exception visible to the application + handleReadException(bre); 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. - coyoteRequest.setAttribute(RequestDispatcher.ERROR_EXCEPTION, ioe); - coyoteRequest.getResponse().setStatus(400); - coyoteRequest.getResponse().setError(); + handleReadException(ioe); // 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); @@ -339,6 +328,17 @@ public int realReadBytes() throws IOException { } + private void handleReadException(Exception e) throws IOException { + // Set flag used by asynchronous processing to detect errors on non-container threads + coyoteRequest.setErrorException(e); + // In synchronous processing, this exception may be swallowed by the application so set error flags here. + Request request = (Request) coyoteRequest.getNote(CoyoteAdapter.ADAPTER_NOTES); + Response response = request.getResponse(); + request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, e); + response.sendError(400); + } + + public int readByte() throws IOException { throwIfClosed(); From 516ab38e8bfa657c8690164ad467ba9aec490385 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 16:43:48 +0000 Subject: [PATCH 07/16] Better message if size limit is reached reading trailer header name --- java/org/apache/coyote/http11/filters/ChunkedInputFilter.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java index 2b0ebe951fa7..82aea66aa1da 100644 --- a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java +++ b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java @@ -501,6 +501,8 @@ private boolean parseHeader() throws IOException { } else if (!HttpParser.isToken(chr)) { // Non-token characters are illegal in header names throwIOException(sm.getString("chunkedInputFilter.invalidTrailerHeaderName")); + } else if (trailingHeaders.getEnd() >= trailingHeaders.getLimit()) { + throwIOException(sm.getString("chunkedInputFilter.maxTrailer")); } else { trailingHeaders.append(chr); } From 00e40a0f98e5d85f74743d48d162c0c9be290e06 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 20:21:43 +0000 Subject: [PATCH 08/16] Move BadRequestException so it is visible to coyote package This makes the exception accessible to the ChunkedInputFilter which is step 1 in improving handling of bad requests while parsing parameters in chunked requests. --- .../{catalina/connector => coyote}/BadRequestException.java | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename java/org/apache/{catalina/connector => coyote}/BadRequestException.java (100%) diff --git a/java/org/apache/catalina/connector/BadRequestException.java b/java/org/apache/coyote/BadRequestException.java similarity index 100% rename from java/org/apache/catalina/connector/BadRequestException.java rename to java/org/apache/coyote/BadRequestException.java From 169625ac6570c50f43e34004637137e3bb7a55b4 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 14:23:06 -0600 Subject: [PATCH 09/16] Missed files in previous commit --- .../apache/catalina/connector/ClientAbortException.java | 2 ++ java/org/apache/catalina/connector/InputBuffer.java | 9 ++++----- java/org/apache/catalina/connector/Request.java | 1 + java/org/apache/catalina/core/ApplicationDispatcher.java | 2 +- java/org/apache/catalina/core/StandardWrapperValve.java | 2 +- java/org/apache/coyote/BadRequestException.java | 2 +- 6 files changed, 10 insertions(+), 8 deletions(-) diff --git a/java/org/apache/catalina/connector/ClientAbortException.java b/java/org/apache/catalina/connector/ClientAbortException.java index a3ba607366e3..34fd2b509639 100644 --- a/java/org/apache/catalina/connector/ClientAbortException.java +++ b/java/org/apache/catalina/connector/ClientAbortException.java @@ -16,6 +16,8 @@ */ package org.apache.catalina.connector; +import org.apache.coyote.BadRequestException; + /** * Extend IOException to identify it as being caused by an abort of a request by * a remote client. diff --git a/java/org/apache/catalina/connector/InputBuffer.java b/java/org/apache/catalina/connector/InputBuffer.java index bdcb0665a00b..0462a63a3d23 100644 --- a/java/org/apache/catalina/connector/InputBuffer.java +++ b/java/org/apache/catalina/connector/InputBuffer.java @@ -33,7 +33,7 @@ import org.apache.catalina.security.SecurityUtil; import org.apache.coyote.ActionCode; -import org.apache.coyote.Request; +import org.apache.coyote.BadRequestException; import org.apache.juli.logging.Log; import org.apache.juli.logging.LogFactory; import org.apache.tomcat.util.buf.B2CConverter; @@ -109,7 +109,7 @@ public class InputBuffer extends Reader /** * Associated Coyote request. */ - private Request coyoteRequest; + private org.apache.coyote.Request coyoteRequest; /** @@ -168,7 +168,7 @@ public InputBuffer(int size) { * * @param coyoteRequest Associated Coyote request */ - public void setRequest(Request coyoteRequest) { + public void setRequest(org.apache.coyote.Request coyoteRequest) { this.coyoteRequest = coyoteRequest; } @@ -216,8 +216,7 @@ public void close() throws IOException { public int available() { int available = availableInThisBuffer(); if (available == 0) { - coyoteRequest.action(ActionCode.AVAILABLE, - Boolean.valueOf(coyoteRequest.getReadListener() != null)); + coyoteRequest.action(ActionCode.AVAILABLE, Boolean.valueOf(coyoteRequest.getReadListener() != null)); available = (coyoteRequest.getAvailable() > 0) ? 1 : 0; } return available; diff --git a/java/org/apache/catalina/connector/Request.java b/java/org/apache/catalina/connector/Request.java index 9253d2b9741c..553a4ff98ed8 100644 --- a/java/org/apache/catalina/connector/Request.java +++ b/java/org/apache/catalina/connector/Request.java @@ -87,6 +87,7 @@ import org.apache.catalina.util.TLSUtil; import org.apache.catalina.util.URLEncoder; import org.apache.coyote.ActionCode; +import org.apache.coyote.BadRequestException; import org.apache.coyote.UpgradeToken; import org.apache.coyote.http11.upgrade.InternalHttpUpgradeHandler; import org.apache.juli.logging.Log; diff --git a/java/org/apache/catalina/core/ApplicationDispatcher.java b/java/org/apache/catalina/core/ApplicationDispatcher.java index 4374ed792742..252134cb8c30 100644 --- a/java/org/apache/catalina/core/ApplicationDispatcher.java +++ b/java/org/apache/catalina/core/ApplicationDispatcher.java @@ -41,11 +41,11 @@ import org.apache.catalina.Context; import org.apache.catalina.Globals; import org.apache.catalina.Wrapper; -import org.apache.catalina.connector.BadRequestException; import org.apache.catalina.connector.Request; import org.apache.catalina.connector.RequestFacade; import org.apache.catalina.connector.Response; import org.apache.catalina.connector.ResponseFacade; +import org.apache.coyote.BadRequestException; import org.apache.tomcat.util.ExceptionUtils; import org.apache.tomcat.util.res.StringManager; diff --git a/java/org/apache/catalina/core/StandardWrapperValve.java b/java/org/apache/catalina/core/StandardWrapperValve.java index 8920ae87890a..f66b67d8b30f 100644 --- a/java/org/apache/catalina/core/StandardWrapperValve.java +++ b/java/org/apache/catalina/core/StandardWrapperValve.java @@ -31,10 +31,10 @@ import org.apache.catalina.Context; import org.apache.catalina.Globals; import org.apache.catalina.LifecycleException; -import org.apache.catalina.connector.BadRequestException; import org.apache.catalina.connector.Request; import org.apache.catalina.connector.Response; import org.apache.catalina.valves.ValveBase; +import org.apache.coyote.BadRequestException; import org.apache.coyote.CloseNowException; import org.apache.tomcat.util.ExceptionUtils; import org.apache.tomcat.util.buf.MessageBytes; diff --git a/java/org/apache/coyote/BadRequestException.java b/java/org/apache/coyote/BadRequestException.java index 71a792d7fbd0..d02f1be6e6e6 100644 --- a/java/org/apache/coyote/BadRequestException.java +++ b/java/org/apache/coyote/BadRequestException.java @@ -14,7 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.catalina.connector; +package org.apache.coyote; import java.io.IOException; From 080bcfd673f50ae611016a260b1c48f63ebc7b2e Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 20:58:15 +0000 Subject: [PATCH 10/16] Fix back-port --- java/org/apache/catalina/connector/Request.java | 1 - 1 file changed, 1 deletion(-) diff --git a/java/org/apache/catalina/connector/Request.java b/java/org/apache/catalina/connector/Request.java index 553a4ff98ed8..9253d2b9741c 100644 --- a/java/org/apache/catalina/connector/Request.java +++ b/java/org/apache/catalina/connector/Request.java @@ -87,7 +87,6 @@ import org.apache.catalina.util.TLSUtil; import org.apache.catalina.util.URLEncoder; import org.apache.coyote.ActionCode; -import org.apache.coyote.BadRequestException; import org.apache.coyote.UpgradeToken; import org.apache.coyote.http11.upgrade.InternalHttpUpgradeHandler; import org.apache.juli.logging.Log; From 260d9a6327aabf7095e6b04218537905f86e6e68 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 20:27:22 +0000 Subject: [PATCH 11/16] Use more appropriate exception so a bad request triggers a 400 response --- .../http11/filters/ChunkedInputFilter.java | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java index 82aea66aa1da..22db959a5aaa 100644 --- a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java +++ b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java @@ -24,6 +24,7 @@ import java.util.Map; import java.util.Set; +import org.apache.coyote.BadRequestException; import org.apache.coyote.InputBuffer; import org.apache.coyote.Request; import org.apache.coyote.http11.Constants; @@ -163,7 +164,7 @@ public int doRead(ApplicationBufferHandler handler) throws IOException { if (remaining <= 0) { if (!parseChunkHeader()) { - throwIOException(sm.getString("chunkedInputFilter.invalidHeader")); + throwBadRequestException(sm.getString("chunkedInputFilter.invalidHeader")); } if (endChunk) { parseEndChunk(); @@ -175,7 +176,7 @@ public int doRead(ApplicationBufferHandler handler) throws IOException { if (readChunk == null || readChunk.position() >= readChunk.limit()) { if (readBytes() < 0) { - throwIOException(sm.getString("chunkedInputFilter.eos")); + throwEOFException(sm.getString("chunkedInputFilter.eos")); } } @@ -230,7 +231,7 @@ public long end() throws IOException { while ((read = doRead(this)) >= 0) { swallowed += read; if (maxSwallowSize > -1 && swallowed > maxSwallowSize) { - throwIOException(sm.getString("inputFilter.maxSwallow")); + throwBadRequestException(sm.getString("inputFilter.maxSwallow")); } } @@ -368,7 +369,7 @@ protected boolean parseChunkHeader() throws IOException { // validated. Currently it is simply ignored. extensionSize++; if (maxExtensionSize > -1 && extensionSize > maxExtensionSize) { - throwIOException(sm.getString("chunkedInputFilter.maxExtension")); + throwBadRequestException(sm.getString("chunkedInputFilter.maxExtension")); } } @@ -407,23 +408,23 @@ protected void parseCRLF(boolean tolerant) throws IOException { while (!eol) { if (readChunk == null || readChunk.position() >= readChunk.limit()) { if (readBytes() <= 0) { - throwIOException(sm.getString("chunkedInputFilter.invalidCrlfNoData")); + throwBadRequestException(sm.getString("chunkedInputFilter.invalidCrlfNoData")); } } byte chr = readChunk.get(readChunk.position()); if (chr == Constants.CR) { if (crfound) { - throwIOException(sm.getString("chunkedInputFilter.invalidCrlfCRCR")); + throwBadRequestException(sm.getString("chunkedInputFilter.invalidCrlfCRCR")); } crfound = true; } else if (chr == Constants.LF) { if (!tolerant && !crfound) { - throwIOException(sm.getString("chunkedInputFilter.invalidCrlfNoCR")); + throwBadRequestException(sm.getString("chunkedInputFilter.invalidCrlfNoCR")); } eol = true; } else { - throwIOException(sm.getString("chunkedInputFilter.invalidCrlf")); + throwBadRequestException(sm.getString("chunkedInputFilter.invalidCrlf")); } readChunk.position(readChunk.position() + 1); @@ -500,9 +501,9 @@ private boolean parseHeader() throws IOException { colon = true; } else if (!HttpParser.isToken(chr)) { // Non-token characters are illegal in header names - throwIOException(sm.getString("chunkedInputFilter.invalidTrailerHeaderName")); + throwBadRequestException(sm.getString("chunkedInputFilter.invalidTrailerHeaderName")); } else if (trailingHeaders.getEnd() >= trailingHeaders.getLimit()) { - throwIOException(sm.getString("chunkedInputFilter.maxTrailer")); + throwBadRequestException(sm.getString("chunkedInputFilter.maxTrailer")); } else { trailingHeaders.append(chr); } @@ -541,7 +542,7 @@ private boolean parseHeader() throws IOException { // limit placed on trailing header size int newlimit = trailingHeaders.getLimit() -1; if (trailingHeaders.getEnd() > newlimit) { - throwIOException(sm.getString("chunkedInputFilter.maxTrailer")); + throwBadRequestException(sm.getString("chunkedInputFilter.maxTrailer")); } trailingHeaders.setLimit(newlimit); } else { @@ -617,9 +618,9 @@ private boolean parseHeader() throws IOException { } - private void throwIOException(String msg) throws IOException { + private void throwBadRequestException(String msg) throws IOException { error = true; - throw new IOException(msg); + throw new BadRequestException(msg); } From 94e3d9026bcc3545cb48cbb85a37d101d49b565b Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Thu, 9 Nov 2023 08:46:00 +0000 Subject: [PATCH 12/16] Better message if size limit is reached reading trailer header value --- java/org/apache/coyote/http11/filters/ChunkedInputFilter.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java index 22db959a5aaa..b93a2a2971b7 100644 --- a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java +++ b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java @@ -567,6 +567,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()) { + throwBadRequestException(sm.getString("chunkedInputFilter.maxTrailer")); } else if (chr == Constants.SP || chr == Constants.HT) { trailingHeaders.append(chr); } else { @@ -592,6 +594,8 @@ private boolean parseHeader() throws IOException { chr = readChunk.get(readChunk.position()); 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 From ed83454f1065e3e687d715f3368178dc4b3d8b45 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 16 Aug 2023 08:57:31 -0600 Subject: [PATCH 13/16] Use application provided status code for error page if present --- .../catalina/core/StandardHostValve.java | 13 ++-- .../catalina/core/TestStandardHostValve.java | 72 ++++++++++++++----- webapps/docs/changelog.xml | 6 ++ 3 files changed, 68 insertions(+), 23 deletions(-) diff --git a/java/org/apache/catalina/core/StandardHostValve.java b/java/org/apache/catalina/core/StandardHostValve.java index 2c7004ecdc62..55aa854e1ad1 100644 --- a/java/org/apache/catalina/core/StandardHostValve.java +++ b/java/org/apache/catalina/core/StandardHostValve.java @@ -317,11 +317,14 @@ 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); + } // The response is an error response.setError(); diff --git a/test/org/apache/catalina/core/TestStandardHostValve.java b/test/org/apache/catalina/core/TestStandardHostValve.java index 9e6927c3b88d..db4b7ccc0965 100644 --- a/test/org/apache/catalina/core/TestStandardHostValve.java +++ b/test/org/apache/catalina/core/TestStandardHostValve.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.List; +import jakarta.servlet.RequestDispatcher; import jakarta.servlet.ServletException; import jakarta.servlet.ServletRequestEvent; import jakarta.servlet.ServletRequestListener; @@ -40,7 +41,32 @@ public class TestStandardHostValve extends TomcatBaseTest { @Test - public void testErrorPageHandling() throws Exception { + public void testErrorPageHandling400() throws Exception { + doTestErrorPageHandling(400, "", "/400"); + } + + + @Test + public void testErrorPageHandling400WithException() throws Exception { + doTestErrorPageHandling(400, "java.lang.IllegalStateException", "/400"); + } + + + @Test + public void testErrorPageHandling500() throws Exception { + doTestErrorPageHandling(500, "", "/500"); + } + + + @Test + public void testErrorPageHandlingDefault() throws Exception { + doTestErrorPageHandling(501, "", "/default"); + } + + + private void doTestErrorPageHandling(int error, String exception, String report) + throws Exception { + // Set up a container Tomcat tomcat = getTomcatInstance(); @@ -55,6 +81,12 @@ public void testErrorPageHandling() throws Exception { Tomcat.addServlet(ctx, "report", new ReportServlet()); ctx.addServletMappingDecoded("/report/*", "report"); + // Add the handling for 400 responses + ErrorPage errorPage400 = new ErrorPage(); + errorPage400.setErrorCode(Response.SC_BAD_REQUEST); + errorPage400.setLocation("/report/400"); + ctx.addErrorPage(errorPage400); + // And the handling for 500 responses ErrorPage errorPage500 = new ErrorPage(); errorPage500.setErrorCode(Response.SC_INTERNAL_SERVER_ERROR); @@ -68,8 +100,13 @@ public void testErrorPageHandling() throws Exception { tomcat.start(); - doTestErrorPageHandling(500, "/500"); - doTestErrorPageHandling(501, "/default"); + // Request a page that triggers an error + ByteChunk bc = new ByteChunk(); + int rc = getUrl("http://localhost:" + getPort() + "/error?errorCode=" + error + "&exception=" + exception, + bc, null); + + Assert.assertEquals(error, rc); + Assert.assertEquals(report, bc.toString()); } @@ -131,19 +168,6 @@ public void requestInitialized(ServletRequestEvent sre) { } - private void doTestErrorPageHandling(int error, String report) - throws Exception { - - // Request a page that triggers an error - ByteChunk bc = new ByteChunk(); - int rc = getUrl("http://localhost:" + getPort() + - "/error?errorCode=" + error, bc, null); - - Assert.assertEquals(error, rc); - Assert.assertEquals(report, bc.toString()); - } - - @Test public void testIncompleteResponse() throws Exception { // Set up a container @@ -160,13 +184,13 @@ public void testIncompleteResponse() throws Exception { Tomcat.addServlet(ctx, "report", new ReportServlet()); ctx.addServletMappingDecoded("/report/*", "report"); - // And the handling for 500 responses + // Add the handling for 500 responses ErrorPage errorPage500 = new ErrorPage(); errorPage500.setErrorCode(Response.SC_INTERNAL_SERVER_ERROR); errorPage500.setLocation("/report/500"); ctx.addErrorPage(errorPage500); - // And the default error handling + // Add the default error handling ErrorPage errorPageDefault = new ErrorPage(); errorPageDefault.setLocation("/report/default"); ctx.addErrorPage(errorPageDefault); @@ -195,6 +219,18 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { int error = Integer.parseInt(req.getParameter("errorCode")); resp.sendError(error); + + Throwable t = null; + String exception = req.getParameter("exception"); + if (exception != null && exception.length() > 0) { + try { + t = (Throwable) Class.forName(exception).getConstructor().newInstance(); + } catch (ReflectiveOperationException e) { + // Should never happen but in case it does... + t = new IllegalArgumentException(); + } + req.setAttribute(RequestDispatcher.ERROR_EXCEPTION, t); + } } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 6f184f2ddfb5..3708876aa34f 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -160,6 +160,12 @@ request triggers always error handling, regardless of whether the application swallows the exception. (markt) + + If an application or library sets both a non-500 error code and the + jakarta.servlet.error.exception request attribute, use the + provided error code during error page processing rather than assuming an + error code of 500. (markt) + From 23eb398bbf1b524b97fc903abd961f7c6f4426b6 Mon Sep 17 00:00:00 2001 From: Cesar Hernandez Date: Thu, 7 Dec 2023 20:42:48 -0600 Subject: [PATCH 14/16] fix Http2TestBase --- test/org/apache/coyote/http2/Http2TestBase.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/org/apache/coyote/http2/Http2TestBase.java b/test/org/apache/coyote/http2/Http2TestBase.java index 61b1e419bca2..92c2f8024cd7 100644 --- a/test/org/apache/coyote/http2/Http2TestBase.java +++ b/test/org/apache/coyote/http2/Http2TestBase.java @@ -149,6 +149,11 @@ protected void http2Connect(boolean tls) throws Exception { protected void validateHttp2InitialResponse() throws Exception { + validateHttp2InitialResponse(200); + } + + protected void validateHttp2InitialResponse(long maxConcurrentStreams) throws Exception { + // - 101 response acts as acknowledgement of the HTTP2-Settings header // Need to read 5 frames // - settings (server settings - must be first) @@ -598,6 +603,11 @@ protected void enableHttp2(boolean tls) { } protected void enableHttp2(long maxConcurrentStreams, boolean tls) { + enableHttp2(maxConcurrentStreams, tls, 10000, 10000, 25000, 5000, 5000); + } + + protected void enableHttp2(long maxConcurrentStreams, boolean tls, long readTimeout, long writeTimeout, + long keepAliveTimeout, long streamReadTimout, long streamWriteTimeout) { Tomcat tomcat = getTomcatInstance(); Connector connector = tomcat.getConnector(); Assert.assertTrue(connector.setProperty("useAsyncIO", Boolean.toString(useAsyncIO))); From 0edcdbe94359da7e4fcc46c85f29989f046ac134 Mon Sep 17 00:00:00 2001 From: Cesar Hernandez Date: Fri, 8 Dec 2023 11:33:36 -0600 Subject: [PATCH 15/16] commenting out test from 700d7d9 --- test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java index 6f4e91a92647..d9f555e63e36 100644 --- a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java +++ b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java @@ -168,8 +168,8 @@ private void doTestUpgradeWithRequestBody(boolean usePost, boolean useReader, bo } } - - @Test + //Disabling this tests as this was introduced in 700d7d9 and it's out of the scope for CVE-2023-46589 patching. + //@Test public void testActiveConnectionCountAndClientTimeout() throws Exception { enableHttp2(2, false, 10000, 10000, 4000, 2000, 2000); From 8d9191310856917d91b772a5e9ef9633a052ff7e Mon Sep 17 00:00:00 2001 From: Cesar Hernandez Date: Fri, 8 Dec 2023 11:40:17 -0600 Subject: [PATCH 16/16] fix TestCharsetCache --- java/org/apache/tomcat/util/buf/CharsetCache.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/java/org/apache/tomcat/util/buf/CharsetCache.java b/java/org/apache/tomcat/util/buf/CharsetCache.java index 4474bee3c914..b26033bb1fb1 100644 --- a/java/org/apache/tomcat/util/buf/CharsetCache.java +++ b/java/org/apache/tomcat/util/buf/CharsetCache.java @@ -153,7 +153,9 @@ public class CharsetCache { // Added from OpenJDK 15 ea24 "iso8859_16", // Added from HPE JVM 1.8.0.17-hp-ux - "cp1051", "cp1386", "cshproman8", "hp-roman8", "ibm-1051", "r8", "roman8", "roman9" + "cp1051", "cp1386", "cshproman8", "hp-roman8", "ibm-1051", "r8", "roman8", "roman9", + // Added from OpenJDK 21 ea18 + "gb18030-2022" // If you add and entry to this list, ensure you run // TestCharsetUtil#testIsAcsiiSupersetAll() };