Jelajahi Sumber

Fail a content provider that makes no progress

write_content_with_progress() advances its offset only by what the provider
writes, so a provider that reported success without writing anything and
without calling done() was handed the same offset and length again on the next
pass. With the peer still connected it spun there, re-entering the provider as
fast as the loop could run.

make_file_body()'s provider was one way to reach this and was fixed in #2566,
but any user-supplied provider can do the same. Treat a pass that makes no
progress as a short body, which is how done() called early is already handled.
yhirose 2 hari lalu
induk
melakukan
c58061ea81
2 mengubah file dengan 48 tambahan dan 0 penghapusan
  1. 11 0
      httplib.h
  2. 37 0
      test/test.cc

+ 11 - 0
httplib.h

@@ -8401,6 +8401,8 @@ inline bool write_content_with_progress(Stream &strm,
   data_sink.done = [&]() { finished = true; };
 
   while (offset < end_offset && !finished && !is_shutting_down()) {
+    auto last_offset = offset;
+
     if (!strm.wait_writable() || !strm.is_peer_alive()) {
       error = Error::Write;
       return false;
@@ -8411,6 +8413,15 @@ inline bool write_content_with_progress(Stream &strm,
       error = Error::Write;
       return false;
     }
+
+    // A provider that reports success without writing anything and without
+    // reporting itself done gets handed the same offset and length again on
+    // the next pass, so it would spin here for as long as the peer stays
+    // connected. Treat making no progress as a short body, like done() early.
+    if (!finished && offset == last_offset) {
+      error = Error::Write;
+      return false;
+    }
   }
 
   if (offset < end_offset) { // done() called early, or is_shutting_down()

+ 37 - 0
test/test.cc

@@ -10948,6 +10948,43 @@ TEST(ClientProblemDetectionTest, ContentProvider) {
   }
 }
 
+TEST(ContentProviderTest, ProviderMakingNoProgressFails) {
+  // A provider that reports success without writing anything and without
+  // calling done() used to be handed the same offset and length again on every
+  // pass, so it spun for as long as the peer stayed connected.
+  Server svr;
+
+  svr.Post("/", [](const Request & /*req*/, Response &res) {
+    res.set_content("ok", "text/plain");
+  });
+
+  auto port = svr.bind_to_any_port(HOST);
+  auto listen_thread = std::thread([&svr] { svr.listen_after_bind(); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    listen_thread.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  std::atomic<int> call_count{0};
+
+  Client cli(HOST, port);
+  auto res = cli.Post(
+      "/", 1024,
+      [&](size_t /*offset*/, size_t /*length*/, DataSink & /*sink*/) {
+        // Give up after enough passes to show the spin, so that losing the
+        // check below fails this test instead of hanging it.
+        return ++call_count < 100;
+      },
+      "text/plain");
+
+  ASSERT_FALSE(res);
+  EXPECT_EQ(Error::Write, res.error());
+  EXPECT_EQ(1, call_count.load());
+}
+
 TEST(DataSinkTest, OptionalCallbacksAreCallableByDefault) {
   // A writer only has to assign `write`. The other three used to be left as
   // empty std::functions, so a provider calling one threw