Providing the error does not require the immediate closure of the connection, pass all errors (invalid requests etc.) that occur before the request processing pipeline is reached to the standard error handling mechanism so that the application error handling or the ErrorReportVlave can handle it.

markt-asf · markt-asf · commit d30d3e3a111c · 2019-02-07T22:36:52.000Z
git-svn-id: https://svn.apache.org/repos/asf/tomcat/tc8.5.x/trunk@1853174 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/java/org/apache/catalina/connector/CoyoteAdapter.java b/java/org/apache/catalina/connector/CoyoteAdapter.java
@@ -393,14 +393,25 @@ public void service(org.apache.coyote.Request req, org.apache.coyote.Response re
                 // Log only if processing was invoked.
                 // If postParseRequest() failed, it has already logged it.
                 Context context = request.getContext();
+                Host host = request.getHost();
                 // If the context is null, it is likely that the endpoint was
                 // shutdown, this connection closed and the request recycled in
                 // a different thread. That thread will have updated the access
                 // log so it is OK not to update the access log here in that
                 // case.
+                // The other possibility is that an error occurred early in
+                // processing and the request could not be mapped to a Context.
+                // Log via the host or engine in that case.
+                long time = System.currentTimeMillis() - req.getStartTime();
                 if (context != null) {
-                    context.logAccess(request, response,
-                            System.currentTimeMillis() - req.getStartTime(), false);
+                    context.logAccess(request, response, time, false);
+                } else if (response.isError()) {
+                    if (host != null) {
+                        host.logAccess(request, response, time, false);
+                    } else {
+                        connector.getService().getContainer().logAccess(
+                                request, response, time, false);
+                    }
                 }
             }
 
