Skip to content

Commit

Permalink
Fix #11325 Content-Length checks
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
  • Loading branch information
gregw committed Dec 10, 2024
1 parent 0941581 commit 55aef42
Show file tree
Hide file tree
Showing 8 changed files with 86 additions and 84 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ enum ApiState
private WriteListener _writeListener;
private volatile Throwable _onError;
private Callback _closedCallback;
private long _contentLengthSet = -1;
private long _applicationContentLength = -1;

public HttpOutput(ServletChannel channel)
{
Expand Down Expand Up @@ -180,27 +180,39 @@ public long getWritten()
* similar handlers are used.
* @param len The content-length as set by the application.
*/
public void setContentLengthSet(long len)
public void setApplicationContentLength(long len)
{
_contentLengthSet = 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 getContentLengthSet()
public long getApplicationContentLength()
{
return _contentLengthSet;
return _applicationContentLength;
}

/**
* @return {@code true} if a Content-Length has been set and insufficient content has been written.
*/
public boolean isContentIncomplete()
{
return _contentLengthSet >= 0 && _written < _contentLengthSet;
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;
}

/**
Expand Down Expand Up @@ -381,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 (_contentLengthSet >= 0 && _written < _contentLengthSet)
if (isContentIncomplete())
error = new CancellationException("Completed whilst write pending");
break;

Expand Down Expand Up @@ -477,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 @@ -751,7 +763,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 = false;
boolean last;
boolean aggregate;
boolean flush;

Expand All @@ -764,12 +776,7 @@ public void write(byte[] b, int off, int len) throws IOException
int space = maximizeAggregateSpace();

// 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;
}
last = isAllContentWritten(written);

// Write will be aggregated if:
// + it is smaller than the commitSize
Expand Down Expand Up @@ -896,7 +903,7 @@ public void write(ByteBuffer buffer) throws IOException
// This write always bypasses aggregate buffer
int len = BufferUtil.length(buffer);
boolean flush;
boolean last = false;
boolean last;

// Async or Blocking ?
boolean async;
Expand All @@ -906,12 +913,7 @@ public void write(ByteBuffer buffer) throws IOException
long written = _written + len;

// 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;
}
last = isAllContentWritten(written);

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

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

boolean async = false;
Expand All @@ -994,12 +996,7 @@ public void write(int b) throws IOException
int space = maximizeAggregateSpace();

// 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;
}
last = isAllContentWritten(written);

flush = last || space == 1;

Expand Down Expand Up @@ -1148,6 +1145,7 @@ else if (crlf != null && crlf.hasRemaining())
*
* @param content The whole content to send
* @throws IOException if the send fails
* @deprecated Use {@link ResourceServlet}
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(ByteBuffer content) throws IOException
Expand All @@ -1164,6 +1162,7 @@ public void sendContent(ByteBuffer content) throws IOException
*
* @param in The stream content to send
* @throws IOException if the send fails
* @deprecated Use {@link ResourceServlet}
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(InputStream in) throws IOException
Expand All @@ -1180,6 +1179,7 @@ public void sendContent(InputStream in) throws IOException
*
* @param in The channel content to send
* @throws IOException if the send fails
* @deprecated Use {@link ResourceServlet}
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(ReadableByteChannel in) throws IOException
Expand All @@ -1196,6 +1196,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 Use {@link ResourceServlet}
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(ByteBuffer content, final Callback callback)
Expand Down Expand Up @@ -1229,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 Use {@link ResourceServlet}
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(InputStream in, Callback callback)
Expand All @@ -1246,6 +1248,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 Use {@link ResourceServlet}
*/
@Deprecated(forRemoval = true, since = "12.1.0")
public void sendContent(ReadableByteChannel in, Callback callback)
Expand All @@ -1257,6 +1260,7 @@ public void sendContent(ReadableByteChannel in, Callback callback)
new ReadableByteChannelWritingCB(in, callback).iterate();
}

@Deprecated(forRemoval = true, since = "12.1.0")
private boolean prepareSendContent(int len, Callback callback)
{
try (AutoLock ignored = _channelState.lock())
Expand Down Expand Up @@ -1328,7 +1332,7 @@ public void recycle()
_onError = null;
_firstByteNanoTime = -1;
_closedCallback = null;
_contentLengthSet = -1;
_applicationContentLength = -1;
}
}

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -352,11 +352,11 @@ public void setContentLengthLong(long len)

if (len >= 0)
{
getServletChannel().getHttpOutput().setContentLengthSet(len);
if (len > 0)
getResponse().getHeaders().put(HttpHeader.CONTENT_LENGTH, len);
else
getResponse().getHeaders().put(HttpFields.CONTENT_LENGTH_0);
getServletChannel().getHttpOutput().setApplicationContentLength(len);
if (len > 0)
getResponse().getHeaders().put(HttpHeader.CONTENT_LENGTH, len);
else
getResponse().getHeaders().put(HttpFields.CONTENT_LENGTH_0);
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ public void handle()
// they might be compressed (or changed) by child Handlers.
if (getHttpOutput().isContentIncomplete())
{
String message = "Insufficient content written %d < %d".formatted(getHttpOutput().getWritten(), getHttpOutput().getContentLengthSet());
String message = "Insufficient content written %d < %d".formatted(getHttpOutput().getWritten(), getHttpOutput().getApplicationContentLength());
if (isCommitted() || _completeAttempted)
abort(new IOException(message));
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import java.nio.ByteBuffer;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.function.Supplier;

import jakarta.servlet.DispatcherType;
Expand Down Expand Up @@ -503,9 +502,7 @@ public boolean onRemoveField(HttpField field)
{
if (isCommitted())
return false;
if (field.getHeader() == null)
return true;
if (Objects.requireNonNull(field.getHeader()) == HttpHeader.CONTENT_TYPE)
if (field.getHeader() == HttpHeader.CONTENT_TYPE)
{
_contentType = null;
_mimeType = null;
Expand Down
Loading

0 comments on commit 55aef42

Please sign in to comment.