Skip to content

Commit

Permalink
Fix bug in max header calculation in DefaultPartHttpMessageReader
Browse files Browse the repository at this point in the history
This commit fixes a bug in the DefaultPartHttpMessageReader, in the
check for exceeding the maximum header size. Before this commit, the
entire buffer size was considered, thus triggering an exception even
though the max header limit was not exceeded. After this commit, we only
consider the size up until the end-of-header mark (CRLFCRLF).

Furthermore, this commit increases the default maximum header size to
10k, the same default as Commons File upload.

Closes gh-27612
  • Loading branch information
poutsma committed Oct 27, 2021
1 parent c4c3d59 commit 0416168
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 33 deletions.
Expand Up @@ -63,7 +63,7 @@ public class DefaultPartHttpMessageReader extends LoggingCodecSupport implements

private int maxInMemorySize = 256 * 1024;

private int maxHeadersSize = 8 * 1024;
private int maxHeadersSize = 10 * 1024;

private long maxDiskUsagePerPart = -1;

Expand Down
Expand Up @@ -342,33 +342,23 @@ private final class HeadersState implements State {

/**
* First checks whether the multipart boundary leading to this state
* was the final boundary, or whether {@link #maxHeadersSize} is
* exceeded. Then looks for the header-body boundary
* ({@code CR LF CR LF}) in the given buffer. If found, convert
* all buffers collected so far into a {@link HttpHeaders} object
* was the final boundary. Then looks for the header-body boundary
* ({@code CR LF CR LF}) in the given buffer. If found, checks whether
* the size of all header buffers does not exceed {@link #maxHeadersSize},
* converts all buffers collected so far into a {@link HttpHeaders} object
* and changes to {@link BodyState}, passing the remainder of the
* buffer. If the boundary is not found, the buffer is collected.
* buffer. If the boundary is not found, the buffer is collected if
* its size does not exceed {@link #maxHeadersSize}.
*/
@Override
public void onNext(DataBuffer buf) {
long prevCount = this.byteCount.get();
long count = this.byteCount.addAndGet(buf.readableByteCount());
if (prevCount < 2 && count >= 2) {
if (isLastBoundary(buf)) {
if (logger.isTraceEnabled()) {
logger.trace("Last boundary found in " + buf);
}

if (changeState(this, DisposedState.INSTANCE, buf)) {
emitComplete();
}
return;
if (isLastBoundary(buf)) {
if (logger.isTraceEnabled()) {
logger.trace("Last boundary found in " + buf);
}
}
else if (count > MultipartParser.this.maxHeadersSize) {

if (changeState(this, DisposedState.INSTANCE, buf)) {
emitError(new DataBufferLimitException("Part headers exceeded the memory usage limit of " +
MultipartParser.this.maxHeadersSize + " bytes"));
emitComplete();
}
return;
}
Expand All @@ -377,17 +367,23 @@ else if (count > MultipartParser.this.maxHeadersSize) {
if (logger.isTraceEnabled()) {
logger.trace("End of headers found @" + endIdx + " in " + buf);
}
DataBuffer headerBuf = MultipartUtils.sliceTo(buf, endIdx);
this.buffers.add(headerBuf);
DataBuffer bodyBuf = MultipartUtils.sliceFrom(buf, endIdx);
DataBufferUtils.release(buf);

emitHeaders(parseHeaders());
changeState(this, new BodyState(), bodyBuf);
long count = this.byteCount.addAndGet(endIdx);
if (belowMaxHeaderSize(count)) {
DataBuffer headerBuf = MultipartUtils.sliceTo(buf, endIdx);
this.buffers.add(headerBuf);
DataBuffer bodyBuf = MultipartUtils.sliceFrom(buf, endIdx);
DataBufferUtils.release(buf);

emitHeaders(parseHeaders());
changeState(this, new BodyState(), bodyBuf);
}
}
else {
this.buffers.add(buf);
requestBuffer();
long count = this.byteCount.addAndGet(buf.readableByteCount());
if (belowMaxHeaderSize(count)) {
this.buffers.add(buf);
requestBuffer();
}
}
}

Expand All @@ -407,6 +403,21 @@ private boolean isLastBoundary(DataBuffer buf) {
buf.getByte(0) == HYPHEN);
}

/**
* Checks whether the given {@code count} is below or equal to {@link #maxHeadersSize}
* and emits a {@link DataBufferLimitException} if not.
*/
private boolean belowMaxHeaderSize(long count) {
if (count <= MultipartParser.this.maxHeadersSize) {
return true;
}
else {
emitError(new DataBufferLimitException("Part headers exceeded the memory usage limit of " +
MultipartParser.this.maxHeadersSize + " bytes"));
return false;
}
}

/**
* Parses the list of buffers into a {@link HttpHeaders} instance.
* Converts the joined buffers into a string using ISO=8859-1, and parses
Expand Down
Expand Up @@ -270,6 +270,31 @@ public void utf8Headers(String displayName, DefaultPartHttpMessageReader reader)
latch.await();
}

// gh-27612
@Test
public void exceedHeaderLimit() throws InterruptedException {
Flux<DataBuffer> body = DataBufferUtils
.readByteChannel((new ClassPathResource("files.multipart", getClass()))::readableChannel, bufferFactory, 282);

MediaType contentType = new MediaType("multipart", "form-data", singletonMap("boundary", "----WebKitFormBoundaryG8fJ50opQOML0oGD"));
MockServerHttpRequest request = MockServerHttpRequest.post("/")
.contentType(contentType)
.body(body);

DefaultPartHttpMessageReader reader = new DefaultPartHttpMessageReader();

reader.setMaxHeadersSize(230);

Flux<Part> result = reader.read(forClass(Part.class), request, emptyMap());

CountDownLatch latch = new CountDownLatch(2);
StepVerifier.create(result)
.consumeNextWith(part -> testPart(part, null, LOREM_IPSUM, latch))
.consumeNextWith(part -> testPart(part, null, MUSPI_MEROL, latch))
.verifyComplete();

latch.await();
}

private void testBrowser(DefaultPartHttpMessageReader reader, Resource resource, String boundary)
throws InterruptedException {
Expand Down
Expand Up @@ -3,11 +3,9 @@ Content-Disposition: form-data; name="file2"; filename="a.txt"
Content-Type: text/plain

Lorem ipsum dolor sit amet, consectetur adipiscing elit. Integer iaculis metus id vestibulum nullam.

------WebKitFormBoundaryG8fJ50opQOML0oGD
Content-Disposition: form-data; name="file2"; filename="b.txt"
Content-Type: text/plain

.mallun mulubitsev di sutem silucai regetnI .tile gnicsipida rutetcesnoc ,tema tis rolod muspi meroL

------WebKitFormBoundaryG8fJ50opQOML0oGD--

0 comments on commit 0416168

Please sign in to comment.