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

net/dns/dns_transaction.cc

Index: net/dns/dns_transaction.cc
diff --git a/net/dns/dns_transaction.cc b/net/dns/dns_transaction.cc
index 130922c6025620f821e6662d5ebfb318da00fb89..65c2f5e3771f8f4daf915050262d81b22e211453 100644
--- a/net/dns/dns_transaction.cc
+++ b/net/dns/dns_transaction.cc
@@ -338,7 +338,9 @@ class DnsTCPAttempt : public DnsAttempt {
     STATE_CONNECT_COMPLETE,
     STATE_SEND_LENGTH,
     STATE_SEND_QUERY,
+    STATE_START_READING_LENGTH,
     STATE_READ_LENGTH,
+    STATE_START_READING_RESPONSE,
     STATE_READ_RESPONSE,
     STATE_NONE,
   };
@@ -359,9 +361,15 @@ class DnsTCPAttempt : public DnsAttempt {
         case STATE_SEND_QUERY:
           rv = DoSendQuery(rv);
           break;
+        case STATE_START_READING_LENGTH:
+          rv = DoStartReadingLength(rv);
+          break;
         case STATE_READ_LENGTH:
           rv = DoReadLength(rv);
           break;
+        case STATE_START_READING_RESPONSE:
+          rv = DoStartReadingResponse(rv);
+          break;
         case STATE_READ_RESPONSE:
           rv = DoReadResponse(rv);
           break;
@@ -430,23 +438,32 @@ class DnsTCPAttempt : public DnsAttempt {
     }
     buffer_ =
         new DrainableIOBuffer(length_buffer_.get(), length_buffer_->size());
-    next_state_ = STATE_READ_LENGTH;
+    next_state_ = STATE_START_READING_LENGTH;
     return OK;
   }
 
+  int DoStartReadingLength(int rv) {
+    DCHECK_NE(ERR_IO_PENDING, rv);
+    if (rv < 0)
+      return rv;
+
+    next_state_ = STATE_READ_LENGTH;
+    return ReadIntoBuffer();
+  }
+
   int DoReadLength(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 ReadIntoBuffer();
     }
+
     base::ReadBigEndian<uint16>(length_buffer_->data(), &response_length_);
     // Check if advertised response is too short. (Optimization only.)
     if (response_length_ < query_->io_buffer()->size())
@@ -454,23 +471,32 @@ class DnsTCPAttempt : public DnsAttempt {
     // Allocate more space so that DnsResponse::InitParse sanity check passes.
     response_.reset(new DnsResponse(response_length_ + 1));
     buffer_ = new DrainableIOBuffer(response_->io_buffer(), response_length_);
-    next_state_ = STATE_READ_RESPONSE;
+    next_state_ = STATE_START_READING_RESPONSE;
     return OK;
   }
 
+  int DoStartReadingResponse(int rv) {
+    DCHECK_NE(ERR_IO_PENDING, rv);
+    if (rv < 0)
+      return rv;
+
+    next_state_ = STATE_READ_RESPONSE;
+    return ReadIntoBuffer();
+  }
+
   int DoReadResponse(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 ReadIntoBuffer();

[mmenke, 2014/08/01 15:38:42]

I think this would be much clearer with:

// Note no argument.  Could have a DCHECK_NE(ERR_IO_PENDING, rv);
// before the call to it, if you wanted, though not sure it gets us
// much, given our other DCHECKs about that.
int DoReadResponse() {
  next_state_ = STATE_READ_RESPONSE_COMPLETE;
  return socket_->Read(
      buffer_.get(),
      buffer_->BytesRemaining(),
      base::Bind(&DnsTCPAttempt::OnIOComplete,
                 base::Unretained(this)));
}

int DoReadResponseComplete(int rv) {
  DCHECK_NE(ERR_IO_PENDING, rv);
  if (rv < 0)
    return rv;
  if (rv == 0)
    return ERR_CONNECTION_CLOSED;

  if (buffer_->BytesRemaining() > 0) {
    next_state_ = STATE_READ_RESPONSE;
    return OK;
  }

  // Parse the response here.
}


And do the same when reading the length.  This is how a lot of loops in net/
work, and it makes all reads, including the first one, follow the same basic
path through the code.

[Deprecated (see juliatuttle), 2014/08/01 22:01:10]

Done.

     }
+
     if (!response_->InitParse(buffer_->BytesConsumed(), *query_))
       return ERR_DNS_MALFORMED_RESPONSE;
     if (response_->flags() & dns_protocol::kFlagTC)
@@ -490,6 +516,14 @@ class DnsTCPAttempt : public DnsAttempt {
       callback_.Run(rv);
   }
 
+  int ReadIntoBuffer() {
+    int rv = socket_->Read(
+        buffer_.get(),
+        buffer_->BytesRemaining(),
+        base::Bind(&DnsTCPAttempt::OnIOComplete, base::Unretained(this)));
+    return (rv != 0) ? rv : ERR_CONNECTION_CLOSED;
+  }
+
   State next_state_;
   base::TimeTicks start_time_;