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

Fix #11325 Content-Length checks #12622

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Fix #11325 Content-Length checks #12622

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0941581
Fix #11325 Content-Length checks
gregw Dec 10, 2024
55aef42
Fix #11325 Content-Length checks
gregw Dec 10, 2024
af40148
Fix #11325 Content-Length checks
gregw Dec 10, 2024
6637ed1
Fix #11325 Content-Length checks
gregw Dec 11, 2024
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
75 changes: 68 additions & 7 deletions jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java
View file
Open in desktop
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 _contentLengthSet = -1;

public HttpOutput(ServletChannel channel)
{
Expand Down Expand Up @@ -174,6 +175,34 @@ 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 setContentLengthSet(long len)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
_contentLengthSet = 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 getContentLengthSet()
{
return _contentLengthSet;
}

/**
* @return {@code true} if a Content-Length has been set and insufficient content has been written.
*/
public boolean isContentIncomplete()
{
return _contentLengthSet >= 0 && _written < _contentLengthSet;
gregw marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Used by ServletCoreResponse when it bypasses HttpOutput to update bytes written.
* @param written The bytes written
Expand Down Expand Up @@ -352,7 +381,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 (_contentLengthSet >= 0 && _written < _contentLengthSet)
gregw marked this conversation as resolved.
Show resolved Hide resolved
error = new CancellationException("Completed whilst write pending");
break;

Expand Down Expand Up @@ -722,7 +751,7 @@ public void write(byte[] b, int off, int len) throws IOException
if (LOG.isDebugEnabled())
LOG.debug("write(array {})", BufferUtil.toDetailString(ByteBuffer.wrap(b, off, len)));

boolean last;
boolean last = false;
boolean aggregate;
boolean flush;

Expand All @@ -733,7 +762,15 @@ 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?
if (_contentLengthSet >= 0)
{
if (_written > _contentLengthSet)
throw new IllegalStateException("too much content written");
last = written == _contentLengthSet;
}
gregw marked this conversation as resolved.
Show resolved Hide resolved

// 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 @@ -859,15 +896,23 @@ public void write(ByteBuffer buffer) throws IOException
// This write always bypasses aggregate buffer
int len = BufferUtil.length(buffer);
boolean flush;
boolean last;
boolean last = false;

// Async or Blocking ?
boolean async;
try (AutoLock ignored = _channelState.lock())
{
checkWritable();
long written = _written + len;
last = _servletChannel.getServletContextResponse().isAllContentWritten(written);

// Is this the last write due to content-length?
if (_contentLengthSet >= 0)
{
if (_written > _contentLengthSet)
throw new IllegalStateException("too much content written");
last = written == _contentLengthSet;
}

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

if (last && _state == State.OPEN)
Expand Down Expand Up @@ -938,7 +983,7 @@ else if (last && !complete)
public void write(int b) throws IOException
{
boolean flush;
boolean last;
boolean last = false;
// Async or Blocking ?

boolean async = false;
Expand All @@ -947,7 +992,15 @@ 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?
if (_contentLengthSet >= 0)
{
if (_written > _contentLengthSet)
throw new IllegalStateException("too much content written");
last = written == _contentLengthSet;
}

flush = last || space == 1;

if (last && _state == State.OPEN)
Expand Down Expand Up @@ -1096,6 +1149,7 @@ else if (crlf != null && crlf.hasRemaining())
* @param content The whole content to send
* @throws IOException if the send fails
*/
@Deprecated(forRemoval = true, since = "12.1.0")
gregw marked this conversation as resolved.
Show resolved Hide resolved
public void sendContent(ByteBuffer content) throws IOException
{
if (LOG.isDebugEnabled())
Expand All @@ -1111,6 +1165,7 @@ public void sendContent(ByteBuffer content) throws IOException
* @param in The stream content to send
* @throws IOException if the send fails
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(InputStream in) throws IOException
{
try (Blocker.Callback blocker = _writeBlocker.callback())
Expand All @@ -1126,6 +1181,7 @@ public void sendContent(InputStream in) throws IOException
* @param in The channel content to send
* @throws IOException if the send fails
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(ReadableByteChannel in) throws IOException
{
try (Blocker.Callback blocker = _writeBlocker.callback())
Expand All @@ -1141,6 +1197,7 @@ public void sendContent(ReadableByteChannel in) throws IOException
* @param content The whole content to send
* @param callback The callback to use to notify success or failure
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(ByteBuffer content, final Callback callback)
{
if (LOG.isDebugEnabled())
Expand Down Expand Up @@ -1173,6 +1230,7 @@ public void failed(Throwable x)
* @param in The stream content to send
* @param callback The callback to use to notify success or failure
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(InputStream in, Callback callback)
{
if (LOG.isDebugEnabled())
Expand All @@ -1189,6 +1247,7 @@ public void sendContent(InputStream in, Callback callback)
* @param in The channel content to send
* @param callback The callback to use to notify success or failure
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(ReadableByteChannel in, Callback callback)
{
if (LOG.isDebugEnabled())
Expand Down Expand Up @@ -1269,6 +1328,7 @@ public void recycle()
_onError = null;
_firstByteNanoTime = -1;
_closedCallback = null;
_contentLengthSet = -1;
}
}

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

Expand Down
8 changes: 7 additions & 1 deletion ...0/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiResponse.java
View file
Open in desktop
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)
{
getServletChannel().getHttpOutput().setContentLengthSet(len);
if (len > 0)
getResponse().getHeaders().put(HttpHeader.CONTENT_LENGTH, len);
else if (len == 0)
else
getResponse().getHeaders().put(HttpFields.CONTENT_LENGTH_0);
gregw marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
getResponse().getHeaders().remove(HttpHeader.CONTENT_LENGTH);
}
}

@Override
Expand Down
40 changes: 11 additions & 29 deletions ...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java
View file
Open in desktop
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().getContentLengthSet());
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
Loading