Skip to content

Commit

Permalink
Various cleanups in HttpParser (#10329)
Browse files Browse the repository at this point in the history 
Various cleanups in HttpParser
Signed-off-by: gregw <gregw@webtide.com>

---------

Signed-off-by: gregw <gregw@webtide.com>
  • Loading branch information
@gregw
gregw committed Aug 18, 2023
1 parent e33d026 commit 003e46c
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 84 deletions.
47 changes: 23 additions & 24 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
View file
Expand Up @@ -489,7 +489,7 @@ private HttpTokens.Token next(ByteBuffer buffer)
/* Quick lookahead for the start state looking for a request method or an HTTP version,
* otherwise skip white space until something else to parse.
*/
private boolean quickStart(ByteBuffer buffer)
private void quickStart(ByteBuffer buffer)
{
if (_requestHandler != null)
{
Expand All @@ -500,7 +500,7 @@ private boolean quickStart(ByteBuffer buffer)
buffer.position(buffer.position() + _methodString.length() + 1);

setState(State.SPACE1);
return false;
return;
}
}
else if (_responseHandler != null)
Expand All @@ -510,7 +510,7 @@ else if (_responseHandler != null)
{
buffer.position(buffer.position() + _version.asString().length() + 1);
setState(State.SPACE1);
return false;
return;
}
}

Expand All @@ -531,7 +531,7 @@ else if (_responseHandler != null)
_string.setLength(0);
_string.append(t.getChar());
setState(_requestHandler != null ? State.METHOD : State.RESPONSE_VERSION);
return false;
return;
}
case OTEXT:
case SPACE:
Expand All @@ -549,7 +549,6 @@ else if (_responseHandler != null)
throw new BadMessageException(HttpStatus.BAD_REQUEST_400);
}
}
return false;
}

private void setString(String s)
Expand Down Expand Up @@ -970,22 +969,19 @@ private void parsedHeader()
case CONTENT_LENGTH:
if (_hasTransferEncoding)
checkViolation(TRANSFER_ENCODING_WITH_CONTENT_LENGTH);

long contentLength = convertContentLength(_valueString);
if (_hasContentLength)
{
checkViolation(MULTIPLE_CONTENT_LENGTHS);
if (convertContentLength(_valueString) != _contentLength)
if (contentLength != _contentLength)
throw new BadMessageException(HttpStatus.BAD_REQUEST_400, MULTIPLE_CONTENT_LENGTHS.getDescription());
}
_hasContentLength = true;

if (_endOfContent != EndOfContent.CHUNKED_CONTENT)
{
_contentLength = convertContentLength(_valueString);
if (_contentLength <= 0)
_endOfContent = EndOfContent.NO_CONTENT;
else
_endOfContent = EndOfContent.CONTENT_LENGTH;
_contentLength = contentLength;
_endOfContent = EndOfContent.CONTENT_LENGTH;
}
break;

