This is an automated email from the ASF dual-hosted git repository.
remm 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 b9aff64 Remove internal Range helpers
b9aff64 is described below
commit b9aff64f78740235a5565004423be40cadc740ac
Author: remm <r...@apache.org>
AuthorDate: Mon Mar 23 15:54:23 2020 +0100
Remove internal Range helpers
Refactor DefaultServlet to avoid using an internal Range structure that
is duplicated from the parsing result. Hopefully no regressions.
---
TOMCAT-NEXT.txt | 2 -
.../apache/catalina/servlets/DefaultServlet.java | 172 ++++++++++-----------
.../org/apache/tomcat/util/http/parser/Ranges.java | 2 +-
webapps/docs/changelog.xml | 4 +
4 files changed, 83 insertions(+), 97 deletions(-)
diff --git a/TOMCAT-NEXT.txt b/TOMCAT-NEXT.txt
index 0b3a519..f8cd94c 100644
--- a/TOMCAT-NEXT.txt
+++ b/TOMCAT-NEXT.txt
@@ -36,8 +36,6 @@ New items for 10.0.x onwards:
3. RFC 3986 states (section 2.2) that a %nn encoded delimiter is NOT
equivalent
to the decoded form. Provide an option not to decode delimiters in %nn
form.
- 4. Refactor DefaultServlet to use Ranges in parseRanges().
-
Deferred until 10.1.x:
1. Remove the ExtensionValidator and associated classes (assuming that the
diff --git a/java/org/apache/catalina/servlets/DefaultServlet.java
b/java/org/apache/catalina/servlets/DefaultServlet.java
index 2db2251..406b120 100644
--- a/java/org/apache/catalina/servlets/DefaultServlet.java
+++ b/java/org/apache/catalina/servlets/DefaultServlet.java
@@ -41,7 +41,6 @@ import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.Enumeration;
-import java.util.Iterator;
import java.util.List;
import java.util.Locale;
import java.util.StringTokenizer;
@@ -151,9 +150,9 @@ public class DefaultServlet extends HttpServlet {
/**
* Full range marker.
*/
- protected static final ArrayList<Range> FULL = new ArrayList<>();
+ protected static final Ranges FULL = new Ranges(null, new
ArrayList<Ranges.Entry>());
- private static final Range IGNORE = new Range();
+ private static final ContentRange IGNORE = new ContentRange(null, 0, 0, 0);
/**
* MIME multipart separation string
@@ -614,7 +613,7 @@ public class DefaultServlet extends HttpServlet {
WebResource resource = resources.getResource(path);
- Range range = parseContentRange(req, resp);
+ ContentRange range = parseContentRange(req, resp);
if (range == null) {
// Processing error. parseContentRange() set the error code
@@ -666,7 +665,7 @@ public class DefaultServlet extends HttpServlet {
* @return the associated file object
* @throws IOException an IO error occurred
*/
- protected File executePartialPut(HttpServletRequest req, Range range,
+ protected File executePartialPut(HttpServletRequest req, ContentRange
range,
String path)
throws IOException {
@@ -703,10 +702,10 @@ public class DefaultServlet extends HttpServlet {
}
}
- randAccessContentFile.setLength(range.length);
+ randAccessContentFile.setLength(range.getLength());
// Append data in request input stream to contentFile
- randAccessContentFile.seek(range.start);
+ randAccessContentFile.seek(range.getStart());
int numBytesRead;
byte[] transferBuffer = new byte[BUFFER_SIZE];
try (BufferedInputStream requestBufInStream =
@@ -928,7 +927,7 @@ public class DefaultServlet extends HttpServlet {
}
}
- ArrayList<Range> ranges = FULL;
+ Ranges ranges = FULL;
long contentLength = -1L;
if (resource.isDirectory()) {
@@ -1148,21 +1147,22 @@ public class DefaultServlet extends HttpServlet {
} else {
- if ((ranges == null) || (ranges.isEmpty()))
+ if ((ranges == null) || (ranges.getEntries().isEmpty()))
return;
// Partial content response.
response.setStatus(HttpServletResponse.SC_PARTIAL_CONTENT);
- if (ranges.size() == 1) {
+ if (ranges.getEntries().size() == 1) {
- Range range = ranges.get(0);
- response.addHeader("Content-Range", "bytes "
- + range.start
- + "-" + range.end + "/"
- + range.length);
- long length = range.end - range.start + 1;
+ Ranges.Entry range = ranges.getEntries().get(0);
+ long resourceLength = resource.getContentLength();
+ long start = getStart(range, resourceLength);
+ long end = getEnd(range, resourceLength);
+ response.addHeader("Content-Range",
+ "bytes " + start + "-" + end + "/" + resourceLength);
+ long length = end - start + 1;
response.setContentLengthLong(length);
if (contentType != null) {
@@ -1180,7 +1180,7 @@ public class DefaultServlet extends HttpServlet {
}
if (ostream != null) {
if (!checkSendfile(request, response, resource,
- range.end - range.start + 1, range))
+ end - start + 1, range))
copy(resource, ostream, range);
} else {
// we should not get here
@@ -1197,7 +1197,7 @@ public class DefaultServlet extends HttpServlet {
// Silent catch
}
if (ostream != null) {
- copy(resource, ostream, ranges.iterator(),
contentType);
+ copy(resource, ostream, ranges, contentType);
} else {
// we should not get here
throw new IllegalStateException();
@@ -1291,6 +1291,41 @@ public class DefaultServlet extends HttpServlet {
contentType.endsWith("xml") ||
contentType.contains("/javascript");
}
+ private static boolean validate(ContentRange range) {
+ // bytes is the only range unit supported
+ return (range != null) && ("bytes".equals(range.getUnits())) &&
(range.getStart() >= 0)
+ && (range.getEnd() >= 0) && (range.getStart() <=
range.getEnd()) && (range.getLength() > 0);
+ }
+
+ private static boolean validate(Ranges.Entry range, long length) {
+ long start = getStart(range, length);
+ long end = getEnd(range, length);
+ return (range != null) && (start >= 0) && (end >= 0) && (start <= end);
+ }
+
+ private static long getStart(Ranges.Entry range, long length) {
+ long start = range.getStart();
+ if (start == -1 ) {
+ long end = range.getEnd();
+ // If there is no start, then the start is based on the end
+ if (end >= length) {
+ return 0;
+ } else {
+ return length - end;
+ }
+ } else {
+ return start;
+ }
+ }
+
+ private static long getEnd(Ranges.Entry range, long length) {
+ long end = range.getEnd();
+ if (range.getStart() == -1 || end == -1 || end >= length) {
+ return length - 1;
+ } else {
+ return end;
+ }
+ }
private boolean pathEndsWithCompressedExtension(String path) {
for (CompressionFormat format : compressionFormats) {
@@ -1397,7 +1432,7 @@ public class DefaultServlet extends HttpServlet {
* process
* @throws IOException an IO error occurred
*/
- protected Range parseContentRange(HttpServletRequest request,
+ protected ContentRange parseContentRange(HttpServletRequest request,
HttpServletResponse response)
throws IOException {
@@ -1420,26 +1455,12 @@ public class DefaultServlet extends HttpServlet {
return null;
}
-
- // bytes is the only range unit supported
- if (!contentRange.getUnits().equals("bytes")) {
- response.sendError(HttpServletResponse.SC_BAD_REQUEST);
- return null;
- }
-
- // TODO: Remove the internal representation and use Ranges
- // Convert to internal representation
- Range range = new Range();
- range.start = contentRange.getStart();
- range.end = contentRange.getEnd();
- range.length = contentRange.getLength();
-
- if (!range.validate()) {
+ if (!validate(contentRange)) {
response.sendError(HttpServletResponse.SC_BAD_REQUEST);
return null;
}
- return range;
+ return contentRange;
}
@@ -1453,7 +1474,7 @@ public class DefaultServlet extends HttpServlet {
* {@code #FULL} if the Range header should be ignored.
* @throws IOException an IO error occurred
*/
- protected ArrayList<Range> parseRange(HttpServletRequest request,
+ protected Ranges parseRange(HttpServletRequest request,
HttpServletResponse response,
WebResource resource) throws IOException {
@@ -1528,37 +1549,15 @@ public class DefaultServlet extends HttpServlet {
return FULL;
}
- // TODO: Remove the internal representation and use Ranges
- // Convert to internal representation
- ArrayList<Range> result = new ArrayList<>();
-
- for (Ranges.Entry entry : ranges.getEntries()) {
- Range currentRange = new Range();
- 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 {
- currentRange.start = entry.getStart();
- currentRange.end = entry.getEnd();
- }
- currentRange.length = fileLength;
-
- if (!currentRange.validate()) {
+ for (Ranges.Entry range : ranges.getEntries()) {
+ if (!validate(range, fileLength)) {
response.addHeader("Content-Range", "bytes */" + fileLength);
response.sendError(HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE);
return null;
}
-
- result.add(currentRange);
}
- return result;
+ return ranges;
}
@@ -2123,7 +2122,7 @@ public class DefaultServlet extends HttpServlet {
protected boolean checkSendfile(HttpServletRequest request,
HttpServletResponse response,
WebResource resource,
- long length, Range range) {
+ long length, Ranges.Entry range) {
String canonicalPath;
if (sendfileSize > 0
&& length > sendfileSize
@@ -2138,8 +2137,8 @@ public class DefaultServlet extends HttpServlet {
request.setAttribute(Globals.SENDFILE_FILE_START_ATTR,
Long.valueOf(0L));
request.setAttribute(Globals.SENDFILE_FILE_END_ATTR,
Long.valueOf(length));
} else {
- request.setAttribute(Globals.SENDFILE_FILE_START_ATTR,
Long.valueOf(range.start));
- request.setAttribute(Globals.SENDFILE_FILE_END_ATTR,
Long.valueOf(range.end + 1));
+ request.setAttribute(Globals.SENDFILE_FILE_START_ATTR,
Long.valueOf(getStart(range, resource.getContentLength())));
+ request.setAttribute(Globals.SENDFILE_FILE_END_ATTR,
Long.valueOf(getEnd(range, resource.getContentLength()) + 1));
}
return true;
}
@@ -2387,7 +2386,7 @@ public class DefaultServlet extends HttpServlet {
* @exception IOException if an input/output error occurs
*/
protected void copy(WebResource resource, ServletOutputStream ostream,
- Range range)
+ Ranges.Entry range)
throws IOException {
IOException exception = null;
@@ -2395,7 +2394,8 @@ public class DefaultServlet extends HttpServlet {
InputStream resourceInputStream = resource.getInputStream();
InputStream istream =
new BufferedInputStream(resourceInputStream, input);
- exception = copyRange(istream, ostream, range.start, range.end);
+ exception = copyRange(istream, ostream, getStart(range,
resource.getContentLength()),
+ getEnd(range, resource.getContentLength()));
// Clean up the input stream
istream.close();
@@ -2420,31 +2420,33 @@ public class DefaultServlet extends HttpServlet {
* @exception IOException if an input/output error occurs
*/
protected void copy(WebResource resource, ServletOutputStream ostream,
- Iterator<Range> ranges, String contentType)
+ Ranges ranges, String contentType)
throws IOException {
IOException exception = null;
+ long length = resource.getContentLength();
- while ( (exception == null) && (ranges.hasNext()) ) {
-
+ for (Ranges.Entry range : ranges.getEntries()) {
+ if (exception != null) {
+ break;
+ }
InputStream resourceInputStream = resource.getInputStream();
try (InputStream istream = new
BufferedInputStream(resourceInputStream, input)) {
- Range currentRange = ranges.next();
-
// Writing MIME header.
ostream.println();
ostream.println("--" + mimeSeparation);
if (contentType != null)
ostream.println("Content-Type: " + contentType);
- ostream.println("Content-Range: bytes " + currentRange.start
- + "-" + currentRange.end + "/"
- + currentRange.length);
+ long start = getStart(range, length);
+ long end = getEnd(range, length);
+ ostream.println("Content-Range: bytes " + start
+ + "-" + end + "/"
+ + (end - start));
ostream.println();
// Printing content
- exception = copyRange(istream, ostream, currentRange.start,
- currentRange.end);
+ exception = copyRange(istream, ostream, start, end);
}
}
@@ -2580,24 +2582,6 @@ public class DefaultServlet extends HttpServlet {
}
- protected static class Range {
-
- public long start;
- public long end;
- public long length;
-
- /**
- * Validate range.
- *
- * @return true if the range is valid, otherwise false
- */
- public boolean validate() {
- if (end >= length)
- end = length - 1;
- return (start >= 0) && (end >= 0) && (start <= end) && (length >
0);
- }
- }
-
protected static class CompressionFormat implements Serializable {
private static final long serialVersionUID = 1L;
public final String extension;
diff --git a/java/org/apache/tomcat/util/http/parser/Ranges.java
b/java/org/apache/tomcat/util/http/parser/Ranges.java
index c937eb5..15bdbbf 100644
--- a/java/org/apache/tomcat/util/http/parser/Ranges.java
+++ b/java/org/apache/tomcat/util/http/parser/Ranges.java
@@ -28,7 +28,7 @@ public class Ranges {
private final List<Entry> entries;
- private Ranges(String units, List<Entry> entries) {
+ public Ranges(String units, List<Entry> entries) {
this.units = units;
this.entries = Collections.unmodifiableList(entries);
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index cc6269d..1e8e357 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -66,6 +66,10 @@
not override a possibly present <code>jarsToScan</code>. Based on code
submitted by Iridias. (remm)
</fix>
+ <update>
+ Refactor DefaultServlet to avoid using an internal Range structure that
+ is duplicated from the parsing result. (remm)
+ </update>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
