Improve processing of chuck size from chunked headers. Avoid overflow and use a bit shift instead of a multiplication as it is marginally faster.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1578337 13f79535-47bb-0310-9956-ffa450edef68

Author: markt-asf

java/org/apache/coyote/http11/filters/ChunkedInputFilter.java

@@ -319,7 +319,7 @@ protected boolean parseChunkHeader()
 
         int result = 0;
         boolean eol = false;
-        boolean readDigit = false;
+        int readDigit = 0;
         boolean extension = false;
 
         while (!eol) {
@@ -341,10 +341,9 @@ protected boolean parseChunkHeader()
             } else if (!extension) {
                 //don't read data after the trailer
                 int charValue = HexUtils.getDec(buf[pos]);
-                if (charValue != -1) {
-                    readDigit = true;
-                    result *= 16;
-                    result += charValue;
+                if (charValue != -1 && readDigit < 8) {
+                    readDigit++;
+                    result = (result << 4) | charValue;
                 } else {
                     //we shouldn't allow invalid, non hex characters
                     //in the chunked header
@@ -367,7 +366,7 @@ protected boolean parseChunkHeader()
 
         }
 
-        if (!readDigit)
+        if (readDigit == 0 || result < 0)
             return false;
 
         if (result == 0)

test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java

@@ -105,7 +105,7 @@ private void doTestChunkingCRLF(boolean chunkHeaderUsesCRLF,
         Context ctx =
             tomcat.addContext("", System.getProperty("java.io.tmpdir"));
 
-        EchoHeaderServlet servlet = new EchoHeaderServlet();
+        EchoHeaderServlet servlet = new EchoHeaderServlet(expectPass);
         Tomcat.addServlet(ctx, "servlet", servlet);
         ctx.addServletMapping("/", "servlet");
 
@@ -169,7 +169,7 @@ public void testTrailingHeadersSizeLimit() throws Exception {
         Context ctx =
             tomcat.addContext("", System.getProperty("java.io.tmpdir"));
 
-        Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet());
+        Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet(false));
         ctx.addServletMapping("/", "servlet");
 
         // Limit the size of the trailing header
@@ -233,7 +233,7 @@ private void doTestExtensionSizeLimit(int len, boolean ok) throws Exception {
         Context ctx =
             tomcat.addContext("", System.getProperty("java.io.tmpdir"));
 
-        Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet());
+        Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet(ok));
         ctx.addServletMapping("/", "servlet");
 
         tomcat.start();
@@ -282,7 +282,7 @@ public void testNoTrailingHeaders() throws Exception {
         Context ctx =
             tomcat.addContext("", System.getProperty("java.io.tmpdir"));
 
-        Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet());
+        Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet(true));
         ctx.addServletMapping("/", "servlet");
 
         tomcat.start();
@@ -311,11 +311,136 @@ public void testNoTrailingHeaders() throws Exception {
         assertEquals("nullnull7nullnull", client.getResponseBody());
     }
 
