DNS: Don't spin on unexpected EOF reading TCP response

DnsTCPAttempt doesn't handle when read returns 0, which is EOF.
It keeps reading over and over, which will peg the CPU.

Fix it so it stops reading with ERR_CONNECTION_CLOSED,
and add unit tests for both ways the connection can close.

BUG=393923

Review URL: https://codereview.chromium.org/422323004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287456 0039d316-1c4b-4281-b951-d872f2087c98

net/dns/dns_transaction.cc

@@ -339,7 +339,9 @@ class DnsTCPAttempt : public DnsAttempt {
     STATE_SEND_LENGTH,
     STATE_SEND_QUERY,
     STATE_READ_LENGTH,
+    STATE_READ_LENGTH_COMPLETE,
     STATE_READ_RESPONSE,
+    STATE_READ_RESPONSE_COMPLETE,
     STATE_NONE,
   };
 
@@ -362,9 +364,15 @@ class DnsTCPAttempt : public DnsAttempt {
         case STATE_READ_LENGTH:
           rv = DoReadLength(rv);
           break;
+        case STATE_READ_LENGTH_COMPLETE:
+          rv = DoReadLengthComplete(rv);
+          break;
         case STATE_READ_RESPONSE:
           rv = DoReadResponse(rv);
           break;
+        case STATE_READ_RESPONSE_COMPLETE:
+          rv = DoReadResponseComplete(rv);
+          break;
         default:
           NOTREACHED();
           break;
@@ -435,18 +443,25 @@ class DnsTCPAttempt : public DnsAttempt {
   }
 
   int DoReadLength(int rv) {
+    DCHECK_EQ(OK, rv);
+
+    next_state_ = STATE_READ_LENGTH_COMPLETE;
+    return ReadIntoBuffer();
+  }
+
+  int DoReadLengthComplete(int rv) {
     DCHECK_NE(ERR_IO_PENDING, rv);
     if (rv < 0)
       return rv;
+    if (rv == 0)
+      return ERR_CONNECTION_CLOSED;
 
     buffer_->DidConsume(rv);
     if (buffer_->BytesRemaining() > 0) {
       next_state_ = STATE_READ_LENGTH;
-      return socket_->Read(
-          buffer_.get(),
-          buffer_->BytesRemaining(),
-          base::Bind(&DnsTCPAttempt::OnIOComplete, base::Unretained(this)));
+      return OK;
     }
+
     base::ReadBigEndian<uint16>(length_buffer_->data(), &response_length_);
     // Check if advertised response is too short. (Optimization only.)
     if (response_length_ < query_->io_buffer()->size())
@@ -459,18 +474,25 @@ class DnsTCPAttempt : public DnsAttempt {
   }
 
   int DoReadResponse(int rv) {
+    DCHECK_EQ(OK, rv);
+
+    next_state_ = STATE_READ_RESPONSE_COMPLETE;
+    return ReadIntoBuffer();
+  }
+
+  int DoReadResponseComplete(int rv) {
     DCHECK_NE(ERR_IO_PENDING, rv);
     if (rv < 0)
       return rv;
+    if (rv == 0)
+      return ERR_CONNECTION_CLOSED;
 
     buffer_->DidConsume(rv);
     if (buffer_->BytesRemaining() > 0) {
       next_state_ = STATE_READ_RESPONSE;
-      return socket_->Read(
-          buffer_.get(),
-          buffer_->BytesRemaining(),
-          base::Bind(&DnsTCPAttempt::OnIOComplete, base::Unretained(this)));
+      return OK;
     }
+
     if (!response_->InitParse(buffer_->BytesConsumed(), *query_))
       return ERR_DNS_MALFORMED_RESPONSE;
     if (response_->flags() & dns_protocol::kFlagTC)
@@ -490,6 +512,13 @@ class DnsTCPAttempt : public DnsAttempt {
       callback_.Run(rv);
   }
 
+  int ReadIntoBuffer() {
+    return socket_->Read(
+        buffer_.get(),
+        buffer_->BytesRemaining(),
+        base::Bind(&DnsTCPAttempt::OnIOComplete, base::Unretained(this)));
+  }
+
   State next_state_;
   base::TimeTicks start_time_;
 

net/dns/dns_transaction_unittest.cc

@@ -100,6 +100,11 @@ class DnsSocketData {
     AddResponse(response.Pass(), mode);
   }
 
+  // Add error response.
+  void AddReadError(int error, IoMode mode) {
+    reads_.push_back(MockRead(mode, error));
+  }
+
   // Build, if needed, and return the SocketDataProvider. No new responses
   // should be added afterwards.
   SocketDataProvider* GetProvider() {
@@ -902,6 +907,9 @@ TEST_F(DnsTransactionTest, TCPMalformed) {
   scoped_ptr<DnsSocketData> data(
       new DnsSocketData(0 /* id */, kT0HostName, kT0Qtype, ASYNC, true));
   // Valid response but length too short.
+  // This must be truncated in the question section. The DnsResponse doesn't
+  // examine the answer section until asked to parse it, so truncating it in
+  // the answer section would result in the DnsTransaction itself succeeding.
   data->AddResponseWithLength(
       make_scoped_ptr(
           new DnsResponse(reinterpret_cast<const char*>(kT0ResponseDatagram),
@@ -926,6 +934,70 @@ TEST_F(DnsTransactionTest, TCPTimeout) {
   EXPECT_TRUE(helper0.RunUntilDone(transaction_factory_.get()));
 }
 
+TEST_F(DnsTransactionTest, TCPReadReturnsZeroAsync) {
+  AddAsyncQueryAndRcode(kT0HostName, kT0Qtype,
+                        dns_protocol::kRcodeNOERROR | dns_protocol::kFlagTC);
+  scoped_ptr<DnsSocketData> data(
+      new DnsSocketData(0 /* id */, kT0HostName, kT0Qtype, ASYNC, true));
+  // Return all but the last byte of the response.
+  data->AddResponseWithLength(
+      make_scoped_ptr(
+          new DnsResponse(reinterpret_cast<const char*>(kT0ResponseDatagram),
+                          arraysize(kT0ResponseDatagram) - 1, 0)),
+      ASYNC,
+      static_cast<uint16>(arraysize(kT0ResponseDatagram)));
+  // Then return a 0-length read.
+  data->AddReadError(0, ASYNC);
+  AddSocketData(data.Pass());
+
+  TransactionHelper helper0(kT0HostName, kT0Qtype, ERR_CONNECTION_CLOSED);
+  EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
+}
+
+TEST_F(DnsTransactionTest, TCPReadReturnsZeroSynchronous) {
+  AddAsyncQueryAndRcode(kT0HostName, kT0Qtype,
+                        dns_protocol::kRcodeNOERROR | dns_protocol::kFlagTC);
+  scoped_ptr<DnsSocketData> data(
+      new DnsSocketData(0 /* id */, kT0HostName, kT0Qtype, ASYNC, true));
+  // Return all but the last byte of the response.
+  data->AddResponseWithLength(
+      make_scoped_ptr(
+          new DnsResponse(reinterpret_cast<const char*>(kT0ResponseDatagram),
+                          arraysize(kT0ResponseDatagram) - 1, 0)),
+      SYNCHRONOUS,
+      static_cast<uint16>(arraysize(kT0ResponseDatagram)));
+  // Then return a 0-length read.
+  data->AddReadError(0, SYNCHRONOUS);
+  AddSocketData(data.Pass());
+
+  TransactionHelper helper0(kT0HostName, kT0Qtype, ERR_CONNECTION_CLOSED);
+  EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
+}
+
+TEST_F(DnsTransactionTest, TCPConnectionClosedAsync) {
+  AddAsyncQueryAndRcode(kT0HostName, kT0Qtype,
+                        dns_protocol::kRcodeNOERROR | dns_protocol::kFlagTC);
+  scoped_ptr<DnsSocketData> data(
+      new DnsSocketData(0 /* id */, kT0HostName, kT0Qtype, ASYNC, true));
+  data->AddReadError(ERR_CONNECTION_CLOSED, ASYNC);
+  AddSocketData(data.Pass());
+
+  TransactionHelper helper0(kT0HostName, kT0Qtype, ERR_CONNECTION_CLOSED);
+  EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
+}
+
+TEST_F(DnsTransactionTest, TCPConnectionClosedSynchronous) {
+  AddAsyncQueryAndRcode(kT0HostName, kT0Qtype,
+                        dns_protocol::kRcodeNOERROR | dns_protocol::kFlagTC);
+  scoped_ptr<DnsSocketData> data(
+      new DnsSocketData(0 /* id */, kT0HostName, kT0Qtype, ASYNC, true));
+  data->AddReadError(ERR_CONNECTION_CLOSED, SYNCHRONOUS);
+  AddSocketData(data.Pass());
+
+  TransactionHelper helper0(kT0HostName, kT0Qtype, ERR_CONNECTION_CLOSED);
+  EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
+}
+
 TEST_F(DnsTransactionTest, InvalidQuery) {
   config_.timeout = TestTimeouts::tiny_timeout();
   ConfigureFactory();