Improve parsing of Range request headers

markt-asf · markt-asf · commit 276faa5536cd · 2019-06-25T09:26:09.000+01:00
diff --git a/TOMCAT-NEXT.txt b/TOMCAT-NEXT.txt
@@ -58,3 +58,6 @@ New items for 10.0.x onwards:
  9. BZ 56966. Refactor internal request timing to use System.nanoTime()
 
 10. BZ 63286. Make behaviour of %D and %T consistent with httpd.
+
+11. Refactor DefaultServlet to use Ranges in parseRanges()
+
diff --git a/java/org/apache/catalina/servlets/DefaultServlet.java b/java/org/apache/catalina/servlets/DefaultServlet.java
@@ -80,6 +80,7 @@
 import org.apache.catalina.webresources.CachedResource;
 import org.apache.tomcat.util.buf.B2CConverter;
 import org.apache.tomcat.util.http.ResponseUtil;
+import org.apache.tomcat.util.http.parser.Ranges;
 import org.apache.tomcat.util.res.StringManager;
 import org.apache.tomcat.util.security.Escape;
 import org.apache.tomcat.util.security.PrivilegedGetTccl;
@@ -1480,87 +1481,51 @@ protected ArrayList<Range> parseRange(HttpServletRequest request,
 
         long fileLength = resource.getContentLength();
 
-        if (fileLength == 0)
+        if (fileLength == 0) {
             return null;
+        }
 
         // Retrieving the range header (if any is specified
         String rangeHeader = request.getHeader("Range");
 
-        if (rangeHeader == null)
+        if (rangeHeader == null) {
             return null;
+        }
+
+        Ranges ranges = Ranges.parseRanges(new StringReader(rangeHeader));
+
         // bytes is the only range unit supported (and I don't see the point
         // of adding new ones).
-        if (!rangeHeader.startsWith("bytes")) {
+        if (ranges == null || !ranges.getUnits().equals("bytes")) {
             response.addHeader("Content-Range", "bytes */" + fileLength);
-            response.sendError
-                (HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE);
+            response.sendError(HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE);
             return null;
         }
 
-        rangeHeader = rangeHeader.substring(6);
-
-        // Collection which will contain all the ranges which are successfully
-        // parsed.
+        // TODO: Remove the internal representation and use Ranges
+        // Convert to internal representation
         ArrayList<Range> result = new ArrayList<>();
-        StringTokenizer commaTokenizer = new StringTokenizer(rangeHeader, ",");
-
-        // Parsing the range list
-        while (commaTokenizer.hasMoreTokens()) {
-            String rangeDefinition = commaTokenizer.nextToken().trim();
 
+        for (Ranges.Entry entry : ranges.getEntries()) {
             Range currentRange = new Range();
-            currentRange.length = fileLength;
-
-            int dashPos = rangeDefinition.indexOf('-');
-
-            if (dashPos == -1) {
-                response.addHeader("Content-Range", "bytes */" + fileLength);
-                response.sendError
-                    (HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE);
-                return null;
-            }
-
-            if (dashPos == 0) {
-
-                try {
-                    long offset = Long.parseLong(rangeDefinition);
-                    currentRange.start = fileLength + offset;
-                    currentRange.end = fileLength - 1;
-                } catch (NumberFormatException e) {
-                    response.addHeader("Content-Range",
-                                       "bytes */" + fileLength);
-                    response.sendError
-                        (HttpServletResponse
-                         .SC_REQUESTED_RANGE_NOT_SATISFIABLE);
-                    return null;
+            if (entry.getStart() == -1) {
+                currentRange.start = fileLength - entry.getEnd();
+                if (currentRange.start < 0) {
+                    currentRange.start = 0;
                 }
-
+                currentRange.end = fileLength - 1;
+            } else if (entry.getEnd() == -1) {
+                currentRange.start = entry.getStart();
+                currentRange.end = fileLength - 1;
             } else {
-
-                try {
-                    currentRange.start = Long.parseLong
-                        (rangeDefinition.substring(0, dashPos));
-                    if (dashPos < rangeDefinition.length() - 1)
-                        currentRange.end = Long.parseLong
-                            (rangeDefinition.substring
-                             (dashPos + 1, rangeDefinition.length()));
-                    else
-                        currentRange.end = fileLength - 1;
-                } catch (NumberFormatException e) {
-                    response.addHeader("Content-Range",
-                                       "bytes */" + fileLength);
-                    response.sendError
-                        (HttpServletResponse
-                         .SC_REQUESTED_RANGE_NOT_SATISFIABLE);
-                    return null;
-                }
-
+                currentRange.start = entry.getStart();
+                currentRange.end = entry.getEnd();
             }
+            currentRange.length = fileLength;
 
             if (!currentRange.validate()) {
                 response.addHeader("Content-Range", "bytes */" + fileLength);
-                response.sendError
-                    (HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE);
+                response.sendError(HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE);
                 return null;
             }
 
diff --git a/java/org/apache/tomcat/util/http/parser/HttpParser.java b/java/org/apache/tomcat/util/http/parser/HttpParser.java
@@ -384,6 +384,43 @@ static String readToken(Reader input) throws IOException {
         }
     }
 
+    /**
+     * @return  the digits if any were found, the empty string if no data was
+     *          found or if data other than digits was found
+     */
+    static String readDigits(Reader input) throws IOException {
+        StringBuilder result = new StringBuilder();
+
+        skipLws(input);
+        input.mark(1);
+        int c = input.read();
+
+        while (c != -1 && isNumeric(c)) {
+            result.append((char) c);
+            input.mark(1);
+            c = input.read();
+        }
+        // Use mark(1)/reset() rather than skip(-1) since skip() is a NOP
+        // once the end of the String has been reached.
+        input.reset();
+
+        return result.toString();
+    }
+
+    /**
+     * @return  the number if digits were found, -1 if no data was found
+     *          or if data other than digits was found
+     */
+    static long readLong(Reader input) throws IOException {
+        String digits = readDigits(input);
+
+        if (digits.length() == 0) {
+            return -1;
+        }
+
+        return Long.parseLong(digits);
+    }
+
     /**
      * @return the quoted string if one was found, null if data other than a
      *         quoted string was found or null if the end of data was reached
diff --git a/java/org/apache/tomcat/util/http/parser/Ranges.java b/java/org/apache/tomcat/util/http/parser/Ranges.java
@@ -0,0 +1,124 @@
+/*
+ *  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.tomcat.util.http.parser;
+
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+public class Ranges {
+
+    private final String units;
+    private final List<Entry> entries;
+
+
+    private Ranges(String units, List<Entry> entries) {
+        this.units = units;
+        this.entries = Collections.unmodifiableList(entries);
+    }
+
+
+    public List<Entry> getEntries() {
+        return entries;
+    }
+
+    public String getUnits() {
+        return units;
+    }
+
+
+    public static class Entry {
+
+        private final long start;
+        private final long end;
+
+
+        public Entry(long start, long end) {
+            this.start = start;
+            this.end = end;
+        }
+
+
+        public long getStart() {
+            return start;
+        }
+
+
+        public long getEnd() {
+            return end;
+        }
+    }
+
+
+    /**
+     * Parses a Range header from an HTTP header.
+     *
+     * @param input a reader over the header text
+     *
+     * @return a set of ranges parsed from the input, or null if not valid
+     *
+     * @throws IOException if there was a problem reading the input
+     */
+    public static Ranges parseRanges(StringReader input) throws IOException {
+
+        // Units (required)
+        String units = HttpParser.readToken(input);
+        if (units == null || units.length() == 0) {
+            return null;
+        }
+
+        // Must be followed by '='
+        if (HttpParser.skipConstant(input, "=") == SkipResult.NOT_FOUND) {
+            return null;
+        }
+
+        // Range entries
+        List<Entry> entries = new ArrayList<>();
+
+        SkipResult skipResult;
+        do {
+            long start = HttpParser.readLong(input);
+            // Must be followed by '-'
+            if (HttpParser.skipConstant(input, "-") == SkipResult.NOT_FOUND) {
+                return null;
+            }
+            long end = HttpParser.readLong(input);
+
+            if (start == -1 && end == -1) {
+                // Invalid range
+                return null;
+            }
+
+            entries.add(new Entry(start, end));
+
+            skipResult = HttpParser.skipConstant(input, ",");
+            if (skipResult == SkipResult.NOT_FOUND) {
+                // Invalid range
+                return null;
+            }
+        } while (skipResult == SkipResult.FOUND);
+
+        // There must be at least one entry
+        if (entries.size() == 0) {
+            return null;
+        }
+
+        return new Ranges(units, entries);
+    }
+}
diff --git a/test/org/apache/catalina/servlets/TestDefaultServletRangeRequests.java b/test/org/apache/catalina/servlets/TestDefaultServletRangeRequests.java
@@ -37,22 +37,50 @@
 @RunWith(Parameterized.class)
 public class TestDefaultServletRangeRequests extends TomcatBaseTest {
 
-    @Parameterized.Parameters
+    // TODO: Add a check for response headers
+    @Parameterized.Parameters(name = "{index} rangeHeader [{0}]")
     public static Collection<Object[]> parameters() {
+
+        // Note: The index page used by this test has a content-length of 934 bytes
         List<Object[]> parameterSets = new ArrayList<>();
 
-        parameterSets.add(new Object[] { null, Integer.valueOf(200)});
+        parameterSets.add(new Object[] { "", Integer.valueOf(200), "934", "" });
         // Invalid
-        // Commented out as these tests currently fail
-        //parameterSets.add(new Object[] { buildRangeHeader("bytes"), Integer.valueOf(416)});
-        //parameterSets.add(new Object[] { buildRangeHeader("bytes="), Integer.valueOf(416)});
+        parameterSets.add(new Object[] { "bytes", Integer.valueOf(416), "", "*/934" });
+        parameterSets.add(new Object[] { "bytes=", Integer.valueOf(416), "", "*/934" });
+        // Invalid with unknown type
+        parameterSets.add(new Object[] { "unknown", Integer.valueOf(416), "", "*/934" });
+        parameterSets.add(new Object[] { "unknown=", Integer.valueOf(416), "", "*/934" });
+        // Invalid ranges
+        parameterSets.add(new Object[] { "bytes=-", Integer.valueOf(416), "", "*/934" });
+        parameterSets.add(new Object[] { "bytes=10-b", Integer.valueOf(416), "", "*/934" });
+        parameterSets.add(new Object[] { "bytes=b-10", Integer.valueOf(416), "", "*/934" });
+        // Invalid no equals
+        parameterSets.add(new Object[] { "bytes 1-10", Integer.valueOf(416), "", "*/934" });
+        parameterSets.add(new Object[] { "bytes1-10", Integer.valueOf(416), "", "*/934" });
+        parameterSets.add(new Object[] { "bytes10-", Integer.valueOf(416), "", "*/934" });
+        parameterSets.add(new Object[] { "bytes-10", Integer.valueOf(416), "", "*/934" });
+        // Unknown types
+        parameterSets.add(new Object[] { "unknown=1-2", Integer.valueOf(416), "", "*/934" });
+        parameterSets.add(new Object[] { "bytesX=1-2", Integer.valueOf(416), "", "*/934" });
+        // Valid range
+        parameterSets.add(new Object[] { "bytes=0-9", Integer.valueOf(206), "10", "0-9/934" });
+        parameterSets.add(new Object[] { "bytes=-100", Integer.valueOf(206), "100", "834-933/934" });
+        parameterSets.add(new Object[] { "bytes=100-", Integer.valueOf(206), "834", "100-933/934" });
+        // Valid range (too much)
+        parameterSets.add(new Object[] { "bytes=0-1000", Integer.valueOf(206), "934", "0-933/934" });
+        parameterSets.add(new Object[] { "bytes=-1000", Integer.valueOf(206), "934", "0-933/934" });
         return parameterSets;
     }
 
     @Parameter(0)
-    public Map<String,List<String>> requestHeaders;
+    public String rangeHeader;
     @Parameter(1)
     public int responseCodeExpected;
+    @Parameter(2)
+    public String contentLengthExpected;
+    @Parameter(3)
+    public String responseRangeExpected;
 
     @Test
     public void testRange() throws Exception {
@@ -72,19 +100,36 @@ public void testRange() throws Exception {
         ByteChunk responseBody = new ByteChunk();
         Map<String,List<String>> responseHeaders = new HashMap<>();
 
-        int rc = getUrl(path, responseBody, requestHeaders, responseHeaders);
+        int rc = getUrl(path, responseBody, buildRangeHeader(rangeHeader), responseHeaders);
 
         // Check the result
         Assert.assertEquals(responseCodeExpected, rc);
+
+        if (contentLengthExpected.length() > 0) {
+            String contentLength = responseHeaders.get("Content-Length").get(0);
+            Assert.assertEquals(contentLengthExpected, contentLength);
+        }
+
+        if (responseRangeExpected.length() > 0) {
+            String responseRange = responseHeaders.get("Content-Range").get(0);
+            Assert.assertEquals("bytes " + responseRangeExpected, responseRange);
+        }
     }
 
 
     private static Map<String,List<String>> buildRangeHeader(String... headerValues) {
         Map<String,List<String>> requestHeaders = new HashMap<>();
         List<String> values = new ArrayList<>();
         for (String headerValue : headerValues) {
-            values.add(headerValue);
+            if (headerValue.length() > 0) {
+                values.add(headerValue);
+            }
+        }
+
+        if (values.size() == 0) {
+            return null;
         }
+
         requestHeaders.put("range", values);
 
         return requestHeaders;
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
