From 3bafa52145ee466fc5899ed519ceccecb691487e Mon Sep 17 00:00:00 2001 From: "gunsch@chromium.org" Date: Wed, 30 Apr 2014 04:49:30 +0000 Subject: HttpServer: avoid DCHECK'ing on non-HTTP/1.1 requests. BUG=b/14249697 R=mef@chromium.org,byungchul@chromium.org Review URL: https://codereview.chromium.org/251213004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267066 0039d316-1c4b-4281-b951-d872f2087c98 --- net/http/http_response_headers.cc | 55 +++++------------------------------ net/http/http_response_headers.h | 8 ----- net/http/http_util.cc | 42 ++++++++++++++++++++++++++ net/http/http_util.h | 6 ++++ net/server/http_server.cc | 7 +++-- net/server/http_server_request_info.h | 4 +++ net/server/http_server_unittest.cc | 12 ++++++++ 7 files changed, 76 insertions(+), 58 deletions(-) diff --git a/net/http/http_response_headers.cc b/net/http/http_response_headers.cc index 940c048..4e18056 100644 --- a/net/http/http_response_headers.cc +++ b/net/http/http_response_headers.cc @@ -627,62 +627,21 @@ HttpResponseHeaders::~HttpResponseHeaders() { // Note: this implementation implicitly assumes that line_end points at a valid // sentinel character (such as '\0'). -// static -HttpVersion HttpResponseHeaders::ParseVersion( - std::string::const_iterator line_begin, - std::string::const_iterator line_end) { - std::string::const_iterator p = line_begin; - - // RFC2616 sec 3.1: HTTP-Version = "HTTP" "/" 1*DIGIT "." 1*DIGIT - // TODO: (1*DIGIT apparently means one or more digits, but we only handle 1). - // TODO: handle leading zeros, which is allowed by the rfc1616 sec 3.1. - - if ((line_end - p < 4) || !LowerCaseEqualsASCII(p, p + 4, "http")) { - DVLOG(1) << "missing status line"; - return HttpVersion(); - } - - p += 4; - - if (p >= line_end || *p != '/') { - DVLOG(1) << "missing version"; - return HttpVersion(); - } - - std::string::const_iterator dot = std::find(p, line_end, '.'); - if (dot == line_end) { - DVLOG(1) << "malformed version"; - return HttpVersion(); - } - - ++p; // from / to first digit. - ++dot; // from . to second digit. - - if (!(*p >= '0' && *p <= '9' && *dot >= '0' && *dot <= '9')) { - DVLOG(1) << "malformed version number"; - return HttpVersion(); - } - - uint16 major = *p - '0'; - uint16 minor = *dot - '0'; - - return HttpVersion(major, minor); -} - -// Note: this implementation implicitly assumes that line_end points at a valid -// sentinel character (such as '\0'). void HttpResponseHeaders::ParseStatusLine( std::string::const_iterator line_begin, std::string::const_iterator line_end, bool has_headers) { // Extract the version number - parsed_http_version_ = ParseVersion(line_begin, line_end); + std::string::const_iterator first_space = + std::find(line_begin, line_end, ' '); + std::string version(line_begin, first_space); + bool success = HttpUtil::ParseVersion(version, &parsed_http_version_); // Clamp the version number to one of: {0.9, 1.0, 1.1} - if (parsed_http_version_ == HttpVersion(0, 9) && !has_headers) { + if (success && parsed_http_version_ == HttpVersion(0, 9) && !has_headers) { http_version_ = HttpVersion(0, 9); raw_headers_ = "HTTP/0.9"; - } else if (parsed_http_version_ >= HttpVersion(1, 1)) { + } else if (success && parsed_http_version_ >= HttpVersion(1, 1)) { http_version_ = HttpVersion(1, 1); raw_headers_ = "HTTP/1.1"; } else { @@ -696,7 +655,7 @@ void HttpResponseHeaders::ParseStatusLine( } // TODO(eroman): this doesn't make sense if ParseVersion failed. - std::string::const_iterator p = std::find(line_begin, line_end, ' '); + std::string::const_iterator p = first_space; if (p == line_end) { DVLOG(1) << "missing response status; assuming 200 OK"; diff --git a/net/http/http_response_headers.h b/net/http/http_response_headers.h index 2bf4514..90ba7eb 100644 --- a/net/http/http_response_headers.h +++ b/net/http/http_response_headers.h @@ -335,14 +335,6 @@ class NET_EXPORT HttpResponseHeaders // Initializes from the given raw headers. void Parse(const std::string& raw_input); - // Helper function for ParseStatusLine. - // Tries to extract the "HTTP/X.Y" from a status line formatted like: - // HTTP/1.1 200 OK - // with line_begin and end pointing at the begin and end of this line. If the - // status line is malformed, returns HttpVersion(0,0). - static HttpVersion ParseVersion(std::string::const_iterator line_begin, - std::string::const_iterator line_end); - // Tries to extract the status line from a header block, given the first // line of said header block. If the status line is malformed, we'll // construct a valid one. Example input: diff --git a/net/http/http_util.cc b/net/http/http_util.cc index f4f994a..481b104 100644 --- a/net/http/http_util.cc +++ b/net/http/http_util.cc @@ -77,6 +77,48 @@ size_t HttpUtil::FindDelimiter(const std::string& line, } // static +bool HttpUtil::ParseVersion(const std::string& version_str, + HttpVersion* version) { + std::string::const_iterator p = version_str.begin(); + + // RFC2616 sec 3.1: HTTP-Version = "HTTP" "/" 1*DIGIT "." 1*DIGIT + // TODO: (1*DIGIT apparently means one or more digits, but we only handle 1). + // TODO: handle leading zeros, which is allowed by the rfc1616 sec 3.1. + + if (version_str.length() < 4 || !LowerCaseEqualsASCII(p, p + 4, "http")) { + DVLOG(1) << "missing status line"; + return false; + } + + p += 4; + + if (p >= version_str.end() || *p != '/') { + DVLOG(1) << "missing version"; + return false; + } + + std::string::const_iterator dot = std::find(p, version_str.end(), '.'); + if (dot == version_str.end()) { + DVLOG(1) << "malformed version"; + return false; + } + + ++p; // from / to first digit. + ++dot; // from . to second digit. + + if (!(*p >= '0' && *p <= '9' && *dot >= '0' && *dot <= '9')) { + DVLOG(1) << "malformed version number"; + return false; + } + + uint16 major = *p - '0'; + uint16 minor = *dot - '0'; + + *version = HttpVersion(major, minor); + return true; +} + +// static void HttpUtil::ParseContentType(const std::string& content_type_str, std::string* mime_type, std::string* charset, diff --git a/net/http/http_util.h b/net/http/http_util.h index ae65146..5ab27a6 100644 --- a/net/http/http_util.h +++ b/net/http/http_util.h @@ -40,6 +40,12 @@ class NET_EXPORT HttpUtil { size_t search_start, char delimiter); + // Parses an "HTTP/X.Y" version from a string, such as: + // "HTTP/1.1" + // Returns whether or not the version string was successfully parsed. + static bool ParseVersion(const std::string& version_str, + HttpVersion* version); + // Parses the value of a Content-Type header. The resulting mime_type and // charset values are normalized to lowercase. The mime_type and charset // output values are only modified if the content_type_str contains a mime diff --git a/net/server/http_server.cc b/net/server/http_server.cc index df77c36..20926e7 100644 --- a/net/server/http_server.cc +++ b/net/server/http_server.cc @@ -13,6 +13,7 @@ #include "base/sys_byteorder.h" #include "build/build_config.h" #include "net/base/net_errors.h" +#include "net/http/http_util.h" #include "net/server/http_connection.h" #include "net/server/http_server_request_info.h" #include "net/server/http_server_response_info.h" @@ -282,8 +283,10 @@ bool HttpServer::ParseHeaders(HttpConnection* connection, buffer.clear(); break; case ST_PROTO: - // TODO(mbelshe): Deal better with parsing protocol. - DCHECK(buffer == "HTTP/1.1"); + if (!HttpUtil::ParseVersion(buffer, &info->http_version)) { + // Treat everything else like HTTP 1.0 + info->http_version = HttpVersion(1, 0); + } buffer.clear(); break; case ST_NAME: diff --git a/net/server/http_server_request_info.h b/net/server/http_server_request_info.h index 183da1c..0376b3a 100644 --- a/net/server/http_server_request_info.h +++ b/net/server/http_server_request_info.h @@ -9,6 +9,7 @@ #include #include "net/base/ip_endpoint.h" +#include "net/http/http_version.h" namespace net { @@ -28,6 +29,9 @@ class HttpServerRequestInfo { // Request peer address. IPEndPoint peer; + // Request protocol/version + HttpVersion http_version; + // Request method. std::string method; diff --git a/net/server/http_server_unittest.cc b/net/server/http_server_unittest.cc index de8690f..25a9cbc 100644 --- a/net/server/http_server_unittest.cc +++ b/net/server/http_server_unittest.cc @@ -218,11 +218,23 @@ TEST_F(HttpServerTest, Request) { ASSERT_EQ("/test", GetRequest(0).path); ASSERT_EQ("", GetRequest(0).data); ASSERT_EQ(0u, GetRequest(0).headers.size()); + ASSERT_EQ(HttpVersion(1, 1), GetRequest(0).http_version); ASSERT_TRUE(StartsWithASCII(GetRequest(0).peer.ToString(), "127.0.0.1", true)); } +TEST_F(HttpServerTest, InvalidHttpVersion) { + TestHttpClient client; + ASSERT_EQ(OK, client.ConnectAndWait(server_address_)); + client.Send("GET /test HTTP//1\r\n\r\n"); + ASSERT_TRUE(RunUntilRequestsReceived(1)); + ASSERT_EQ("GET", GetRequest(0).method); + ASSERT_EQ("/test", GetRequest(0).path); + ASSERT_EQ("", GetRequest(0).data); + ASSERT_EQ(HttpVersion(1, 0), GetRequest(0).http_version); +} + TEST_F(HttpServerTest, RequestWithHeaders) { TestHttpClient client; ASSERT_EQ(OK, client.ConnectAndWait(server_address_)); -- cgit v1.1