file body reports short_read on eof

fixes #1818
fixes #1815
close #1821

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

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.
     ]
 ]
 [

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_);

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

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";

include/boost/beast/http/impl/file_body_win32.hpp

@@ -17,6 +17,7 @@
 #include <boost/beast/core/buffers_range.hpp>
 #include <boost/beast/core/detail/clamp.hpp>
 #include <boost/beast/core/detail/is_invocable.hpp>
+#include <boost/beast/http/error.hpp>
 #include <boost/beast/http/write.hpp>
 #include <boost/beast/http/serializer.hpp>
 #include <boost/asio/async_result.hpp>
@@ -133,14 +134,16 @@ struct basic_file_body<file_win32>
         template<bool isRequest, class Fields>
         writer(header<isRequest, Fields>&, 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<std::pair<const_buffers_type, bool>>
@@ -156,6 +159,11 @@ struct basic_file_body<file_win32>
             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 = {};

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?

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);
     }
 };
 

test/beast/http/file_body.cpp

@@ -18,6 +18,7 @@
 #include <boost/beast/http/serializer.hpp>
 #include <boost/beast/_experimental/unit_test/suite.hpp>
 #include <boost/filesystem.hpp>
+#include <boost/asio/error.hpp>
 
 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<class File>
     void
     doTestFileBody()
@@ -98,16 +136,80 @@ public:
         boost::filesystem::remove(temp, ec);
         BEAST_EXPECTS(! ec, ec.message());
     }
+
+    template<class File>
+    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<File>;
+
+            typename file_body_type::value_type value;
+            // opened in write mode so we can truncate later
+            value.open(temp.path().string<std::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<file_stdio>();
-    #if BOOST_BEAST_USE_WIN32_FILE
+#if BOOST_BEAST_USE_WIN32_FILE
         doTestFileBody<file_win32>();
-    #endif
-    #if BOOST_BEAST_USE_POSIX_FILE
+#endif
+#if BOOST_BEAST_USE_POSIX_FILE
         doTestFileBody<file_posix>();
+#endif
+
+        fileBodyUnexpectedEofOnGet<file_stdio>();
+    #if BOOST_BEAST_USE_POSIX_FILE
+        fileBodyUnexpectedEofOnGet<file_posix>();
     #endif
+    #if BOOST_BEAST_USE_WIN32_FILE
+        fileBodyUnexpectedEofOnGet<file_win32>();
+    #endif
+
     }
 };