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

net/dns/dns_transaction.cc

 // Copyright (c) 2012 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/dns/dns_transaction.h"
 
 #include <deque>
 #include <string>
 #include <vector>
 
@@ skipping 320 matching lines @@
 
   virtual const BoundNetLog& GetSocketNetLog() const OVERRIDE {
     return socket_->NetLog();
   }
 
  private:
   enum State {
     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,
   };
 
   int DoLoop(int result) {
     CHECK_NE(STATE_NONE, next_state_);
     int rv = result;
     do {
       State state = next_state_;
       next_state_ = STATE_NONE;
       switch (state) {
         case STATE_CONNECT_COMPLETE:
           rv = DoConnectComplete(rv);
           break;
         case STATE_SEND_LENGTH:
           rv = DoSendLength(rv);
           break;
         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;
         default:
           NOTREACHED();
           break;
       }
     } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
     set_result(rv);
@@ skipping 48 matching lines @@
     buffer_->DidConsume(rv);
     if (buffer_->BytesRemaining() > 0) {
       next_state_ = STATE_SEND_QUERY;
       return socket_->Write(
           buffer_.get(),
           buffer_->BytesRemaining(),
           base::Bind(&DnsTCPAttempt::OnIOComplete, base::Unretained(this)));
     }
     buffer_ =
         new DrainableIOBuffer(length_buffer_.get(), length_buffer_->size());
+    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 OK;
+    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())
       return ERR_DNS_MALFORMED_RESPONSE;
     // 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_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 OK;
+    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)
       return ERR_UNEXPECTED;
     // TODO(szym): Frankly, none of these are expected.
     if (response_->rcode() == dns_protocol::kRcodeNXDOMAIN)
       return ERR_NAME_NOT_RESOLVED;
     if (response_->rcode() != dns_protocol::kRcodeNOERROR)
       return ERR_DNS_SERVER_FAILED;
 
     return OK;
   }
 
   void OnIOComplete(int rv) {
     rv = DoLoop(rv);
     if (rv != ERR_IO_PENDING)
       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_;
 
   scoped_ptr<StreamSocket> socket_;
   scoped_ptr<DnsQuery> query_;
   scoped_refptr<IOBufferWithSize> length_buffer_;
   scoped_refptr<DrainableIOBuffer> buffer_;
 
   uint16 response_length_;
   scoped_ptr<DnsResponse> response_;
@@ skipping 452 matching lines @@
 }  // namespace
 
 // static
 scoped_ptr<DnsTransactionFactory> DnsTransactionFactory::CreateFactory(
     DnsSession* session) {
   return scoped_ptr<DnsTransactionFactory>(
       new DnsTransactionFactoryImpl(session));
 }
 
 }  // namespace net