Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #5214 - Servlet HEAD doesn't support content-length over Integer.MAX_VALUE #5215

Merged
merged 7 commits into from
Sep 2, 2020
Merged

Issue #5214 - Servlet HEAD doesn't support content-length over Integer.MAX_VALUE #5215

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1168713
Issue #5214 - Replicating HEAD Content-Length failure for huge files
joakime Aug 31, 2020
eba360f
Issue #5214 - Use known content_length when in bypass write
joakime Aug 31, 2020
dcb06d3
Issue #5214 - Use known content_length when in HEAD mode
joakime Aug 31, 2020
9128fc4
Cleanup dead code in ResourceService
joakime Aug 31, 2020
9e326fe
+ improved sentinels values for content-length
gregw Aug 31, 2020
a6b7f73
Merge branch 'jetty-9.4.x-5214-head-huge-static' of https://github.co…
gregw Aug 31, 2020
418093a
Feedback from review:
gregw Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ protected boolean passConditionalHeaders(HttpServletRequest request, HttpServlet
{
//Get jetty's Response impl
String mdlm = content.getLastModifiedValue();
if (mdlm != null && ifms.equals(mdlm))
if (ifms.equals(mdlm))
{
sendStatus(response, HttpServletResponse.SC_NOT_MODIFIED, content::getETagValue);
return false;
Expand Down Expand Up @@ -674,10 +674,10 @@ protected boolean sendData(HttpServletRequest request,
content.getResource().writeTo(out, 0, content_length);
}
// else if we can't do a bypass write because of wrapping
else if (written || !(out instanceof HttpOutput))
else if (written)
{
// write normally
putHeaders(response, content, written ? -1 : 0);
putHeaders(response, content, Response.NO_CONTENT_LENGTH);
ByteBuffer buffer = content.getIndirectBuffer();
if (buffer != null)
BufferUtil.writeTo(buffer, out);
Expand All @@ -688,7 +688,7 @@ else if (written || !(out instanceof HttpOutput))
else
{
// write the headers
putHeaders(response, content, 0);
putHeaders(response, content, Response.USE_KNOWN_CONTENT_LENGTH);

// write the content asynchronously if supported
if (request.isAsyncSupported() && content.getContentLengthValue() > response.getBufferSize())
Expand Down Expand Up @@ -736,7 +736,7 @@ public String toString()
// if there are no satisfiable ranges, send 416 response
if (ranges == null || ranges.size() == 0)
{
putHeaders(response, content, 0);
putHeaders(response, content, Response.USE_KNOWN_CONTENT_LENGTH);
response.setHeader(HttpHeader.CONTENT_RANGE.asString(),
InclusiveByteRange.to416HeaderRangeString(content_length));
sendStatus(response, HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE, null);
Expand All @@ -763,8 +763,8 @@ public String toString()
// 216 response which does not require an overall
// content-length header
//
putHeaders(response, content, -1);
String mimetype = (content == null ? null : content.getContentTypeValue());
putHeaders(response, content, Response.NO_CONTENT_LENGTH);
String mimetype = content.getContentTypeValue();
if (mimetype == null)
LOG.warn("Unknown mimetype for " + request.getRequestURI());
MultiPartOutputStream multi = new MultiPartOutputStream(out);
Expand Down
10 changes: 6 additions & 4 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ public class Response implements HttpServletResponse
private static final Logger LOG = Log.getLogger(Response.class);
private static final int __MIN_BUFFER_SIZE = 1;
private static final HttpField __EXPIRES_01JAN1970 = new PreEncodedHttpField(HttpHeader.EXPIRES, DateGenerator.__01Jan1970);
public static final int NO_CONTENT_LENGTH = -1;
public static final int USE_KNOWN_CONTENT_LENGTH = -2;

public enum OutputType
{
Expand Down Expand Up @@ -1289,12 +1291,12 @@ public void putHeaders(HttpContent content, long contentLength, boolean etag)
if (lm != null)
_fields.put(lm);

if (contentLength == 0)
if (contentLength == USE_KNOWN_CONTENT_LENGTH)
{
_fields.put(content.getContentLength());
_contentLength = content.getContentLengthValue();
}
else if (contentLength > 0)
else if (contentLength > NO_CONTENT_LENGTH)
{
_fields.putLongField(HttpHeader.CONTENT_LENGTH, contentLength);
_contentLength = contentLength;
Expand Down Expand Up @@ -1337,9 +1339,9 @@ public static void putHeaders(HttpServletResponse response, HttpContent content,
if (lml >= 0)
response.setDateHeader(HttpHeader.LAST_MODIFIED.asString(), lml);

if (contentLength == 0)
if (contentLength == USE_KNOWN_CONTENT_LENGTH)
contentLength = content.getContentLengthValue();
if (contentLength >= 0)
if (contentLength > NO_CONTENT_LENGTH)
{
if (contentLength < Integer.MAX_VALUE)
response.setContentLength((int)contentLength);
Expand Down
16 changes: 12 additions & 4 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/DefaultServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,20 +462,28 @@ protected void doPost(HttpServletRequest request, HttpServletResponse response)
doGet(request, response);
}

@Override
protected void doHead(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException
{
doGet(request, response);
}

/* (non-Javadoc)
* @see javax.servlet.http.HttpServlet#doTrace(javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse)
*/
@Override
protected void doTrace(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
protected void doTrace(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException
{
resp.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
response.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
}

@Override
protected void doOptions(HttpServletRequest req, HttpServletResponse resp)
protected void doOptions(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException
{
resp.setHeader("Allow", "GET,HEAD,POST,OPTIONS");
response.setHeader("Allow", "GET,HEAD,POST,OPTIONS");
}

/*
Expand Down
20 changes: 20 additions & 0 deletions jetty-webapp/src/test/java/org/eclipse/jetty/webapp/HugeResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,26 @@ public void testDownload(String filename, long expectedSize) throws Exception
}
}

@ParameterizedTest
@MethodSource("staticFiles")
public void testHead(String filename, long expectedSize) throws Exception
{
URI destUri = server.getURI().resolve("/" + filename);
InputStreamResponseListener responseListener = new InputStreamResponseListener();

Request request = client.newRequest(destUri)
.method(HttpMethod.HEAD);
request.send(responseListener);
Response response = responseListener.get(5, TimeUnit.SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responseListener.get() only waits for the headers, so to be sure here I would also do (maybe in a try-with-resources):

assertEquals(-1, response.getInputStream().read());

which would guarantee that the HEAD response really came without body, as it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.
But if that fails, does that mean that both jetty server and jetty client have bugs then? ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not matter - we will have a failed test and we'll investigate :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joakime I'll fix this to get this merged ASAP.


assertThat("HTTP Response Code", response.getStatus(), is(200));
// dumpResponse(response);

String contentLength = response.getHeaders().get(HttpHeader.CONTENT_LENGTH);
long contentLengthLong = Long.parseLong(contentLength);
assertThat("Http Response Header: \"Content-Length: " + contentLength + "\"", contentLengthLong, is(expectedSize));
}

@ParameterizedTest
@MethodSource("staticFiles")
public void testUpload(String filename, long expectedSize) throws Exception
Expand Down