diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ba340b8..a7d4a151 100644 --- a/CHANGELOG.md +++ b/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: diff --git a/include/beast/http/basic_parser.hpp b/include/beast/http/basic_parser.hpp index c5b5dc90..86525044 100644 --- a/include/beast/http/basic_parser.hpp +++ b/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<< 8; - static unsigned constexpr flagConnectionUpgrade = 1<< 9; - static unsigned constexpr flagConnectionKeepAlive = 1<< 10; - static unsigned constexpr flagContentLength = 1<< 11; - static unsigned constexpr flagChunked = 1<< 12; - static unsigned constexpr flagUpgrade = 1<< 13; + static unsigned constexpr flagConnectionClose = 1<< 7; + static unsigned constexpr flagConnectionUpgrade = 1<< 8; + static unsigned constexpr flagConnectionKeepAlive = 1<< 9; + static unsigned constexpr flagContentLength = 1<< 10; + static unsigned constexpr flagChunked = 1<< 11; + static unsigned constexpr flagUpgrade = 1<< 12; + static unsigned constexpr flagFinalChunk = 1<< 13; std::uint64_t len_; // size of chunk or body std::unique_ptr 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 diff --git a/include/beast/http/impl/basic_parser.ipp b/include/beast/http/impl/basic_parser.ipp index d1131926..00eb3a55 100644 --- a/include/beast/http/impl/basic_parser.ipp +++ b/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 void basic_parser:: -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; - - // 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; + char const* eol; 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 - // extension to validate p here. - auto const ext = make_string(p, term - 2); - impl().on_chunk(v, ext, ec); + // VFALCO TODO Validate extension + impl().on_chunk(v, make_string( + p, eol - 2), 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(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( - pbegin + skip_, pend, ec); + auto eom = find_eom(p0 + skip_, pend, ec); if(ec) return; - if(! term) + if(! eom) { - // VFALCO Is this right? - if(n > 3) - skip_ = (pend - pbegin) - 3; + BOOST_ASSERT(n >= 3); + skip_ = n - 3; ec = error::need_more; return; } if(*p == ';') { - auto const ext = make_string(p, pbegin + x_); - impl().on_chunk(0, ext, ec); + // VFALCO TODO Validate extension + impl().on_chunk(0, make_string( + p, eol - 2), ec); if(ec) return; - p = pbegin + x_; } - if(! parse_crlf(p)) - { - ec = error::bad_chunk; - return; - } - parse_fields(p, term, ec); + p = eol; + parse_fields(p, eom, ec); if(ec) return; - BOOST_ASSERT(p == term); + BOOST_ASSERT(p == eom); + p0 = eom; impl().on_complete(ec); if(ec) diff --git a/test/http/basic_parser.cpp b/test/http/basic_parser.cpp index 1e6b7e98..8a168675 100644 --- a/test/http/basic_parser.cpp +++ b/test/http/basic_parser.cpp @@ -942,6 +942,116 @@ public: #endif } + //-------------------------------------------------------------------------- + + template + void + put(basic_parser& 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 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 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 + 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>( + "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 const& p) + { + BEAST_EXPECT(p.body == "abcd"); + }); + bufgrind>( + "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 const& p) + { + BEAST_EXPECT(p.body == "*****--"); + }); + } + + //-------------------------------------------------------------------------- + void run() override { @@ -956,6 +1066,8 @@ public: testUpgradeField(); testBody(); testSplit(); + testRegression430(); + testBufGrind(); } }; diff --git a/test/http/read.cpp b/test/http/read.cpp index 2cd82a75..91453e59 100644 --- a/test/http/read.cpp +++ b/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 p; + read(c.server, fb, p, ec); + BEAST_EXPECTS(! ec, ec.message()); + } + + //-------------------------------------------------------------------------- + + template + 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>( + "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 const& p) + { + BEAST_EXPECT(p.body == "abcd"); + }); + readgrind>( + "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 const& p) + { + BEAST_EXPECT(p.body == "*****--"); + }); + } + void run() override { +#if 0 testThrow(); testBufferOverflow(); @@ -377,6 +454,9 @@ public: testEof(yield); }); testIoService(); + testRegression430(); +#endif + testReadGrind(); } };