From c49441ae6455ac827556f9f3a72cfe6022d55469 Mon Sep 17 00:00:00 2001
From: Matthew DeVore <matvore@google.com>
Date: Thu, 23 Apr 2020 06:05:45 -0700
Subject: [PATCH] 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.
---
 httplib.h    | 13 +++++++++----
 test/test.cc | 41 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/httplib.h b/httplib.h
index ab1e529..fa8492d 100644
--- a/httplib.h
+++ b/httplib.h
@@ -1893,9 +1893,16 @@ inline bool read_content_chunked(Stream &strm, ContentReceiver out) {
 
   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)) {
       return false;
     }
@@ -1905,8 +1912,6 @@ inline bool read_content_chunked(Stream &strm, ContentReceiver out) {
     if (strcmp(line_reader.ptr(), "\r\n")) { break; }
 
     if (!line_reader.getline()) { return false; }
-
-    chunk_len = std::stoul(line_reader.ptr(), 0, 16);
   }
 
   if (chunk_len == 0) {
diff --git a/test/test.cc b/test/test.cc
index 033d734..b84c89f 100644
--- a/test/test.cc
+++ b/test/test.cc
@@ -2189,7 +2189,8 @@ TEST_F(ServerTest, MultipartFormDataGzip) {
 #endif
 
 // 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,
                                                   std::string());
 
@@ -2207,7 +2208,9 @@ static bool send_request(time_t read_timeout_sec, const std::string &req) {
         char buf[512];
 
         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;
       });
 }
@@ -2239,11 +2242,15 @@ TEST(ServerRequestParsingTest, TrimWhitespaceFromHeaderValues) {
 }
 
 // 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;
   svr.Get("/hi", [&](const Request & /*req*/, Response &res) {
     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
   // 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));
   }
 
-  ASSERT_TRUE(send_request(client_read_timeout_sec, req));
+  ASSERT_TRUE(send_request(client_read_timeout_sec, req, out));
   svr.stop();
   t.join();
   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) {
   Server svr;