Skip to content

Commit

Permalink
Fix #11325 Content-Length checks (#12622)
Browse files Browse the repository at this point in the history
Fix #11325 by:
 + removing content-length tracking from ServletContextResponse
 + adding a setContentLengthSet field to HttpOutput
 + added HttpOutputTest for EE10/EE11
  • Loading branch information
gregw authored Dec 12, 2024
1 parent 2c55f4d commit 3188ccd
Show file tree
Hide file tree
Showing 21 changed files with 3,682 additions and 355 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -589,11 +589,11 @@ public boolean addEventListener(EventListener listener)
{
if (super.addEventListener(listener))
{
if (listener instanceof ContextScopeListener)
if (listener instanceof ContextScopeListener contextScopeListener)
{
_contextListeners.add((ContextScopeListener)listener);
_contextListeners.add(contextScopeListener);
if (__context.get() != null)
((ContextScopeListener)listener).enterScope(__context.get(), null);
contextScopeListener.enterScope(__context.get(), null);
}
return true;
}
Expand All @@ -605,9 +605,12 @@ public boolean removeEventListener(EventListener listener)
{
if (super.removeEventListener(listener))
{
if (listener instanceof ContextScopeListener)
_contextListeners.remove(listener);

if (listener instanceof ContextScopeListener contextScopeListener)
{
_contextListeners.remove(contextScopeListener);
if (__context.get() != null)
contextScopeListener.exitScope(__context.get(), null);
}
return true;
}
return false;
Expand Down Expand Up @@ -1053,6 +1056,7 @@ public boolean handle(Request request, Response response, Callback callback) thr
return true;

// Past this point we are calling the downstream handler in scope.
Context lastContext = getCurrentContext();
ClassLoader lastLoader = enterScope(contextRequest);
ContextResponse contextResponse = wrapResponse(contextRequest, response);
try
Expand All @@ -1068,7 +1072,7 @@ public boolean handle(Request request, Response response, Callback callback) thr
{
// We exit scope here, even though handle() is asynchronous,
// as we have wrapped all our callbacks to re-enter the scope.
exitScope(contextRequest, request.getContext(), lastLoader);
exitScope(contextRequest, lastContext, lastLoader);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ enum ApiState
private WriteListener _writeListener;
private volatile Throwable _onError;
private Callback _closedCallback;
private long _applicationContentLength = -1;

public HttpOutput(ServletChannel channel)
{
Expand Down Expand Up @@ -174,6 +175,46 @@ public long getWritten()
return _written;
}

/**
* Set the content-length as set by the application. This may not be the actual content length if compression or
* similar handlers are used.
* @param len The content-length as set by the application.
*/
public void setApplicationContentLength(long len)
{
_applicationContentLength = len;
}

/**
* Get the content-length as set by the application. This may not be the actual content length if compression or
* similar handlers are used.
* @return The content-length as set by the application.
*/
public long getApplicationContentLength()
{
return _applicationContentLength;
}

/**
* @return {@code true} if a Content-Length has been set and insufficient content has been written.
*/
public boolean isContentIncomplete()
{
long applicationContentLength = _applicationContentLength;
return applicationContentLength >= 0 && _written < applicationContentLength;
}

private boolean isAllContentWritten(long written)
{
if (_applicationContentLength >= 0)
{
if (written > _applicationContentLength)
throw new IllegalStateException("too much content written");
return written == _applicationContentLength;
}
return false;
}

/**
* Used by ServletCoreResponse when it bypasses HttpOutput to update bytes written.
* @param written The bytes written
Expand Down Expand Up @@ -352,7 +393,7 @@ public void complete(Callback callback)

case PENDING: // an async write is pending and may complete at any time
// If this is not the last write, then we must abort
if (_servletChannel.getServletContextResponse().isContentIncomplete(_written))
if (isContentIncomplete())
error = new CancellationException("Completed whilst write pending");
break;

Expand Down Expand Up @@ -448,9 +489,9 @@ public void complete(Callback callback)
/**
* Called to indicate that the request cycle has been completed.
*/
public void completed(Throwable failure)
public void completed(Throwable ignored)
{
try (AutoLock ignored = _channelState.lock())
try (AutoLock ignoredLock = _channelState.lock())
{
_state = State.CLOSED;
lockedReleaseBuffer();
Expand Down Expand Up @@ -733,7 +774,10 @@ public void write(byte[] b, int off, int len) throws IOException
checkWritable();
long written = _written + len;
int space = maximizeAggregateSpace();
last = _servletChannel.getServletContextResponse().isAllContentWritten(written);

// Is this the last write due to content-length?
last = isAllContentWritten(written);

// Write will be aggregated if:
// + it is smaller than the commitSize
// + is not the last one, or is last but will fit in an already allocated aggregate buffer.
Expand Down Expand Up @@ -867,7 +911,10 @@ public void write(ByteBuffer buffer) throws IOException
{
checkWritable();
long written = _written + len;
last = _servletChannel.getServletContextResponse().isAllContentWritten(written);

// Is this the last write due to content-length?
last = isAllContentWritten(written);

flush = last || len > 0 || (_aggregate != null && _aggregate.hasRemaining());

if (last && _state == State.OPEN)
Expand Down Expand Up @@ -947,7 +994,10 @@ public void write(int b) throws IOException
checkWritable();
long written = _written + 1;
int space = maximizeAggregateSpace();
last = _servletChannel.getServletContextResponse().isAllContentWritten(written);

// Is this the last write due to content-length?
last = isAllContentWritten(written);

flush = last || space == 1;

if (last && _state == State.OPEN)
Expand Down Expand Up @@ -1269,6 +1319,7 @@ public void recycle()
_onError = null;
_firstByteNanoTime = -1;
_closedCallback = null;
_applicationContentLength = -1;
}
}

Expand All @@ -1279,6 +1330,7 @@ public void resetBuffer()
if (_aggregate != null)
_aggregate.clear();
_written = 0;
_applicationContentLength = -1;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,12 +350,18 @@ public void setContentLengthLong(long len)
if (isCommitted())
return;

if (len > 0)
getResponse().getHeaders().put(HttpHeader.CONTENT_LENGTH, len);
else if (len == 0)
getResponse().getHeaders().put(HttpFields.CONTENT_LENGTH_0);
if (len >= 0)
{
getServletChannel().getHttpOutput().setApplicationContentLength(len);
if (len > 0)
getResponse().getHeaders().put(HttpHeader.CONTENT_LENGTH, len);
else
getResponse().getHeaders().put(HttpFields.CONTENT_LENGTH_0);
}
else
{
getResponse().getHeaders().remove(HttpHeader.CONTENT_LENGTH);
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ public class ServletChannel
private Request _request;
private Response _response;
private Callback _callback;
private boolean _completeAttempted;

public ServletChannel(ServletContextHandler servletContextHandler, Request request)
{
Expand Down Expand Up @@ -396,6 +397,7 @@ void recycle(Throwable x)
_request = null;
_response = null;
_callback = null;
_completeAttempted = false;
}

/**
Expand Down Expand Up @@ -564,14 +566,19 @@ public void handle()
{
// Compare the bytes written by the application, even if
// they might be compressed (or changed) by child Handlers.
long written = response.getContentBytesWritten();
if (response.isContentIncomplete(written))
if (getHttpOutput().isContentIncomplete())
{
sendErrorOrAbort("Insufficient content written %d < %d".formatted(written, response.getContentLength()));
String message = "Insufficient content written %d < %d".formatted(getHttpOutput().getWritten(), getHttpOutput().getApplicationContentLength());
if (isCommitted() || _completeAttempted)
abort(new IOException(message));
else
getServletContextResponse().getServletApiResponse().sendError(HttpStatus.INTERNAL_SERVER_ERROR_500, message);
break;
}
}

_completeAttempted = true;

// Set a close callback on the HttpOutput to make it an async callback
response.completeOutput(Callback.from(NON_BLOCKING, () -> _state.completed(null), _state::completed));
break;
Expand Down Expand Up @@ -602,31 +609,6 @@ private void reopen()
getHttpOutput().reopen();
}

/**
* @param message the error message.
* @return true if we have sent an error, false if we have aborted.
*/
private boolean sendErrorOrAbort(String message)
{
try
{
if (isCommitted())
{
abort(new IOException(message));
return false;
}

getServletContextResponse().getServletApiResponse().sendError(HttpStatus.INTERNAL_SERVER_ERROR_500, message);
return true;
}
catch (Throwable x)
{
LOG.trace("IGNORED", x);
abort(x);
}
return false;
}

/**
* <p>Sends an error 500, performing a special logic to detect whether the request is suspended,
* to avoid concurrent writes from the application.</p>
Expand Down Expand Up @@ -664,7 +646,7 @@ else if (noStack != null)
try
{
boolean abort = _state.onError(failure);
if (abort)
if (abort || _completeAttempted)
abort(failure);
}
catch (Throwable x)
Expand Down
Loading

0 comments on commit 3188ccd

Please sign in to comment.