HttpServer::ParseHeaders: don't DCHECK on bogus headers termination

net/server/http_server_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/server/http_server.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 115 matching lines @@
       return false;
     while (!IsCompleteResponse(*message)) {
       std::string chunk;
       if (!Read(&chunk, 1))
         return false;
       message->append(chunk);
     }
     return true;
   }
 
+  bool TestUsedThenDisconnectedWithNoData() {

[mmenke, 2017/01/20 18:20:57]

Rather than a bool, suggest:

void ExpectUsedThenDisconnectedWithNoData() {
  ASSERT...
  ASSERT...
  ASSERT...
}

Failures will then tell us which check failed.

[Maks Orlovich, 2017/01/20 19:00:20]

Done.

+    // Check that the socket was opened...
+    if (!socket_->WasEverUsed()) {
+      return false;
+    }

[mmenke, 2017/01/20 18:20:57]

nit:  No braces when if + body take up one line each.

[Maks Orlovich, 2017/01/20 19:00:20]

Made moot by above.

+
+    // ...then closed when the server disconnected. Verify that the socket was
+    // closed by checking that a Read() fails.
+    std::string response;
+    if (Read(&response, 1u)) {
+      return false;
+    }

[mmenke, 2017/01/20 18:20:57]

nit:  --braces

[Maks Orlovich, 2017/01/20 19:00:20]

Likewise

+
+    return response.empty();
+  }
+
   TCPClientSocket& socket() { return *socket_; }
 
  private:
   void OnConnect(const base::Closure& quit_loop, int result) {
     connect_result_ = result;
     quit_loop.Run();
   }
 
   void Write() {
     int result = socket_->Write(
@@ skipping 38 matching lines @@
   scoped_refptr<DrainableIOBuffer> write_buffer_;
   std::unique_ptr<TCPClientSocket> socket_;
   int connect_result_;
 };
 
 }  // namespace
 
 class HttpServerTest : public testing::Test,
                        public HttpServer::Delegate {
  public:
-  HttpServerTest() : quit_after_request_count_(0) {}
+  HttpServerTest()
+      : quit_after_request_count_(0), quit_on_close_connection_(-1) {}
 
   void SetUp() override {
     std::unique_ptr<ServerSocket> server_socket(
         new TCPServerSocket(NULL, NetLogSource()));
     server_socket->ListenWithAddressAndPort("127.0.0.1", 0, 1);
     server_.reset(new HttpServer(std::move(server_socket), this));
     ASSERT_THAT(server_->GetLocalAddress(&server_address_), IsOk());
   }
 
+  void TearDown() override {
+    // We need to run the event loop some to make sure that the memory

[mmenke, 2017/01/20 18:20:57]

nit:  Avoid "we" in comments - it's a bit ambiguous who "we" is (The code
authors?  The committers?  The user?  etc.)

[Maks Orlovich, 2017/01/20 19:00:20]

Done.

+    // handed over to DeleteSoon gets fully freed.
+    base::RunLoop().RunUntilIdle();
+  }
+
   void OnConnect(int connection_id) override {
     DCHECK(connection_map_.find(connection_id) == connection_map_.end());
     connection_map_[connection_id] = true;
   }
 
   void OnHttpRequest(int connection_id,
                      const HttpServerRequestInfo& info) override {
     requests_.push_back(std::make_pair(info, connection_id));
     if (requests_.size() == quit_after_request_count_)
       run_loop_quit_func_.Run();
   }
 
   void OnWebSocketRequest(int connection_id,
                           const HttpServerRequestInfo& info) override {
     NOTREACHED();
   }
 
   void OnWebSocketMessage(int connection_id, const std::string& data) override {
     NOTREACHED();
   }
 
   void OnClose(int connection_id) override {
     DCHECK(connection_map_.find(connection_id) != connection_map_.end());
     connection_map_[connection_id] = false;
+    if (connection_id == quit_on_close_connection_) {
+      run_loop_quit_func_.Run();
+    }

[mmenke, 2017/01/20 18:20:57]

nit:  --braces

[Maks Orlovich, 2017/01/20 19:00:20]

Done.

   }
 
   bool RunUntilRequestsReceived(size_t count) {
     quit_after_request_count_ = count;
     if (requests_.size() == count)
       return true;
 
     base::RunLoop run_loop;
     run_loop_quit_func_ = run_loop.QuitClosure();
     bool success = RunLoopWithTimeout(&run_loop);
     run_loop_quit_func_.Reset();
     return success;
   }
 