Expand Down Expand Up @@ -1039,7 +1035,6 @@ else if (_endOfContent == EndOfContent.CHUNKED_CONTENT)
_parsedHost = _valueString;
if (!(_field instanceof HostPortHttpField) && _valueString != null && !_valueString.isEmpty())
{
HostPort hostPort;
if (UNSAFE_HOST_HEADER.isAllowedBy(_complianceMode))
{
_field = new HostPortHttpField(_header,
Expand Down Expand Up @@ -1111,15 +1106,21 @@ private void parsedTrailer()

private long convertContentLength(String valueString)
{
try
{
return Long.parseLong(valueString);
}
catch (NumberFormatException e)
if (valueString == null || valueString.length() == 0)
throw new BadMessageException("Invalid Content-Length Value", new NumberFormatException());

long value = 0;
int length = valueString.length();

for (int i = 0; i < length; i++)
{
LOG.trace("IGNORED", e);
throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Invalid Content-Length Value", e);
char c = valueString.charAt(i);
if (c < '0' || c > '9')
throw new BadMessageException("Invalid Content-Length Value", new NumberFormatException());

value = Math.addExact(Math.multiplyExact(value, 10), c - '0');
}
return value;
}

/*
Expand Down Expand Up @@ -1516,12 +1517,11 @@ public boolean parseNext(ByteBuffer buffer)
_methodString = null;
_endOfContent = EndOfContent.UNKNOWN_CONTENT;
_header = null;
if (quickStart(buffer))
return true;
quickStart(buffer);
}

// Request/response line
if (_state.ordinal() >= State.START.ordinal() && _state.ordinal() < State.HEADER.ordinal())
if (_state.ordinal() < State.HEADER.ordinal())
{
if (parseLine(buffer))
return true;
Expand Down Expand Up @@ -2020,7 +2020,6 @@ public interface ResponseHandler extends HttpHandler
void startResponse(HttpVersion version, int status, String reason);
}

@SuppressWarnings("serial")
private static class IllegalCharacterException extends BadMessageException
{
private IllegalCharacterException(State state, HttpTokens.Token token, ByteBuffer buffer)
Expand Down
87 changes: 27 additions & 60 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java
View file
Expand Up @@ -42,6 +42,7 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.Matchers.startsWith;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -1601,7 +1602,7 @@ public void testNoURI2()
}

@Test
public void testUnknownReponseVersion()
public void testUnknownResponseVersion()
{
ByteBuffer buffer = BufferUtil.toBuffer(
"HPPT/7.7 200 OK\r\n" +
Expand Down Expand Up @@ -1744,65 +1745,31 @@ public void testBadCR()
assertEquals(HttpParser.State.CLOSED, parser.getState());
}

@Test
public void testBadContentLength0()
{
ByteBuffer buffer = BufferUtil.toBuffer(
"GET / HTTP/1.0\r\n" +
"Content-Length: abc\r\n" +
"Connection: close\r\n" +
"\r\n");

HttpParser.RequestHandler handler = new Handler();
HttpParser parser = new HttpParser(handler);

parser.parseNext(buffer);
assertEquals("GET", _methodOrVersion);
assertEquals("Invalid Content-Length Value", _bad);
assertFalse(buffer.hasRemaining());
assertEquals(HttpParser.State.CLOSE, parser.getState());
parser.atEOF();
parser.parseNext(BufferUtil.EMPTY_BUFFER);
assertEquals(HttpParser.State.CLOSED, parser.getState());
}

@Test
public void testBadContentLength1()
{
ByteBuffer buffer = BufferUtil.toBuffer(
"GET / HTTP/1.0\r\n" +
"Content-Length: 9999999999999999999999999999999999999999999999\r\n" +
"Connection: close\r\n" +
"\r\n");

HttpParser.RequestHandler handler = new Handler();
HttpParser parser = new HttpParser(handler);

parser.parseNext(buffer);
assertEquals("GET", _methodOrVersion);
assertEquals("Invalid Content-Length Value", _bad);
assertFalse(buffer.hasRemaining());
assertEquals(HttpParser.State.CLOSE, parser.getState());
parser.atEOF();
parser.parseNext(BufferUtil.EMPTY_BUFFER);
assertEquals(HttpParser.State.CLOSED, parser.getState());
}

@Test
public void testBadContentLength2()
@ParameterizedTest
@ValueSource(strings = {
"abc",
"1.5",
"9999999999999999999999999999999999999999999999",
"-10",
"+10",
"1.0",
"1,0",
"10,"
})
public void testBadContentLengths(String contentLength)
{
ByteBuffer buffer = BufferUtil.toBuffer(
"GET / HTTP/1.0\r\n" +
"Content-Length: 1.5\r\n" +
"Connection: close\r\n" +
"\r\n");
"GET /test HTTP/1.1\r\n" +
"Host: localhost\r\n" +
"Content-Length: " + contentLength + "\r\n" +
"\r\n" +
"1234567890\r\n");

HttpParser.RequestHandler handler = new Handler();
HttpParser parser = new HttpParser(handler);
HttpParser parser = new HttpParser(handler, HttpCompliance.RFC2616_LEGACY);
parseAll(parser, buffer);

parser.parseNext(buffer);
assertEquals("GET", _methodOrVersion);
assertEquals("Invalid Content-Length Value", _bad);
assertThat(_bad, notNullValue());
assertFalse(buffer.hasRemaining());
assertEquals(HttpParser.State.CLOSE, parser.getState());
parser.atEOF();
Expand Down Expand Up @@ -2013,7 +1980,7 @@ public void testIPv6Host()
@Test
public void testBadIPv6Host()
{
try (StacklessLogging s = new StacklessLogging(HttpParser.class))
try (StacklessLogging ignored = new StacklessLogging(HttpParser.class))
{
ByteBuffer buffer = BufferUtil.toBuffer(
"GET / HTTP/1.1\r\n" +
Expand Down Expand Up @@ -2046,7 +2013,7 @@ public void testHostPort()

public static Stream<String> badHostHeaderSource()
{
return List.of(
return Stream.of(
":80", // no host, port only
"host:", // no port
"127.0.0.1:", // no port
Expand Down Expand Up @@ -2081,7 +2048,7 @@ public static Stream<String> badHostHeaderSource()
"' *; host xyz.hacking.pro; '",
"'/**/OR/**/1/**/=/**/1",
"AND (SELECT 1 FROM(SELECT COUNT(*),CONCAT('x',(SELECT (ELT(1=1,1))),'x',FLOOR(RAND(0)*2))x FROM INFORMATION_SCHEMA.CHARACTER_SETS GROUP BY x)a)"
).stream();
);
}

@ParameterizedTest
Expand Down Expand Up @@ -3023,8 +2990,8 @@ public void init()
private String _methodOrVersion;
private String _uriOrStatus;
private String _versionOrReason;
private List<HttpField> _fields = new ArrayList<>();
private List<HttpField> _trailers = new ArrayList<>();
private final List<HttpField> _fields = new ArrayList<>();
private final List<HttpField> _trailers = new ArrayList<>();
private String[] _hdr;
private String[] _val;
private int _headers;
Expand Down

0 comments on commit 003e46c

Please sign in to comment.