Fix chunk header parsing

fix #431, fix #430

CHANGELOG.md

@@ -12,6 +12,7 @@ Version 50
 * Add missing handler_alloc nested types
 * Fix chunk delimiter parsing
 * Fix test::pipe read_size
+* Fix chunk header parsing
 
 API Changes:
 

include/beast/http/basic_parser.hpp

@@ -85,19 +85,18 @@ class basic_parser
     static unsigned constexpr flagHTTP11                = 1<<  4;
     static unsigned constexpr flagNeedEOF               = 1<<  5;
     static unsigned constexpr flagExpectCRLF            = 1<<  6;
-    static unsigned constexpr flagFinalChunk            = 1<<  7;
+    static unsigned constexpr flagConnectionClose       = 1<<  7;
-    static unsigned constexpr flagConnectionClose       = 1<<  8;
+    static unsigned constexpr flagConnectionUpgrade     = 1<<  8;
-    static unsigned constexpr flagConnectionUpgrade     = 1<<  9;
+    static unsigned constexpr flagConnectionKeepAlive   = 1<<  9;
-    static unsigned constexpr flagConnectionKeepAlive   = 1<< 10;
+    static unsigned constexpr flagContentLength         = 1<< 10;
-    static unsigned constexpr flagContentLength         = 1<< 11;
+    static unsigned constexpr flagChunked               = 1<< 11;
-    static unsigned constexpr flagChunked               = 1<< 12;
+    static unsigned constexpr flagUpgrade               = 1<< 12;
-    static unsigned constexpr flagUpgrade               = 1<< 13;
+    static unsigned constexpr flagFinalChunk            = 1<< 13;
 
     std::uint64_t len_;     // size of chunk or body
     std::unique_ptr<char[]> buf_;
     std::size_t buf_len_ = 0;
     std::size_t skip_ = 0;  // search from here
-    std::size_t x_;         // scratch variable
     state state_ = state::nothing_yet;
     unsigned f_ = 0;        // flags
 

include/beast/http/impl/basic_parser.ipp

