Do not throw exceptions when parsing request chunks (#441)

detail::read_content_chunked was using std::stoul to parse the
hexadecimal chunk lengths for "Transfer-Encoding: chunked" requests.
This throws an exception if the string does not begin with any valid
digits. read_content_chunked is not called in the context of a try block
so this caused the process to terminate.

Rather than use exceptions, I opted for std::stroul, which is similar to
std::stoul but does not throw exceptions. Since malformed user input is
not particularly exceptional, and some projects are compiled without
exception support, this approach seems both more portable and more
correct.
This commit is contained in:
Matthew DeVore 2020-04-23 06:05:45 -07:00 committed by GitHub
parent e1506fa186
commit c49441ae64
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 46 additions and 8 deletions

View file

@ -1893,9 +1893,16 @@ inline bool read_content_chunked(Stream &strm, ContentReceiver out) {
if (!line_reader.getline()) { return false; } if (!line_reader.getline()) { return false; }
auto chunk_len = std::stoul(line_reader.ptr(), 0, 16); unsigned long chunk_len;
while (true) {
char *end_ptr;
chunk_len = std::strtoul(line_reader.ptr(), &end_ptr, 16);
if (end_ptr == line_reader.ptr()) { return false; }
if (chunk_len == 0) { break; }
while (chunk_len > 0) {
if (!read_content_with_length(strm, chunk_len, nullptr, out)) { if (!read_content_with_length(strm, chunk_len, nullptr, out)) {
return false; return false;
} }
@ -1905,8 +1912,6 @@ inline bool read_content_chunked(Stream &strm, ContentReceiver out) {
if (strcmp(line_reader.ptr(), "\r\n")) { break; } if (strcmp(line_reader.ptr(), "\r\n")) { break; }
if (!line_reader.getline()) { return false; } if (!line_reader.getline()) { return false; }
chunk_len = std::stoul(line_reader.ptr(), 0, 16);
} }
if (chunk_len == 0) { if (chunk_len == 0) {

View file

@ -2189,7 +2189,8 @@ TEST_F(ServerTest, MultipartFormDataGzip) {
#endif #endif
// Sends a raw request to a server listening at HOST:PORT. // Sends a raw request to a server listening at HOST:PORT.
static bool send_request(time_t read_timeout_sec, const std::string &req) { static bool send_request(time_t read_timeout_sec, const std::string &req,
std::string *resp = nullptr) {
auto client_sock = detail::create_client_socket(HOST, PORT, /*timeout_sec=*/5, auto client_sock = detail::create_client_socket(HOST, PORT, /*timeout_sec=*/5,
std::string()); std::string());
@ -2207,7 +2208,9 @@ static bool send_request(time_t read_timeout_sec, const std::string &req) {
char buf[512]; char buf[512];
detail::stream_line_reader line_reader(strm, buf, sizeof(buf)); detail::stream_line_reader line_reader(strm, buf, sizeof(buf));
while (line_reader.getline()) {} while (line_reader.getline()) {
if (resp) { *resp += line_reader.ptr(); }
}
return true; return true;
}); });
} }
@ -2239,11 +2242,15 @@ TEST(ServerRequestParsingTest, TrimWhitespaceFromHeaderValues) {
} }
// Sends a raw request and verifies that there isn't a crash or exception. // Sends a raw request and verifies that there isn't a crash or exception.
static void test_raw_request(const std::string &req) { static void test_raw_request(const std::string &req,
std::string *out = nullptr) {
Server svr; Server svr;
svr.Get("/hi", [&](const Request & /*req*/, Response &res) { svr.Get("/hi", [&](const Request & /*req*/, Response &res) {
res.set_content("ok", "text/plain"); res.set_content("ok", "text/plain");
}); });
svr.Put("/put_hi", [&](const Request & /*req*/, Response &res) {
res.set_content("ok", "text/plain");
});
// Server read timeout must be longer than the client read timeout for the // Server read timeout must be longer than the client read timeout for the
// bug to reproduce, probably to force the server to process a request // bug to reproduce, probably to force the server to process a request
@ -2256,7 +2263,7 @@ static void test_raw_request(const std::string &req) {
std::this_thread::sleep_for(std::chrono::milliseconds(1)); std::this_thread::sleep_for(std::chrono::milliseconds(1));
} }
ASSERT_TRUE(send_request(client_read_timeout_sec, req)); ASSERT_TRUE(send_request(client_read_timeout_sec, req, out));
svr.stop(); svr.stop();
t.join(); t.join();
EXPECT_TRUE(listen_thread_ok); EXPECT_TRUE(listen_thread_ok);
@ -2310,6 +2317,32 @@ TEST(ServerRequestParsingTest, ReadHeadersRegexComplexity2) {
"&&&%%%"); "&&&%%%");
} }
TEST(ServerRequestParsingTest, InvalidFirstChunkLengthInRequest) {
std::string out;
test_raw_request(
"PUT /put_hi HTTP/1.1\r\n"
"Content-Type: text/plain\r\n"
"Transfer-Encoding: chunked\r\n"
"\r\n"
"nothex\r\n", &out);
EXPECT_EQ("HTTP/1.1 400 Bad Request", out.substr(0, 24));
}
TEST(ServerRequestParsingTest, InvalidSecondChunkLengthInRequest) {
std::string out;
test_raw_request(
"PUT /put_hi HTTP/1.1\r\n"
"Content-Type: text/plain\r\n"
"Transfer-Encoding: chunked\r\n"
"\r\n"
"3\r\n"
"xyz\r\n"
"NaN\r\n", &out);
EXPECT_EQ("HTTP/1.1 400 Bad Request", out.substr(0, 24));
}
TEST(ServerStopTest, StopServerWithChunkedTransmission) { TEST(ServerStopTest, StopServerWithChunkedTransmission) {
Server svr; Server svr;