+  bool RunUntilConnectionIdClosed(int connection_id) {
+    quit_on_close_connection_ = connection_id;
+    auto iter = connection_map_.find(connection_id);
+    if (iter != connection_map_.end() && !iter->second) {
+      // Already disconnected

[mmenke, 2017/01/20 18:20:57]

nit:  Predominant style is to send more comment-y comments with periods (As
opposed to just label-y comments)

[Maks Orlovich, 2017/01/20 19:00:20]

Done.

+      return true;
+    }
+
+    base::RunLoop run_loop;
+    run_loop_quit_func_ = run_loop.QuitClosure();
+    bool success = RunLoopWithTimeout(&run_loop);
+    run_loop_quit_func_.Reset();
+    return success;
+  }
+
   HttpServerRequestInfo GetRequest(size_t request_index) {
     return requests_[request_index].first;
   }
 
+  size_t num_requests() const { return requests_.size(); }
+
   int GetConnectionId(size_t request_index) {
     return requests_[request_index].second;
   }
 
   void HandleAcceptResult(std::unique_ptr<StreamSocket> socket) {
     server_->accepted_socket_ = std::move(socket);
     server_->HandleAcceptResult(OK);
   }
 
   std::unordered_map<int, bool>& connection_map() { return connection_map_; }
 
  protected:
   std::unique_ptr<HttpServer> server_;
   IPEndPoint server_address_;
   base::Closure run_loop_quit_func_;
   std::vector<std::pair<HttpServerRequestInfo, int> > requests_;
   std::unordered_map<int /* connection_id */, bool /* connected */>
       connection_map_;
 
  private:
   size_t quit_after_request_count_;
+  int quit_on_close_connection_;
 };
 
 namespace {
 
 class WebSocketTest : public HttpServerTest {
   void OnHttpRequest(int connection_id,
                      const HttpServerRequestInfo& info) override {
     NOTREACHED();
   }
 
@@ skipping 12 matching lines @@
   client.Send("GET /test HTTP/1.1\r\n\r\n");
   ASSERT_TRUE(RunUntilRequestsReceived(1));
   ASSERT_EQ("GET", GetRequest(0).method);
   ASSERT_EQ("/test", GetRequest(0).path);
   ASSERT_EQ("", GetRequest(0).data);
   ASSERT_EQ(0u, GetRequest(0).headers.size());
   ASSERT_TRUE(base::StartsWith(GetRequest(0).peer.ToString(), "127.0.0.1",
                                base::CompareCase::SENSITIVE));
 }
 
+TEST_F(HttpServerTest, RequestBrokenTermination) {
+  TestHttpClient client;
+  ASSERT_THAT(client.ConnectAndWait(server_address_), IsOk());
+  client.Send("GET /test HTTP/1.1\r\n\r)");
+  ASSERT_TRUE(RunUntilConnectionIdClosed(1));
+  EXPECT_EQ(0u, num_requests());
+  EXPECT_TRUE(client.TestUsedThenDisconnectedWithNoData());
+}
+
 TEST_F(HttpServerTest, RequestWithHeaders) {
   TestHttpClient client;
   ASSERT_THAT(client.ConnectAndWait(server_address_), IsOk());
   const char* const kHeaders[][3] = {
       {"Header", ": ", "1"},
       {"HeaderWithNoWhitespace", ":", "1"},
       {"HeaderWithWhitespace", "   :  \t   ", "1 1 1 \t  "},
       {"HeaderWithColon", ": ", "1:1"},
       {"EmptyHeader", ":", ""},
       {"EmptyHeaderWithWhitespace", ":  \t  ", ""},
@@ skipping 187 matching lines @@
     TestHttpClient client;
     ASSERT_THAT(client.ConnectAndWait(server_address_), IsOk());
 
     client.Send(kBadProtocolRequests[i]);
     ASSERT_FALSE(RunUntilRequestsReceived(1));
 
     // Assert that the delegate was updated properly.
     ASSERT_EQ(1u, connection_map().size());
     ASSERT_FALSE(connection_map().begin()->second);
 
-    // Assert that the socket was opened...
-    ASSERT_TRUE(client.socket().WasEverUsed());
-
-    // ...then closed when the server disconnected. Verify that the socket was
-    // closed by checking that a Read() fails.
-    std::string response;
-    ASSERT_FALSE(client.Read(&response, 1u));
-    ASSERT_EQ(std::string(), response);
+    ASSERT_TRUE(client.TestUsedThenDisconnectedWithNoData());
 
     // Reset the state of the connection map.
     connection_map().clear();
   }
 }
 
 class MockStreamSocket : public StreamSocket {
  public:
   MockStreamSocket()
       : connected_(true),
@@ skipping 175 matching lines @@
   ASSERT_THAT(client.ConnectAndWait(server_address_), IsOk());
   client.Send("GET / HTTP/1.1\r\n\r\n");
   ASSERT_FALSE(RunUntilRequestsReceived(1));
   ASSERT_EQ(1ul, connection_ids_.size());
   ASSERT_EQ(0ul, requests_.size());
 }
 
 }  // namespace
 
 }  // namespace net