diff --git a/java/org/apache/coyote/AbstractProcessor.java b/java/org/apache/coyote/AbstractProcessor.java
@@ -100,6 +100,7 @@ protected AbstractProcessor(AbstractEndpoint<?> endpoint, Request coyoteRequest,
      * @param t The error which occurred
      */
     protected void setErrorState(ErrorState errorState, Throwable t) {
+        response.setError();
         boolean blockIo = this.errorState.isIoAllowed() && !errorState.isIoAllowed();
         this.errorState = this.errorState.getMostSevere(errorState);
         // Don't change the status code for IOException since that is almost
diff --git a/java/org/apache/coyote/Response.java b/java/org/apache/coyote/Response.java
@@ -224,6 +224,10 @@ public int getStatus() {
      * @param status The status value to set
      */
     public void setStatus(int status) {
+        if (this.status > 399) {
+            // Don't overwrite first recorded error status
+            return;
+        }
         this.status = status;
     }
 
diff --git a/java/org/apache/coyote/ajp/AjpProcessor.java b/java/org/apache/coyote/ajp/AjpProcessor.java
@@ -446,10 +446,9 @@ public SocketState service(SocketWrapperBase<?> socket) throws IOException {
                 // 400 - Bad Request
                 response.setStatus(400);
                 setErrorState(ErrorState.CLOSE_CLEAN, t);
-                getAdapter().log(request, response, 0);
             }
 
-            if (!getErrorState().isError()) {
+            if (getErrorState().isIoAllowed()) {
                 // Setting up filters, and parse some request headers
                 rp.setStage(org.apache.coyote.Constants.STAGE_PREPARE);
                 try {
@@ -460,20 +459,18 @@ public SocketState service(SocketWrapperBase<?> socket) throws IOException {
                     // 500 - Internal Server Error
                     response.setStatus(500);
                     setErrorState(ErrorState.CLOSE_CLEAN, t);
-                    getAdapter().log(request, response, 0);
                 }
             }
 
-            if (!getErrorState().isError() && !cping && endpoint.isPaused()) {
+            if (getErrorState().isIoAllowed() && !cping && endpoint.isPaused()) {
                 // 503 - Service unavailable
                 response.setStatus(503);
                 setErrorState(ErrorState.CLOSE_CLEAN, null);
-                getAdapter().log(request, response, 0);
             }
             cping = false;
 
             // Process the request in the adapter
-            if (!getErrorState().isError()) {
+            if (getErrorState().isIoAllowed()) {
                 try {
                     rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
                     getAdapter().service(request, response);
@@ -933,7 +930,7 @@ private void prepareRequest() {
         MessageBytes valueMB = request.getMimeHeaders().getValue("host");
         parseHost(valueMB);
 
-        if (getErrorState().isError()) {
+        if (!getErrorState().isIoAllowed()) {
             getAdapter().log(request, response, 0);
         }
     }
diff --git a/java/org/apache/coyote/http11/Http11Processor.java b/java/org/apache/coyote/http11/Http11Processor.java
@@ -733,7 +733,6 @@ public SocketState service(SocketWrapperBase<?> socketWrapper)
                 // 400 - Bad Request
                 response.setStatus(400);
                 setErrorState(ErrorState.CLOSE_CLEAN, t);
-                getAdapter().log(request, response, 0);
             }
 
             // Has an upgrade been requested?
@@ -769,7 +768,7 @@ public SocketState service(SocketWrapperBase<?> socketWrapper)
                 }
             }
 
-            if (!getErrorState().isError()) {
+            if (getErrorState().isIoAllowed()) {
                 // Setting up filters, and parse some request headers
                 rp.setStage(org.apache.coyote.Constants.STAGE_PREPARE);
                 try {
@@ -782,7 +781,6 @@ public SocketState service(SocketWrapperBase<?> socketWrapper)
                     // 500 - Internal Server Error
                     response.setStatus(500);
                     setErrorState(ErrorState.CLOSE_CLEAN, t);
-                    getAdapter().log(request, response, 0);
                 }
             }
 
@@ -794,7 +792,7 @@ public SocketState service(SocketWrapperBase<?> socketWrapper)
             }
 
             // Process the request in the adapter
-            if (!getErrorState().isError()) {
+            if (getErrorState().isIoAllowed()) {
                 try {
                     rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
                     getAdapter().service(request, response);
@@ -920,7 +918,6 @@ private boolean handleIncompleteRequestLineRead() {
                 // Partially processed the request so need to respond
                 response.setStatus(503);
                 setErrorState(ErrorState.CLOSE_CLEAN, null);
-                getAdapter().log(request, response, 0);
                 return false;
             } else {
                 // Need to keep processor associated with socket
@@ -1210,7 +1207,7 @@ private void prepareRequest() {
             contentDelimitation = true;
         }
 
-        if (getErrorState().isError()) {
+        if (!getErrorState().isIoAllowed()) {
             getAdapter().log(request, response, 0);
         }
     }
diff --git a/test/org/apache/coyote/ajp/TestAbstractAjpProcessor.java b/test/org/apache/coyote/ajp/TestAbstractAjpProcessor.java
@@ -512,8 +512,8 @@ public void testSecret() throws Exception {
         TesterAjpMessage responseHeaders = ajpClient.sendMessage(forwardMessage);
         // Expect 3 packets: headers, body, end
         validateResponseHeaders(responseHeaders, 403, "403");
-        //TesterAjpMessage responseBody = ajpClient.readMessage();
-        //validateResponseBody(responseBody, HelloWorldServlet.RESPONSE_TEXT);
+        TesterAjpMessage responseBody = ajpClient.readMessage();
+        validateResponseBody(responseBody, "<p><b>Type</b> Status Report</p>");
         validateResponseEnd(ajpClient.readMessage(), false);
 
         ajpClient.connect();
@@ -526,8 +526,8 @@ public void testSecret() throws Exception {
         responseHeaders = ajpClient.sendMessage(forwardMessage);
         // Expect 3 packets: headers, body, end
         validateResponseHeaders(responseHeaders, 403, "403");
-        //responseBody = ajpClient.readMessage();
-        //validateResponseBody(responseBody, HelloWorldServlet.RESPONSE_TEXT);
+        responseBody = ajpClient.readMessage();
+        validateResponseBody(responseBody, "<p><b>Type</b> Status Report</p>");
         validateResponseEnd(ajpClient.readMessage(), false);
 
         ajpClient.connect();
@@ -540,7 +540,7 @@ public void testSecret() throws Exception {
         responseHeaders = ajpClient.sendMessage(forwardMessage);
         // Expect 3 packets: headers, body, end
         validateResponseHeaders(responseHeaders, 200, "200");
-        TesterAjpMessage responseBody = ajpClient.readMessage();
+        responseBody = ajpClient.readMessage();
         validateResponseBody(responseBody, HelloWorldServlet.RESPONSE_TEXT);
         validateResponseEnd(ajpClient.readMessage(), true);
 
@@ -641,7 +641,9 @@ public void doTestPost(boolean multipleCL, int expectedStatus,
             // Double check the connection is still open
             validateCpong(ajpClient.cping());
         } else {
-            // Expect 2 messages: headers, end for an invalid request
+            // Expect 3 messages: headers, error report body, end for an invalid request
+            TesterAjpMessage responseBody = ajpClient.readMessage();
+            validateResponseBody(responseBody, "<p><b>Type</b> Status Report</p>");
             validateResponseEnd(ajpClient.readMessage(), false);
         }
 
diff --git a/test/org/apache/tomcat/util/http/TestMimeHeadersIntegration.java b/test/org/apache/tomcat/util/http/TestMimeHeadersIntegration.java
@@ -94,7 +94,7 @@ private void runHeadersTest(final boolean successExpected,
                     client.getResponseLine() != null && client.isResponse200());
             Assert.assertEquals("OK", client.getResponseBody());
         } else {
-            alv.validateAccessLog(1, 400, 0, 0);
+            alv.validateAccessLog(1, 400, 0, 3000);
             // Connection aborted or response 400
             Assert.assertTrue("Response line is: " + client.getResponseLine(),
                     client.getResponseLine() == null || client.isResponse400());
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
@@ -70,6 +70,11 @@
         Pass 400 errors triggered by invalid request targets to the container
         error handling to generate the response body. (markt)
       </add>
+      <add>
+        Pass errors triggered by invalid requests or unavailable services to the
+        application provided error handling and/or the container provided error
+        handling (<code>ErrorReportValve</code>) as appropriate. (markt)
+      </add>
     </changelog>
   </subsection>
   <subsection name="WebSocket">

Original file line number	Diff line number	Diff line change
`@@ -733,7 +733,6 @@ public SocketState service(SocketWrapperBase<?> socketWrapper)`
`733`	`733`	`// 400 - Bad Request`
`734`	`734`	`response.setStatus(400);`
`735`	`735`	`setErrorState(ErrorState.CLOSE_CLEAN, t);`
`736`		`- getAdapter().log(request, response, 0);`
`737`	`736`	`}`
`738`	`737`
`739`	`738`	`// Has an upgrade been requested?`
`@@ -769,7 +768,7 @@ public SocketState service(SocketWrapperBase<?> socketWrapper)`
`769`	`768`	`}`
`770`	`769`	`}`
`771`	`770`
`772`		`- if (!getErrorState().isError()) {`
	`771`	`+ if (getErrorState().isIoAllowed()) {`
`773`	`772`	`// Setting up filters, and parse some request headers`
`774`	`773`	`rp.setStage(org.apache.coyote.Constants.STAGE_PREPARE);`
`775`	`774`	`try {`
`@@ -782,7 +781,6 @@ public SocketState service(SocketWrapperBase<?> socketWrapper)`
`782`	`781`	`// 500 - Internal Server Error`
`783`	`782`	`response.setStatus(500);`
`784`	`783`	`setErrorState(ErrorState.CLOSE_CLEAN, t);`
`785`		`- getAdapter().log(request, response, 0);`
`786`	`784`	`}`
`787`	`785`	`}`
`788`	`786`
`@@ -794,7 +792,7 @@ public SocketState service(SocketWrapperBase<?> socketWrapper)`
`794`	`792`	`}`
`795`	`793`
`796`	`794`	`// Process the request in the adapter`
`797`		`- if (!getErrorState().isError()) {`
	`795`	`+ if (getErrorState().isIoAllowed()) {`
`798`	`796`	`try {`
`799`	`797`	`rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);`
`800`	`798`	`getAdapter().service(request, response);`
`@@ -920,7 +918,6 @@ private boolean handleIncompleteRequestLineRead() {`
`920`	`918`	`// Partially processed the request so need to respond`
`921`	`919`	`response.setStatus(503);`
`922`	`920`	`setErrorState(ErrorState.CLOSE_CLEAN, null);`
`923`		`- getAdapter().log(request, response, 0);`
`924`	`921`	`return false;`
`925`	`922`	`} else {`
`926`	`923`	`// Need to keep processor associated with socket`
`@@ -1210,7 +1207,7 @@ private void prepareRequest() {`
`1210`	`1207`	`contentDelimitation = true;`
`1211`	`1208`	`}`
`1212`	`1209`
`1213`		`- if (getErrorState().isError()) {`
	`1210`	`+ if (!getErrorState().isIoAllowed()) {`
`1214`	`1211`	`getAdapter().log(request, response, 0);`
`1215`	`1212`	`}`
`1216`	`1213`	`}`