You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/06/25 08:26:51 UTC
[tomcat] branch master updated: Improve parsing of Range request
headers
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push:
new 276faa5 Improve parsing of Range request headers
276faa5 is described below
commit 276faa5536cddebe0d812d46b98cba07a8cc5ea1
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Jun 25 09:26:09 2019 +0100
Improve parsing of Range request headers
---
TOMCAT-NEXT.txt | 3 +
.../apache/catalina/servlets/DefaultServlet.java | 85 +++++---------
.../apache/tomcat/util/http/parser/HttpParser.java | 37 ++++++
.../org/apache/tomcat/util/http/parser/Ranges.java | 124 +++++++++++++++++++++
.../servlets/TestDefaultServletRangeRequests.java | 61 ++++++++--
webapps/docs/changelog.xml | 7 ++
6 files changed, 249 insertions(+), 68 deletions(-)
diff --git a/TOMCAT-NEXT.txt b/TOMCAT-NEXT.txt
index 5454467..9400227 100644
--- 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
index 5c7275d..6bd9882 100644
--- a/java/org/apache/catalina/servlets/DefaultServlet.java
+++ b/java/org/apache/catalina/servlets/DefaultServlet.java
@@ -80,6 +80,7 @@ import org.apache.catalina.util.URLEncoder;
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 @@ public class DefaultServlet extends HttpServlet {
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
index 8702059..989be63 100644
--- a/java/org/apache/tomcat/util/http/parser/HttpParser.java
+++ b/java/org/apache/tomcat/util/http/parser/HttpParser.java
@@ -385,6 +385,43 @@ public class HttpParser {
}
/**
+ * @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
* before the quoted string was terminated
diff --git a/java/org/apache/tomcat/util/http/parser/Ranges.java b/java/org/apache/tomcat/util/http/parser/Ranges.java
new file mode 100644
index 0000000..1921d87
--- /dev/null
+++ 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
index 8459570..14675b1 100644
--- a/test/org/apache/catalina/servlets/TestDefaultServletRangeRequests.java
+++ b/test/org/apache/catalina/servlets/TestDefaultServletRangeRequests.java
@@ -37,22 +37,50 @@ import org.apache.tomcat.util.buf.ByteChunk;
@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,10 +100,20 @@ public class TestDefaultServletRangeRequests extends TomcatBaseTest {
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);
+ }
}
@@ -83,8 +121,15 @@ public class TestDefaultServletRangeRequests extends TomcatBaseTest {
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
index 2f77315..5093cc1 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -45,6 +45,13 @@
issues do not "pop up" wrt. others).
-->
<section name="Tomcat 9.0.22 (markt)" rtext="in development">
+ <subsection name="Catalina">
+ <changelog>
+ <fix>
+ Improve parsing of Range request headers. (markt)
+ </fix>
+ </changelog>
+ </subsection>
<subsection name="Coyote">
<changelog>
<fix>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org