Various cleanups in HttpParser (#10329)

Various cleanups in HttpParser
Signed-off-by: gregw <gregw@webtide.com>

---------

Signed-off-by: gregw <gregw@webtide.com>
This commit is contained in:
Greg Wilkins 2023-08-18 17:09:17 +10:00 committed by GitHub
parent e33d026259
commit 003e46cae4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 50 additions and 84 deletions

View File

@ -489,7 +489,7 @@ public class HttpParser
/* 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)
{
@ -500,7 +500,7 @@ public class HttpParser
buffer.position(buffer.position() + _methodString.length() + 1);
setState(State.SPACE1);
return false;
return;
}
}
else if (_responseHandler != null)
@ -510,7 +510,7 @@ public class HttpParser
{
buffer.position(buffer.position() + _version.asString().length() + 1);
setState(State.SPACE1);
return false;
return;
}
}
@ -531,7 +531,7 @@ public class HttpParser
_string.setLength(0);
_string.append(t.getChar());
setState(_requestHandler != null ? State.METHOD : State.RESPONSE_VERSION);
return false;
return;
}
case OTEXT:
case SPACE:
@ -549,7 +549,6 @@ public class HttpParser
throw new BadMessageException(HttpStatus.BAD_REQUEST_400);
}
}
return false;
}
private void setString(String s)
@ -970,22 +969,19 @@ public class HttpParser
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;
@ -1039,7 +1035,6 @@ public class HttpParser
_parsedHost = _valueString;
if (!(_field instanceof HostPortHttpField) && _valueString != null && !_valueString.isEmpty())
{
HostPort hostPort;
if (UNSAFE_HOST_HEADER.isAllowedBy(_complianceMode))
{
_field = new HostPortHttpField(_header,
@ -1111,15 +1106,21 @@ public class HttpParser
private long convertContentLength(String valueString)
{
try
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++)
{
return Long.parseLong(valueString);
}
catch (NumberFormatException e)
{
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;
}
/*
@ -1516,12 +1517,11 @@ public class HttpParser
_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;
@ -2020,7 +2020,6 @@ public class HttpParser
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)

View File

@ -42,6 +42,7 @@ import static org.hamcrest.Matchers.contains;
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;
@ -1601,7 +1602,7 @@ public class HttpParserTest
}
@Test
public void testUnknownReponseVersion()
public void testUnknownResponseVersion()
{
ByteBuffer buffer = BufferUtil.toBuffer(
"HPPT/7.7 200 OK\r\n" +
@ -1744,65 +1745,31 @@ public class HttpParserTest
assertEquals(HttpParser.State.CLOSED, parser.getState());
}
@Test
public void testBadContentLength0()
@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: abc\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);
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()
{
ByteBuffer buffer = BufferUtil.toBuffer(
"GET / HTTP/1.0\r\n" +
"Content-Length: 1.5\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);
assertThat(_bad, notNullValue());
assertFalse(buffer.hasRemaining());
assertEquals(HttpParser.State.CLOSE, parser.getState());
parser.atEOF();
@ -2013,7 +1980,7 @@ public class HttpParserTest
@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" +
@ -2046,7 +2013,7 @@ public class HttpParserTest
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
@ -2081,7 +2048,7 @@ public class HttpParserTest
"' *; 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
@ -3023,8 +2990,8 @@ public class HttpParserTest
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;