Fix 66591 - Make sure every response includes a Send Headers packet

apache · May 3, 2023 · 2214c80 · 2214c80
1 parent deab34e
commit 2214c80
Show file tree

Hide file tree

Showing 3 changed files with 113 additions and 50 deletions.
diff --git a/java/org/apache/coyote/ajp/AjpProcessor.java b/java/org/apache/coyote/ajp/AjpProcessor.java
@@ -1058,59 +1058,63 @@ protected final void prepareResponse() throws IOException {
         responseMsgPos = -1;
 
         int numHeaders = headers.size();
-        for (int i = 0; i < numHeaders; i++) {
-            if (i == 0) {
-                // Write AJP message header
-                responseMessage.reset();
-                responseMessage.appendByte(Constants.JK_AJP13_SEND_HEADERS);
-
-                // Write HTTP response line
-                responseMessage.appendInt(statusCode);
-                if (sendReasonPhrase) {
-                    String message = null;
-                    if (org.apache.coyote.Constants.USE_CUSTOM_STATUS_MSG_IN_HEADER &&
-                            HttpMessages.isSafeInHttpHeader(response.getMessage())) {
-                        message = response.getMessage();
-                    }
-                    if (message == null) {
-                        message = HttpMessages.getInstance(response.getLocale()).getMessage(response.getStatus());
-                    }
-                    if (message == null) {
-                        // mod_jk + httpd 2.x fails with a null status message - bug 45026
-                        message = Integer.toString(response.getStatus());
-                    }
-                    tmpMB.setString(message);
-                } else {
-                    // Reason phrase is optional but mod_jk + httpd 2.x fails with a null
-                    // reason phrase - bug 45026
-                    tmpMB.setString(Integer.toString(response.getStatus()));
+        boolean needAjpMessageHeader = true;
+        while (needAjpMessageHeader) {
+            // Write AJP message header
+            responseMessage.reset();
+            responseMessage.appendByte(Constants.JK_AJP13_SEND_HEADERS);
+
+            // Write HTTP response line
+            responseMessage.appendInt(statusCode);
+            if (sendReasonPhrase) {
+                String message = null;
+                if (org.apache.coyote.Constants.USE_CUSTOM_STATUS_MSG_IN_HEADER &&
+                        HttpMessages.isSafeInHttpHeader(response.getMessage())) {
+                    message = response.getMessage();
                 }
-                responseMessage.appendBytes(tmpMB);
-
-                // Start headers
-                responseMessage.appendInt(numHeaders);
+                if (message == null) {
+                    message = HttpMessages.getInstance(response.getLocale()).getMessage(response.getStatus());
+                }
+                if (message == null) {
+                    // mod_jk + httpd 2.x fails with a null status message - bug 45026
+                    message = Integer.toString(response.getStatus());
+                }
+                tmpMB.setString(message);
+            } else {
+                // Reason phrase is optional but mod_jk + httpd 2.x fails with a null
+                // reason phrase - bug 45026
+                tmpMB.setString(Integer.toString(response.getStatus()));
             }
+            responseMessage.appendBytes(tmpMB);
 
-            try {
-                // Write headers
-                MessageBytes hN = headers.getName(i);
-                int hC = Constants.getResponseAjpIndex(hN.toString());
-                if (hC > 0) {
-                    responseMessage.appendInt(hC);
-                } else {
-                    responseMessage.appendBytes(hN);
+            // Start headers
+            responseMessage.appendInt(numHeaders);
+
+            needAjpMessageHeader = false;
+
+            for (int i = 0; i < numHeaders; i++) {
+                try {
+                    // Write headers
+                    MessageBytes hN = headers.getName(i);
+                    int hC = Constants.getResponseAjpIndex(hN.toString());
+                    if (hC > 0) {
+                        responseMessage.appendInt(hC);
+                    } else {
+                        responseMessage.appendBytes(hN);
+                    }
+                    MessageBytes hV = headers.getValue(i);
+                    responseMessage.appendBytes(hV);
+                } catch (IllegalArgumentException iae) {
+                    // Log the problematic header
+                    log.warn(sm.getString("ajpprocessor.response.invalidHeader", headers.getName(i), headers.getValue(i)),
+                            iae);
+                    // Remove the problematic header
+                    headers.removeHeader(i);
+                    numHeaders--;
+                    // Restart writing of AJP message
+                    needAjpMessageHeader = true;
+                    break;
                 }
-                MessageBytes hV = headers.getValue(i);
-                responseMessage.appendBytes(hV);
-            } catch (IllegalArgumentException iae) {
-                // Log the problematic header
-                log.warn(sm.getString("ajpprocessor.response.invalidHeader", headers.getName(i), headers.getValue(i)),
-                        iae);
-                // Remove the problematic header
-                headers.removeHeader(i);
-                numHeaders--;
-                // Reset loop and start again
-                i = -1;
             }
         }
 

diff --git a/test/org/apache/coyote/ajp/TestAbstractAjpProcessor.java b/test/org/apache/coyote/ajp/TestAbstractAjpProcessor.java
@@ -885,6 +885,60 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws Se
     }
 
 
+    /*
+     * https://bz.apache.org/bugzilla/show_bug.cgi?id=66591
+     */
+    @Test
+    public void testNoHeaders() throws Exception {
+
+        Tomcat tomcat = getTomcatInstance();
+
+        // No file system docBase required
+        Context ctx = tomcat.addContext("", null);
+
+        Tomcat.addServlet(ctx, "bug66591", new NoHeadersServlet());
+        ctx.addServletMappingDecoded("/", "bug66591");
+
+        tomcat.start();
+
+        SimpleAjpClient ajpClient = new SimpleAjpClient();
+        ajpClient.setPort(getPort());
+        ajpClient.connect();
+
+        validateCpong(ajpClient.cping());
+
+        TesterAjpMessage forwardMessage = ajpClient.createForwardMessage();
+        forwardMessage.end();
+
+        TesterAjpMessage responseHeaderMessage = ajpClient.sendMessage(forwardMessage, null);
+
+        // Expect 3 messages: headers, body chunk, end
+        Map<String, List<String>> responseHeaders = validateResponseHeaders(responseHeaderMessage, 200, "200");
+        Assert.assertTrue(responseHeaders.isEmpty());
+
+        String body = extractResponseBody(ajpClient.readMessage());
+        Assert.assertTrue(body.isEmpty());
+
+        validateResponseEnd(ajpClient.readMessage(), true);
+
+        // Double check the connection is still open
+        validateCpong(ajpClient.cping());
+
+        ajpClient.disconnect();
+    }
+
+
+    private static class NoHeadersServlet extends HttpServlet {
+
+        private static final long serialVersionUID = 1L;
+
+        @Override
+        protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+            resp.flushBuffer();
+        }
+    }
+
+
     /**
      * Process response header packet and checks the status. Any other data is ignored.
      */
@@ -950,7 +1004,7 @@ private String extractResponseBody(TesterAjpMessage message) throws Exception {
         Assert.assertEquals(0x03, message.readByte());
 
         int len = message.readInt();
-        Assert.assertTrue(len > 0);
+        Assert.assertTrue(len >= 0);
         return message.readString(len);
     }
 

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
@@ -163,6 +163,11 @@
         and <code>allowHostHeaderMismatch</code> as they have been removed in
         Tomcat 11 onwards. (markt)
       </update>
+      <fix>
+        <bug>66591</bug>: Fix a regression introduced in the fix for
+        <bug>66512</bug> that meant that an AJP Send Headers was not sent for
+        responses where no HTTP headers were set. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">