From 3d168c033662830ec53f471cad9c6d3088a9f599 Mon Sep 17 00:00:00 2001 From: Richard Hodges Date: Fri, 24 Jan 2020 11:28:24 +0100 Subject: [PATCH] file body reports short_read on eof fixes #1818 fixes #1815 close #1821 --- CHANGELOG.md | 8 ++ doc/qbk/07_concepts/File.qbk | 5 +- include/boost/beast/http/basic_file_body.hpp | 6 + include/boost/beast/http/error.hpp | 10 +- include/boost/beast/http/impl/error.ipp | 1 + .../boost/beast/http/impl/file_body_win32.hpp | 12 +- test/beast/http/buffer_body.cpp | 1 + test/beast/http/error.cpp | 1 + test/beast/http/file_body.cpp | 108 +++++++++++++++++- 9 files changed, 145 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f34026af..67df4f8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +Version XXX: + +* clarify end-of-file behaviour in File::read docs +* file_body returns short_read on eof during read +* fix bug in win32 file_body + +-------------------------------------------------------------------------------- + Version 283: * ostream_buffer satisfies preconditions of DynamicBuffer_v1::commit diff --git a/doc/qbk/07_concepts/File.qbk b/doc/qbk/07_concepts/File.qbk index cce5ad40..b00ea8c0 100644 --- a/doc/qbk/07_concepts/File.qbk +++ b/doc/qbk/07_concepts/File.qbk @@ -141,7 +141,10 @@ In this table: set `ec` to `errc::invalid_argument` and return immediately. The function will ensure that `!ec` is `true` if there was no error or set to the appropriate error code if an error - occurred. + occurred. + If an end-of-file condition is detected prior to reading any + bytes, the function will ensure that `!ec` is `true` and the + return value shall be 0. ] ] [ diff --git a/include/boost/beast/http/basic_file_body.hpp b/include/boost/beast/http/basic_file_body.hpp index 01eae3c1..ae1351d2 100644 --- a/include/boost/beast/http/basic_file_body.hpp +++ b/include/boost/beast/http/basic_file_body.hpp @@ -363,6 +363,12 @@ get(error_code& ec) -> if(ec) return boost::none; + if (nread == 0) + { + ec = error::short_read; + return boost::none; + } + // Make sure there is forward progress BOOST_ASSERT(nread != 0); BOOST_ASSERT(nread <= remain_); diff --git a/include/boost/beast/http/error.hpp b/include/boost/beast/http/error.hpp index d0c287eb..ab6216b1 100644 --- a/include/boost/beast/http/error.hpp +++ b/include/boost/beast/http/error.hpp @@ -153,7 +153,15 @@ enum class error a new parser for each message. This can be easily done by storing the parser in an boost or std::optional container. */ - stale_parser + stale_parser, + + /** The message body is shorter than expected. + + This error is returned by @ref file_body when an unexpected + unexpected end-of-file condition is encountered while trying + to read from the file. + */ + short_read }; } // http diff --git a/include/boost/beast/http/impl/error.ipp b/include/boost/beast/http/impl/error.ipp index e0167b30..2443c818 100644 --- a/include/boost/beast/http/impl/error.ipp +++ b/include/boost/beast/http/impl/error.ipp @@ -56,6 +56,7 @@ public: case error::bad_chunk_extension: return "bad chunk extension"; case error::bad_obs_fold: return "bad obs-fold"; case error::stale_parser: return "stale parser"; + case error::short_read: return "unexpected eof in body"; default: return "beast.http error"; diff --git a/include/boost/beast/http/impl/file_body_win32.hpp b/include/boost/beast/http/impl/file_body_win32.hpp index a2cc661f..06a315c6 100644 --- a/include/boost/beast/http/impl/file_body_win32.hpp +++ b/include/boost/beast/http/impl/file_body_win32.hpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -133,14 +134,16 @@ struct basic_file_body template writer(header&, value_type& b) : body_(b) + , pos_(body_.first_) { + BOOST_ASSERT(body_.file_.is_open()); } void - init(error_code&) + init(error_code& ec) { BOOST_ASSERT(body_.file_.is_open()); - pos_ = body_.first_; + ec.clear(); } boost::optional> @@ -156,6 +159,11 @@ struct basic_file_body auto const nread = body_.file_.read(buf_, n, ec); if(ec) return boost::none; + if (nread == 0) + { + ec = error::short_read; + return boost::none; + } BOOST_ASSERT(nread != 0); pos_ += nread; ec = {}; diff --git a/test/beast/http/buffer_body.cpp b/test/beast/http/buffer_body.cpp index d4f103b0..14ce1413 100644 --- a/test/beast/http/buffer_body.cpp +++ b/test/beast/http/buffer_body.cpp @@ -44,6 +44,7 @@ public: read_header(ts, fb, p, ec); auto const bytes_transferred = read(ts, fb, p, ec); + boost::ignore_unused(bytes_transferred); BEAST_EXPECTS(! ec, ec.message()); // VFALCO What should the read algorithms return? diff --git a/test/beast/http/error.cpp b/test/beast/http/error.cpp index 37d4784d..8689cd9d 100644 --- a/test/beast/http/error.cpp +++ b/test/beast/http/error.cpp @@ -67,6 +67,7 @@ public: check("beast.http", error::bad_obs_fold); check("beast.http", error::stale_parser); + check("beast.http", error::short_read); } }; diff --git a/test/beast/http/file_body.cpp b/test/beast/http/file_body.cpp index f8a04c5f..119663b0 100644 --- a/test/beast/http/file_body.cpp +++ b/test/beast/http/file_body.cpp @@ -18,6 +18,7 @@ #include #include #include +#include namespace boost { namespace beast { @@ -40,6 +41,43 @@ public: } }; + struct temp_file + { + temp_file(std::ostream& logger) + : path_(boost::filesystem::unique_path()) + , log_(logger) + {} + + ~temp_file() + { + if (!path_.empty()) + { + auto ec = error_code(); + boost::filesystem::remove(path_, ec); + if (ec) + { + log_ << "warning: failed to remove temporary file: " << path_ << "\n"; + } + } + + } + + temp_file(temp_file&&) = default; + temp_file(temp_file const&) = delete; + temp_file& operator=(temp_file&&) = default; + temp_file& operator=(temp_file const&) = delete; + + boost::filesystem::path const& path() const + { + return path_; + } + + private: + + boost::filesystem::path path_; + std::ostream& log_; + }; + template void doTestFileBody() @@ -98,16 +136,80 @@ public: boost::filesystem::remove(temp, ec); BEAST_EXPECTS(! ec, ec.message()); } + + template + void + fileBodyUnexpectedEofOnGet() + { + auto temp = temp_file(log); + + error_code ec; + string_view const ten = + "0123456789"; // 40 + // create the temporary file + { + std::ofstream fstemp(temp.path().native()); + std::size_t written = 0; + std::size_t to_write = 4097; + while (written < to_write) + { + auto size = std::min(ten.size(), to_write - written); + fstemp << ten.substr(0, size); + BEAST_EXPECT(fstemp.good()); + written += size; + } + fstemp.close(); + } + + // open the file and read the header + { + using file_body_type = basic_file_body; + + typename file_body_type::value_type value; + // opened in write mode so we can truncate later + value.open(temp.path().string().c_str(), file_mode::read, ec); + BEAST_EXPECTS(! ec, ec.message()); + + response_header<> header; + header.version(11); + header.result(status::accepted); + header.set(field::server, "test"); + header.set(field::content_length, 4097); + + typename file_body_type::writer w(header, value); + auto maybe_range = w.get(ec); + BEAST_EXPECTS(!ec, ec.message()); + BEAST_EXPECTS(maybe_range.has_value(), "no range returned"); + BEAST_EXPECT(maybe_range.value().second); + + value.file().seek(4097, ec); + BEAST_EXPECTS(!ec, ec.message()); + + maybe_range = w.get(ec); + BEAST_EXPECTS(ec == error::short_read, ec.message()); + BEAST_EXPECTS(!maybe_range.has_value(), "range returned on error"); + } + } + void run() override { doTestFileBody(); - #if BOOST_BEAST_USE_WIN32_FILE +#if BOOST_BEAST_USE_WIN32_FILE doTestFileBody(); - #endif - #if BOOST_BEAST_USE_POSIX_FILE +#endif +#if BOOST_BEAST_USE_POSIX_FILE doTestFileBody(); +#endif + + fileBodyUnexpectedEofOnGet(); + #if BOOST_BEAST_USE_POSIX_FILE + fileBodyUnexpectedEofOnGet(); #endif + #if BOOST_BEAST_USE_WIN32_FILE + fileBodyUnexpectedEofOnGet(); + #endif + } };