[net/dns] Don't abandon a DnsUDPAttempt when the response does not match the query.

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 53 matching lines @@
 // A single asynchronous DNS exchange over UDP, which consists of sending out a
 // DNS query, waiting for a response, and returning the response that it
 // matches. Logging is done in the socket and in the outer DnsTransaction.
 class DnsUDPAttempt {
  public:
   DnsUDPAttempt(scoped_ptr<DatagramClientSocket> socket,
                 const IPEndPoint& server,
                 scoped_ptr<DnsQuery> query,
                 const CompletionCallback& callback)
       : next_state_(STATE_NONE),
+        received_malformed_response_(false),
         socket_(socket.Pass()),
         server_(server),
         query_(query.Pass()),
         callback_(callback) {
   }
 
   // Starts the attempt. Returns ERR_IO_PENDING if cannot complete synchronously
   // and calls |callback| upon completion.
   int Start() {
     DCHECK_EQ(STATE_NONE, next_state_);
@@ skipping 59 matching lines @@
           rv = DoReadResponse();
           break;
         case STATE_READ_RESPONSE_COMPLETE:
           rv = DoReadResponseComplete(rv);
           break;
         default:
           NOTREACHED();
           break;
       }
     } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
-
+    // Indicate to the transaction that the server might be misbehaving.
+    if (rv == ERR_IO_PENDING && received_malformed_response_)
+      return ERR_DNS_MALFORMED_RESPONSE;

[mmenke, 2012/08/29 16:23:57]

When does this happen?  Won't we just end up with a ERR_DNS_TIMED_OUT?

[szym, 2012/08/29 21:37:26]

As long as we have packets on the socket, we will be in DoLoop. If we get a
non-ERR_IO_PENDING result, then we should just return that result (this attempt
is done). If we get an ERR_IO_PENDING result, we are waiting for the socket. If
we received a malformed response, we want to signal the parent transaction that
this is the case, so that it immediately makes another attempt. This is no
different than signalling early timeout.

Note that if the transaction cannot make another attempt (retry limit hit), the
transaction will fail.

[mmenke, 2012/08/29 21:43:53]

I'm not following.  It seems to me like this if statement will never fail.  If
we get ERR_IO_PENDING, next_state_ will never be STATE_NONE.  Since this code
can thus never be hit, we should remove it all together, and the
received_malformed_response_ boolean as well.

[mmenke, 2012/08/29 21:46:42]

Err...That should be "that if statement will never be true".

[szym, 2012/08/29 21:55:05]

The goal is not to just ignore malformed packets but to immediately make another
attempt if that's allowed. If I remove this, MismatchedResponseAsync will fail,
because the transaction won't make the second attempt.

