From 003e46cae4d71e147d51fb8ea378e8989d381cc3 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 18 Aug 2023 17:09:17 +1000 Subject: [PATCH] Various cleanups in HttpParser (#10329) Various cleanups in HttpParser Signed-off-by: gregw --------- Signed-off-by: gregw --- .../org/eclipse/jetty/http/HttpParser.java | 47 +++++----- .../eclipse/jetty/http/HttpParserTest.java | 87 ++++++------------- 2 files changed, 50 insertions(+), 84 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index e68c03fab8b..8d910ae7b0d 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -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) diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java index 1bd21034c8a..b79007e07ae 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java @@ -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 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 _fields = new ArrayList<>(); - private List _trailers = new ArrayList<>(); + private final List _fields = new ArrayList<>(); + private final List _trailers = new ArrayList<>(); private String[] _hdr; private String[] _val; private int _headers;