+    @Test
+    public void testChunkSizeZero() throws Exception {
+        doTestChunkSize(true, true, "", 10, 0);
+    }
+
+    @Test
+    public void testChunkSizeAbsent() throws Exception {
+        doTestChunkSize(false, false, SimpleHttpClient.CRLF, 10, 0);
+    }
+
+    @Test
+    public void testChunkSizeTwentyFive() throws Exception {
+        doTestChunkSize(true, true, "19" + SimpleHttpClient.CRLF
+                + "Hello World!Hello World!!" + SimpleHttpClient.CRLF, 40, 25);
+    }
+
+    @Test
+    public void testChunkSizeEightDigit() throws Exception {
+        doTestChunkSize(true, true, "0000000C" + SimpleHttpClient.CRLF
+                + "Hello World!" + SimpleHttpClient.CRLF, 20, 12);
+    }
+
+    @Test
+    public void testChunkSizeNineDigit() throws Exception {
+        doTestChunkSize(false, false, "00000000C" + SimpleHttpClient.CRLF
+                + "Hello World!" + SimpleHttpClient.CRLF, 20, 12);
+    }
+
+    @Test
+    public void testChunkSizeLong() throws Exception {
+        doTestChunkSize(true, false, "7fFFffFF" + SimpleHttpClient.CRLF
+                + "Hello World!" + SimpleHttpClient.CRLF, 10, 10);
+    }
+
+    @Test
+    public void testChunkSizeIntegerMinValue() throws Exception {
+        doTestChunkSize(false, false, "80000000" + SimpleHttpClient.CRLF
+                + "Hello World!" + SimpleHttpClient.CRLF, 10, 10);
+    }
+
+    @Test
+    public void testChunkSizeMinusOne() throws Exception {
+        doTestChunkSize(false, false, "ffffffff" + SimpleHttpClient.CRLF
+                + "Hello World!" + SimpleHttpClient.CRLF, 10, 10);
+    }
+
+    /**
+     * @param expectPass
+     *            If the servlet is expected to process the request
+     * @param expectReadWholeBody
+     *            If the servlet is expected to fully read the body and reliably
+     *            deliver a response
+     * @param chunks
+     *            Text of chunks
+     * @param readLimit
+     *            Do not read more than this many bytes
+     * @param expectReadCount
+     *            Expected count of read bytes
+     * @throws Exception
+     *             Unexpected
+     */
+    private void doTestChunkSize(boolean expectPass,
+            boolean expectReadWholeBody, String chunks, int readLimit,
+            int expectReadCount) throws Exception {
+        // Setup Tomcat instance
+        Tomcat tomcat = getTomcatInstance();
+
+        // Must have a real docBase - just use temp
+        Context ctx = tomcat.addContext("",
+                System.getProperty("java.io.tmpdir"));
+
+        BodyReadServlet servlet = new BodyReadServlet(expectPass, readLimit);
+        Tomcat.addServlet(ctx, "servlet", servlet);
+        ctx.addServletMapping("/", "servlet");
+
+        tomcat.start();
+
+        String request = "POST /echo-params.jsp HTTP/1.1"
+                + SimpleHttpClient.CRLF + "Host: any" + SimpleHttpClient.CRLF
+                + "Transfer-encoding: chunked" + SimpleHttpClient.CRLF
+                + "Content-Type: text/plain" + SimpleHttpClient.CRLF;
+        if (expectPass) {
+            request += "Connection: close" + SimpleHttpClient.CRLF;
+        }
+        request += SimpleHttpClient.CRLF + chunks + "0" + SimpleHttpClient.CRLF
+                + SimpleHttpClient.CRLF;
+
+        TrailerClient client = new TrailerClient(tomcat.getConnector()
+                .getLocalPort());
+        client.setRequest(new String[] { request });
+
+        Exception processException = null;
+        client.connect();
+        try {
+            client.processRequest();
+        } catch (Exception e) {
+            // Socket was probably closed before client had a chance to read
+            // response
+            processException = e;
+        }
+        if (expectPass) {
+            if (expectReadWholeBody) {
+                assertNull(processException);
+            }
+            if (processException == null) {
+                assertTrue(client.getResponseLine(), client.isResponse200());
+                assertEquals(String.valueOf(expectReadCount),
+                        client.getResponseBody());
+            }
+            assertEquals(expectReadCount, servlet.getCountRead());
+        } else {
+            if (processException == null) {
+                assertTrue(client.getResponseLine(), client.isResponse500());
+            }
+            assertEquals(0, servlet.getCountRead());
+            assertTrue(servlet.getExceptionDuringRead());
+        }
+    }
+
     private static class EchoHeaderServlet extends HttpServlet {
         private static final long serialVersionUID = 1L;
 
         private boolean exceptionDuringRead = false;
 
+        private final boolean expectPass;
+
+        public EchoHeaderServlet(boolean expectPass) {
+            this.expectPass = expectPass;
+        }
+
         @Override
         protected void doPost(HttpServletRequest req, HttpServletResponse resp)
                 throws ServletException, IOException {
@@ -334,6 +459,11 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp)
                 }
             } catch (IOException ioe) {
                 exceptionDuringRead = true;
+                if (!expectPass) { // as expected
+                    log(ioe.toString());
+                    resp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+                    return;
+                }
                 throw ioe;
             }
 
@@ -358,6 +488,53 @@ private void dumpHeader(String headerName, HttpServletRequest req,
         }
     }
 
+    private static class BodyReadServlet extends HttpServlet {
+        private static final long serialVersionUID = 1L;
+
+        private boolean exceptionDuringRead = false;
+        private int countRead = 0;
+        private final boolean expectPass;
+        private final int readLimit;
+
+        public BodyReadServlet(boolean expectPass, int readLimit) {
+            this.expectPass = expectPass;
+            this.readLimit = readLimit;
+        }
+
+        @Override
+        protected void doPost(HttpServletRequest req, HttpServletResponse resp)
+                throws ServletException, IOException {
+            resp.setContentType("text/plain");
+            PrintWriter pw = resp.getWriter();
+
+            // Read the body - quick and dirty
+            InputStream is = req.getInputStream();
+            try {
+                while (is.read() > -1 && countRead < readLimit) {
+                    countRead++;
+                }
+            } catch (IOException ioe) {
+                exceptionDuringRead = true;
+                if (!expectPass) { // as expected
+                    log(ioe.toString());
+                    resp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+                    return;
+                }
+                throw ioe;
+            }
+
+            pw.write(Integer.valueOf(countRead).toString());
+        }
+
+        public boolean getExceptionDuringRead() {
+            return exceptionDuringRead;
+        }
+
+        public int getCountRead() {
+            return countRead;
+        }
+    }
+
     private static class TrailerClient extends SimpleHttpClient {
 
         public TrailerClient(int port) {

webapps/docs/changelog.xml

@@ -149,6 +149,11 @@
         Add experimental NIO2 connector. Based on code developed by
         Nabil Benothman. (remm)
       </add>
+      <fix>
+        Improve processing of chuck size from chunked headers. Avoid overflow
+        and use a bit shift instead of a multiplication as it is marginally
+        faster. (markt/kkolinko)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">