From cbb47a0ffd0adac73c410519725ec32dbb34cbd4 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Thu, 27 Jul 2017 17:36:06 -0700 Subject: [PATCH] Fix websocket read of zero length message fix #686 --- CHANGELOG.md | 6 + include/boost/beast/websocket/impl/read.ipp | 339 ++++++++++---------- test/websocket/stream.cpp | 10 + 3 files changed, 190 insertions(+), 165 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 037b5c26..593cd9cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +Version 90: + +* Fix websocket read of zero length message + +-------------------------------------------------------------------------------- + Version 89: * Fix CONTRIBUTING.md links diff --git a/include/boost/beast/websocket/impl/read.ipp b/include/boost/beast/websocket/impl/read.ipp index 123c4529..f2a21bdf 100644 --- a/include/boost/beast/websocket/impl/read.ipp +++ b/include/boost/beast/websocket/impl/read.ipp @@ -233,7 +233,8 @@ operator()( // Empty non-final frame goto go_loop; } - ws_.rd_.done = false; + ws_.rd_.done = + ws_.rd_.remain == 0 && ws_.rd_.fh.fin; break; } @@ -436,6 +437,8 @@ operator()( case do_maybe_fill: if(ec) break; + if(ws_.rd_.done) + break; dispatched_ = true; go_maybe_fill: @@ -495,6 +498,8 @@ operator()( } // Read into caller's buffer step_ = do_read; + BOOST_ASSERT(ws_.rd_.remain > 0); + BOOST_ASSERT(buffer_size(cb_) > 0); return ws_.stream_.async_read_some(buffer_prefix( clamp(ws_.rd_.remain), cb_), std::move(*this)); @@ -526,8 +531,8 @@ operator()( } go_done: - if(ws_.rd_.remain == 0 && ws_.rd_.fh.fin) - ws_.rd_.done = true; + ws_.rd_.done = + ws_.rd_.remain == 0 && ws_.rd_.fh.fin; break; case do_inflate: @@ -1087,203 +1092,207 @@ loop: // Empty non-final frame goto loop; } - rd_.done = false; + rd_.done = rd_.remain == 0 && rd_.fh.fin; } else { ec.assign(0, ec.category()); } - if(! pmd_ || ! pmd_->rd_set) + if( ! rd_.done) { - if(rd_.buf.size() == 0 && rd_.buf.max_size() > - (std::min)(clamp(rd_.remain), - buffer_size(buffers))) + if(! pmd_ || ! pmd_->rd_set) { - // Fill the read buffer first, otherwise we - // get fewer bytes at the cost of one I/O. - auto const mb = rd_.buf.prepare( - read_size(rd_.buf, rd_.buf.max_size())); - auto const bytes_transferred = - stream_.read_some(mb, ec); - failed_ = !!ec; - if(failed_) - return bytes_written; - if(rd_.fh.mask) - detail::mask_inplace(buffer_prefix( - clamp(rd_.remain), mb), rd_.key); - rd_.buf.commit(bytes_transferred); - } - if(rd_.buf.size() > 0) - { - // Copy from the read buffer. - // The mask was already applied. - auto const bytes_transferred = - buffer_copy(buffers, rd_.buf.data(), - clamp(rd_.remain)); - auto const mb = buffer_prefix( - bytes_transferred, buffers); - rd_.remain -= bytes_transferred; - if(rd_.op == detail::opcode::text) + if(rd_.buf.size() == 0 && rd_.buf.max_size() > + (std::min)(clamp(rd_.remain), + buffer_size(buffers))) { - if(! rd_.utf8.write(mb) || - (rd_.remain == 0 && rd_.fh.fin && - ! rd_.utf8.finish())) - { - code = close_code::bad_payload; - goto do_close; - } + // Fill the read buffer first, otherwise we + // get fewer bytes at the cost of one I/O. + auto const mb = rd_.buf.prepare( + read_size(rd_.buf, rd_.buf.max_size())); + auto const bytes_transferred = + stream_.read_some(mb, ec); + failed_ = !!ec; + if(failed_) + return bytes_written; + if(rd_.fh.mask) + detail::mask_inplace(buffer_prefix( + clamp(rd_.remain), mb), rd_.key); + rd_.buf.commit(bytes_transferred); } - bytes_written += bytes_transferred; - rd_.size += bytes_transferred; - rd_.buf.consume(bytes_transferred); + if(rd_.buf.size() > 0) + { + // Copy from the read buffer. + // The mask was already applied. + auto const bytes_transferred = + buffer_copy(buffers, rd_.buf.data(), + clamp(rd_.remain)); + auto const mb = buffer_prefix( + bytes_transferred, buffers); + rd_.remain -= bytes_transferred; + if(rd_.op == detail::opcode::text) + { + if(! rd_.utf8.write(mb) || + (rd_.remain == 0 && rd_.fh.fin && + ! rd_.utf8.finish())) + { + code = close_code::bad_payload; + goto do_close; + } + } + bytes_written += bytes_transferred; + rd_.size += bytes_transferred; + rd_.buf.consume(bytes_transferred); + } + else + { + // Read into caller's buffer + BOOST_ASSERT(rd_.remain > 0); + BOOST_ASSERT(buffer_size(buffers) > 0); + auto const bytes_transferred = + stream_.read_some(buffer_prefix( + clamp(rd_.remain), buffers), ec); + failed_ = !!ec; + if(failed_) + return bytes_written; + BOOST_ASSERT(bytes_transferred > 0); + auto const mb = buffer_prefix( + bytes_transferred, buffers); + rd_.remain -= bytes_transferred; + if(rd_.fh.mask) + detail::mask_inplace(mb, rd_.key); + if(rd_.op == detail::opcode::text) + { + if(! rd_.utf8.write(mb) || + (rd_.remain == 0 && rd_.fh.fin && + ! rd_.utf8.finish())) + { + code = close_code::bad_payload; + goto do_close; + } + } + bytes_written += bytes_transferred; + rd_.size += bytes_transferred; + } + rd_.done = rd_.remain == 0 && rd_.fh.fin; } else { - // Read into caller's buffer - auto const bytes_transferred = - stream_.read_some(buffer_prefix( - clamp(rd_.remain), buffers), ec); - failed_ = !!ec; - if(failed_) - return bytes_written; - BOOST_ASSERT(bytes_transferred > 0); - auto const mb = buffer_prefix( - bytes_transferred, buffers); - rd_.remain -= bytes_transferred; - if(rd_.fh.mask) - detail::mask_inplace(mb, rd_.key); - if(rd_.op == detail::opcode::text) + // Read compressed message frame payload: + // inflate even if rd_.fh.len == 0, otherwise we + // never emit the end-of-stream deflate block. + // + bool did_read = false; + consuming_buffers cb{buffers}; + while(buffer_size(cb) > 0) { - if(! rd_.utf8.write(mb) || - (rd_.remain == 0 && rd_.fh.fin && - ! rd_.utf8.finish())) + zlib::z_params zs; { - code = close_code::bad_payload; - goto do_close; + auto const out = buffer_front(cb); + zs.next_out = buffer_cast(out); + zs.avail_out = buffer_size(out); + BOOST_ASSERT(zs.avail_out > 0); } - } - bytes_written += bytes_transferred; - rd_.size += bytes_transferred; - } - if(rd_.remain == 0 && rd_.fh.fin) - rd_.done = true; - } - else - { - // Read compressed message frame payload: - // inflate even if rd_.fh.len == 0, otherwise we - // never emit the end-of-stream deflate block. - // - bool did_read = false; - consuming_buffers cb{buffers}; - while(buffer_size(cb) > 0) - { - zlib::z_params zs; - { - auto const out = buffer_front(cb); - zs.next_out = buffer_cast(out); - zs.avail_out = buffer_size(out); - BOOST_ASSERT(zs.avail_out > 0); - } - if(rd_.remain > 0) - { - if(rd_.buf.size() > 0) + if(rd_.remain > 0) { - // use what's there - auto const in = buffer_prefix( - clamp(rd_.remain), buffer_front( - rd_.buf.data())); - zs.avail_in = buffer_size(in); - zs.next_in = buffer_cast(in); + if(rd_.buf.size() > 0) + { + // use what's there + auto const in = buffer_prefix( + clamp(rd_.remain), buffer_front( + rd_.buf.data())); + zs.avail_in = buffer_size(in); + zs.next_in = buffer_cast(in); + } + else if(! did_read) + { + // read new + auto const bytes_transferred = + stream_.read_some( + rd_.buf.prepare(read_size( + rd_.buf, rd_.buf.max_size())), + ec); + failed_ = !!ec; + if(failed_) + return bytes_written; + BOOST_ASSERT(bytes_transferred > 0); + rd_.buf.commit(bytes_transferred); + if(rd_.fh.mask) + detail::mask_inplace( + buffer_prefix(clamp(rd_.remain), + rd_.buf.mutable_data()), rd_.key); + auto const in = buffer_prefix( + clamp(rd_.remain), buffer_front( + rd_.buf.data())); + zs.avail_in = buffer_size(in); + zs.next_in = buffer_cast(in); + did_read = true; + } + else + { + break; + } } - else if(! did_read) + else if(rd_.fh.fin) { - // read new - auto const bytes_transferred = - stream_.read_some( - rd_.buf.prepare(read_size( - rd_.buf, rd_.buf.max_size())), - ec); + // append the empty block codes + static std::uint8_t constexpr + empty_block[4] = { + 0x00, 0x00, 0xff, 0xff }; + zs.next_in = empty_block; + zs.avail_in = sizeof(empty_block); + pmd_->zi.write(zs, zlib::Flush::sync, ec); + BOOST_ASSERT(! ec); failed_ = !!ec; if(failed_) return bytes_written; - BOOST_ASSERT(bytes_transferred > 0); - rd_.buf.commit(bytes_transferred); - if(rd_.fh.mask) - detail::mask_inplace( - buffer_prefix(clamp(rd_.remain), - rd_.buf.mutable_data()), rd_.key); - auto const in = buffer_prefix( - clamp(rd_.remain), buffer_front( - rd_.buf.data())); - zs.avail_in = buffer_size(in); - zs.next_in = buffer_cast(in); - did_read = true; + // VFALCO See: + // https://github.com/madler/zlib/issues/280 + BOOST_ASSERT(zs.total_out == 0); + cb.consume(zs.total_out); + rd_.size += zs.total_out; + bytes_written += zs.total_out; + if( + (role_ == role_type::client && + pmd_config_.server_no_context_takeover) || + (role_ == role_type::server && + pmd_config_.client_no_context_takeover)) + pmd_->zi.reset(); + rd_.done = true; + break; } else { break; } - } - else if(rd_.fh.fin) - { - // append the empty block codes - static std::uint8_t constexpr - empty_block[4] = { - 0x00, 0x00, 0xff, 0xff }; - zs.next_in = empty_block; - zs.avail_in = sizeof(empty_block); pmd_->zi.write(zs, zlib::Flush::sync, ec); - BOOST_ASSERT(! ec); + BOOST_ASSERT(ec != zlib::error::end_of_stream); failed_ = !!ec; if(failed_) return bytes_written; - // VFALCO See: - // https://github.com/madler/zlib/issues/280 - BOOST_ASSERT(zs.total_out == 0); + if(rd_msg_max_ && beast::detail::sum_exceeds( + rd_.size, zs.total_out, rd_msg_max_)) + { + code = close_code::too_big; + goto do_close; + } cb.consume(zs.total_out); rd_.size += zs.total_out; + rd_.remain -= zs.total_in; + rd_.buf.consume(zs.total_in); bytes_written += zs.total_out; - if( - (role_ == role_type::client && - pmd_config_.server_no_context_takeover) || - (role_ == role_type::server && - pmd_config_.client_no_context_takeover)) - pmd_->zi.reset(); - rd_.done = true; - break; } - else + if(rd_.op == detail::opcode::text) { - break; - } - pmd_->zi.write(zs, zlib::Flush::sync, ec); - BOOST_ASSERT(ec != zlib::error::end_of_stream); - failed_ = !!ec; - if(failed_) - return bytes_written; - if(rd_msg_max_ && beast::detail::sum_exceeds( - rd_.size, zs.total_out, rd_msg_max_)) - { - code = close_code::too_big; - goto do_close; - } - cb.consume(zs.total_out); - rd_.size += zs.total_out; - rd_.remain -= zs.total_in; - rd_.buf.consume(zs.total_in); - bytes_written += zs.total_out; - } - if(rd_.op == detail::opcode::text) - { - // check utf8 - if(! rd_.utf8.write( - buffer_prefix(bytes_written, buffers)) || ( - rd_.remain == 0 && rd_.fh.fin && - ! rd_.utf8.finish())) - { - code = close_code::bad_payload; - goto do_close; + // check utf8 + if(! rd_.utf8.write( + buffer_prefix(bytes_written, buffers)) || ( + rd_.remain == 0 && rd_.fh.fin && + ! rd_.utf8.finish())) + { + code = close_code::bad_payload; + goto do_close; + } } } } diff --git a/test/websocket/stream.cpp b/test/websocket/stream.cpp index da2cc6a5..cbe830f9 100644 --- a/test/websocket/stream.cpp +++ b/test/websocket/stream.cpp @@ -1638,6 +1638,16 @@ public: } c.handshake(ws, "localhost", "/"); + // send empty message + ws.text(true); + c.write(ws, boost::asio::null_buffers{}); + { + // receive echoed message + multi_buffer db; + c.read(ws, db); + BEAST_EXPECT(ws.got_text()); + BEAST_EXPECT(db.size() == 0); + } // send message ws.auto_fragment(false); ws.binary(false);