@@ -29,7 +29,6 @@ basic_parser(basic_parser<
     , buf_(std::move(other.buf_))
     , buf_len_(other.buf_len_)
     , skip_(other.skip_)
-    , x_(other.x_)
     , state_(other.state_)
     , f_(other.f_)
 {
@@ -152,7 +151,7 @@ loop:
     case state::chunk_header0:
         impl().on_body(content_length(), ec);
         if(ec)
-            return 0;
+            goto done;
         state_ = state::chunk_header;
         // [[fallthrough]]
 
@@ -487,7 +486,7 @@ parse_body_to_eof(char const*& p,
 template<bool isRequest, class Derived>
 void
 basic_parser<isRequest, Derived>::
-parse_chunk_header(char const*& p,
+parse_chunk_header(char const*& p0,
     std::size_t n, error_code& ec)
 {
 /*
@@ -504,30 +503,9 @@ parse_chunk_header(char const*& p,
     chunk-ext-val  = token / quoted-string
 */
 
+    auto p = p0;
     auto const pend = p + n;
-    auto pbegin = p;
+    char const* eol;
-
-    // Treat the last CRLF in a chunk as
-    // part of the next chunk, so p can
-    // be parsed in one call instead of two.
-    if(f_ & flagExpectCRLF)
-    {
-        if(n < 2)
-        {
-            ec = error::need_more;
-            return;
-        }
-        if(! parse_crlf(p))
-        {
-            ec = error::bad_chunk;
-            return;
-        }
-        f_ &= ~flagExpectCRLF;
-        pbegin += 2;
-        n -= 2;
-    }
-
-    char const* term;
 
     if(! (f_ & flagFinalChunk))
     {
@@ -536,15 +514,29 @@ parse_chunk_header(char const*& p,
             ec = error::need_more;
             return;
         }
-        term = find_eol(p + skip_, pend, ec);
+        if(f_ & flagExpectCRLF)
+        {
+            // Treat the last CRLF in a chunk as
+            // part of the next chunk, so p can
+            // be parsed in one call instead of two.
+            if(! parse_crlf(p))
+            {
+                ec = error::bad_chunk;
+                return;
+            }
+        }
+        eol = find_eol(p0 + skip_, pend, ec);
         if(ec)
             return;
-        if(! term)
+        if(! eol)
         {
             ec = error::need_more;
             skip_ = n - 1;
             return;
         }
+        skip_ = static_cast<
+            std::size_t>(eol - 2 - p0);
+
         std::uint64_t v;
         if(! parse_hex(p, v))
         {
@@ -555,64 +547,63 @@ parse_chunk_header(char const*& p,
         {
             if(*p == ';')
             {
-                // VFALCO We need to parse the chunk
+                // VFALCO TODO Validate extension
-                // extension to validate p here.
+                impl().on_chunk(v, make_string(
-                auto const ext = make_string(p, term - 2);
+                    p, eol - 2), ec);
-                impl().on_chunk(v, ext, ec);
                 if(ec)
                     return;
             }
-            else if(p != term - 2)
+            else if(p != eol - 2)
             {
                 ec = error::bad_chunk;
                 return;
             }
-            p = term;
             len_ = v;
-            skip_ = 0;
+            skip_ = 2;
+            p0 = eol;
             f_ |= flagExpectCRLF;
             state_ = state::chunk_body;
             return;
         }
 
-        pbegin = p;
-        n = static_cast<std::size_t>(pend - pbegin);
-        x_ = term - 2 - pbegin; // start of first '\r\n'
-        skip_ = x_;
-
         f_ |= flagFinalChunk;
     }
+    else
+    {
+        BOOST_ASSERT(n >= 5);
+        if(f_ & flagExpectCRLF)
+            BOOST_VERIFY(parse_crlf(p));
+        std::uint64_t v;
+        BOOST_VERIFY(parse_hex(p, v));
+        eol = find_eol(p, pend, ec);
+        BOOST_ASSERT(! ec);
+    }
 
-    term = find_eom(
+    auto eom = find_eom(p0 + skip_, pend, ec);
-        pbegin + skip_, pend, ec);
     if(ec)
         return;
-    if(! term)
+    if(! eom)
     {
-        // VFALCO Is this right?
+        BOOST_ASSERT(n >= 3);
-        if(n > 3)
+        skip_ = n - 3;
-            skip_ = (pend - pbegin) - 3;
         ec = error::need_more;
         return;
     }
 
     if(*p == ';')
     {
-        auto const ext = make_string(p, pbegin + x_);
+        // VFALCO TODO Validate extension
-        impl().on_chunk(0, ext, ec);
+        impl().on_chunk(0, make_string(
+            p, eol - 2), ec);
         if(ec)
             return;
-        p = pbegin + x_;
     }
-    if(! parse_crlf(p))
+    p = eol;
-    {
+    parse_fields(p, eom, ec);
-        ec = error::bad_chunk;
-        return;
-    }
-    parse_fields(p, term, ec);
     if(ec)
         return;
-    BOOST_ASSERT(p == term);
+    BOOST_ASSERT(p == eom);
+    p0 = eom;
 
     impl().on_complete(ec);
     if(ec)

test/http/basic_parser.cpp

@@ -942,6 +942,116 @@ public:
 #endif
     }
 
+    //--------------------------------------------------------------------------
+
+    template<bool isRequest, class Derived>
+    void
+    put(basic_parser<isRequest, Derived>& p,
+        string_view s)
+    {
+        error_code ec;
+        auto const bytes_used = p.put(
+            boost::asio::buffer(s.data(), s.size()), ec);
+        BEAST_EXPECTS(! ec, ec.message());
+        BEAST_EXPECT(bytes_used == s.size());
+    }
+
+    // https://github.com/vinniefalco/Beast/issues/430
+    void
+    testRegression430()
+    {
+        {
+            test_parser<false> p;
+            p.eager(true);
+            put(p,
+              "HTTP/1.1 200 OK\r\n" "Transfer-Encoding: chunked\r\n" "Content-Type: application/octet-stream\r\n" "\r\n"
+              "4\r\nabcd\r\n"
+              "0\r\n\r\n"
+            );
+        }
+        {
+            test_parser<false> p;
+            p.eager(true);
+            put(p,
+              "HTTP/1.1 200 OK\r\n" "Transfer-Encoding: chunked\r\n" "Content-Type: application/octet-stream\r\n" "\r\n"
+              "4\r\nabcd");
+            put(p,
+              "\r\n"
+              "0\r\n\r\n"
+            );
+        }
+    }
+
+    //--------------------------------------------------------------------------
+
+    template<class Parser, class Pred>
+    void
+    bufgrind(string_view s, Pred&& pred)
+    {
+        using boost::asio::buffer;
+        for(std::size_t n = 1; n < s.size() - 1; ++n)
+        {
+            Parser p;
+            p.eager(true);
+            error_code ec;
+            std::size_t used;
+            used = p.put(buffer(s.data(), n), ec);
+            if(ec == error::need_more)
+                continue;
+            if(! BEAST_EXPECTS(! ec, ec.message()))
+                continue;
+            if(! BEAST_EXPECT(used == n))
+                continue;
+            used = p.put(buffer(
+                s.data() + n, s.size() - n), ec);
+            if(ec == error::need_more)
+                continue;
+            if(! BEAST_EXPECTS(! ec, ec.message()))
+                continue;
+            if(! BEAST_EXPECT(used == s.size() -n))
+                continue;
+            if(! BEAST_EXPECT(p.is_done()))
+                continue;
+            pred(p);
+        }
+    }
+
+    void
+    testBufGrind()
+    {
+        bufgrind<test_parser<false>>(
+            "HTTP/1.1 200 OK\r\n"
+            "Transfer-Encoding: chunked\r\n"
+            "Content-Type: application/octet-stream\r\n"
+            "\r\n"
+            "4\r\nabcd\r\n"
+            "0\r\n\r\n"
+            ,[&](test_parser<false> const& p)
+            {
+                BEAST_EXPECT(p.body == "abcd");
+            });
+        bufgrind<test_parser<false>>(
+            "HTTP/1.1 200 OK\r\n"
+            "Server: test\r\n"
+            "Expect: Expires, MD5-Fingerprint\r\n"
+            "Transfer-Encoding: chunked\r\n"
+            "\r\n"
+            "5\r\n"
+            "*****\r\n"
+            "2;a;b=1;c=\"2\"\r\n"
+            "--\r\n"
+            "0;d;e=3;f=\"4\"\r\n"
+            "Expires: never\r\n"
+            "MD5-Fingerprint: -\r\n"
+            "\r\n"
+            ,[&](test_parser<false> const& p)
+            {
+                BEAST_EXPECT(p.body == "*****--");
+            });
+    }
+
+    //--------------------------------------------------------------------------
+
     void
     run() override
     {
@@ -956,6 +1066,8 @@ public:
         testUpgradeField();
         testBody();
         testSplit();
+        testRegression430();
+        testBufGrind();
     }
 };
 

test/http/read.cpp

@@ -363,9 +363,86 @@ public:
         }
     }
 
+    // https://github.com/vinniefalco/Beast/issues/430
+    void
+    testRegression430()
+    {
+        test::pipe c{ios_};
+        c.server.read_size(1);
+        ostream(c.server.buffer) <<
+          "HTTP/1.1 200 OK\r\n"
+          "Transfer-Encoding: chunked\r\n"
+          "Content-Type: application/octet-stream\r\n"
+          "\r\n"
+          "4\r\nabcd\r\n"
+          "0\r\n\r\n";
+        error_code ec;
+        flat_buffer fb;
+        parser<false, dynamic_body> p;
+        read(c.server, fb, p, ec);
+        BEAST_EXPECTS(! ec, ec.message());
+    }
+
+    //--------------------------------------------------------------------------
+
+    template<class Parser, class Pred>
+    void
+    readgrind(string_view s, Pred&& pred)
+    {
+        using boost::asio::buffer;
+        for(std::size_t n = 1; n < s.size() - 1; ++n)
+        {
+            Parser p;
+            error_code ec;
+            flat_buffer b;
+            test::pipe c{ios_};
+            ostream(c.server.buffer) << s;
+            c.server.read_size(n);
+            read(c.server, b, p, ec);
+            if(! BEAST_EXPECTS(! ec, ec.message()))
+                continue;
+            pred(p);
+        }
+    }
+
+    void
+    testReadGrind()
+    {
+        readgrind<test_parser<false>>(
+            "HTTP/1.1 200 OK\r\n"
+            "Transfer-Encoding: chunked\r\n"
+            "Content-Type: application/octet-stream\r\n"
+            "\r\n"
+            "4\r\nabcd\r\n"
+            "0\r\n\r\n"
+            ,[&](test_parser<false> const& p)
+            {
+                BEAST_EXPECT(p.body == "abcd");
+            });
+        readgrind<test_parser<false>>(
+            "HTTP/1.1 200 OK\r\n"
+            "Server: test\r\n"
+            "Expect: Expires, MD5-Fingerprint\r\n"
+            "Transfer-Encoding: chunked\r\n"
+            "\r\n"
+            "5\r\n"
+            "*****\r\n"
+            "2;a;b=1;c=\"2\"\r\n"
+            "--\r\n"
+            "0;d;e=3;f=\"4\"\r\n"
+            "Expires: never\r\n"
+            "MD5-Fingerprint: -\r\n"
+            "\r\n"
+            ,[&](test_parser<false> const& p)
+            {
+                BEAST_EXPECT(p.body == "*****--");
+            });
+    }
+
     void
     run() override
     {
+#if 0
         testThrow();
         testBufferOverflow();
 
@@ -377,6 +454,9 @@ public:
             testEof(yield); });
 
         testIoService();
+        testRegression430();
+#endif
+        testReadGrind();
     }
 };