The purpose of |received_malformed_response_| is to allow us to enter
DoReadResponse but return ERR_DNS_MALFORMED_RESPONSE to the transaction if
DoReadResponse returned ERR_IO_PENDING. It basically means "If at any point you
need to wait for the socket and have previously received a malformed response,
tell the transaction to make another attempt (if that's allowed)."

[mmenke, 2012/08/29 22:01:22]

But DoReadResponse sets next_state_ to STATE_READ_RESPONSE_COMPLETE, so even if
it returns ERR_IO_PENDING, this if statement will be false.  So if you removed
received_malformed_response_ from this CL, it's behavior wouldn't change at all.

[szym, 2012/08/29 22:19:11]

If DoReadResponse returns ERR_IO_PENDING, then we exit the loop and hit this
condition (regardless of next_state_). If (and only if)
received_malformed_response_ is also true, then this statement is hit and the
attempt returns ERR_DNS_MALFORMED_RESPONSE to the transaction.

So, if I don't set received_malformed_response_ to true when InitParse fails,
then the transaction won't know this attempt failed and simply wait for timeout
without making another attempt.

[mmenke, 2012/08/29 22:29:44]

Gah, for some reason, I was thinking the next_state_ != STATE_NONE was also in
the if statement, which is what was confusing me.

The actual behavior, now that that's cleared up, still strikes me as odd.  How
often is there another datagram already waiting to be read synchronously on the
same socket?  I had thought the plan was to just keep on reading off the socket
until we time out.

[szym, 2012/08/29 22:33:41]

Note that the attempt will be kept alive and that the whole transaction will not
fail until the last attempt times out. The purpose of this logic is essentially
to fake a short timeout so that the next attempt is made earlier.

The old behavior was: "give up on this attempt and immediately make another
one."

The simplistic behavior would be: "just ignore malformed responses; wait until
this attempt times out before making another one."

This behavior is: "don't give up on this attempt, but immediately make another
one."

+    // If we received any other result, it takes precedence.
+    received_malformed_response_ = false;

[szym, 2012/08/29 21:37:26]

On second thought, this is not necessary. At that point the attempt is done.

     return rv;
   }
 
   int DoConnect() {
     next_state_ = STATE_SEND_QUERY;
     return socket_->Connect(server_);
   }
 
   int DoSendQuery() {
     next_state_ = STATE_SEND_QUERY_COMPLETE;
@@ skipping 25 matching lines @@
                                     base::Unretained(this)));
   }
 
   int DoReadResponseComplete(int rv) {
     DCHECK_NE(ERR_IO_PENDING, rv);
     if (rv < 0)
       return rv;
 
     DCHECK(rv);
     if (!response_->InitParse(rv, *query_)) {
-      // TODO(szym): Consider making this reaction less aggressive.
       // Other implementations simply ignore mismatched responses. Since each
       // DnsUDPAttempt binds to a different port, we might find that responses
       // to previously timed out queries lead to failures in the future.
-      // http://crbug.com/107413
-      return ERR_DNS_MALFORMED_RESPONSE;
+      // Our solution is to make another attempt, in case the query truly
+      // failed, but keep this attempt alive, in case it was a false alarm.
+      received_malformed_response_ = true;
+      next_state_ = STATE_READ_RESPONSE;
+      return OK;
     }
     if (response_->flags() & dns_protocol::kFlagTC)
       return ERR_DNS_SERVER_REQUIRES_TCP;
     // TODO(szym): Extract TTL for NXDOMAIN results. http://crbug.com/115051
     if (response_->rcode() == dns_protocol::kRcodeNXDOMAIN)
       return ERR_NAME_NOT_RESOLVED;
     if (response_->rcode() != dns_protocol::kRcodeNOERROR)
       return ERR_DNS_SERVER_FAILED;
 
     CHECK(response());
     return OK;
   }
 
   void OnIOComplete(int rv) {
     rv = DoLoop(rv);
     if (rv != ERR_IO_PENDING)
       callback_.Run(rv);
   }
 
   State next_state_;
+  bool received_malformed_response_;
 
   scoped_ptr<DatagramClientSocket> socket_;
   IPEndPoint server_;
   scoped_ptr<DnsQuery> query_;
 
   scoped_ptr<DnsResponse> response_;
 
   CompletionCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(DnsUDPAttempt);
@@ skipping 268 matching lines @@
           break;
         default:
           // Server failure.
           DCHECK(result.attempt);
           if (result.attempt != attempts_.back()) {
             // This attempt already timed out. Ignore it.
             return AttemptResult(ERR_IO_PENDING, NULL);
           }
           if (MoreAttemptsAllowed()) {
             result = MakeAttempt();
-          } else {
+          } else if (result.rv != ERR_DNS_MALFORMED_RESPONSE) {
             return AttemptResult(result.rv, NULL);
-          }
+          }  // else wait until attempt times out.
           break;
       }
     }
     return result;
   }
 
   void OnTimeout() {
     if (callback_.is_null())
       return;
     AttemptResult result = FinishAttempt(
@@ skipping 53 matching lines @@
 }  // namespace
 
 // static
 scoped_ptr<DnsTransactionFactory> DnsTransactionFactory::CreateFactory(
     DnsSession* session) {
   return scoped_ptr<DnsTransactionFactory>(
       new DnsTransactionFactoryImpl(session));
 }
 
 }  // namespace net