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

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 00d8064f6e8d6f263423659519e9691a99e406aa..98fc9742da419d11a869bc289e03d9c4a1f9ba14 100644
--- a/net/dns/dns_transaction.cc
+++ b/net/dns/dns_transaction.cc
@@ -71,6 +71,7 @@ class DnsUDPAttempt {
                 scoped_ptr<DnsQuery> query,
                 const CompletionCallback& callback)
       : next_state_(STATE_NONE),
+        received_malformed_response_(false),
         socket_(socket.Pass()),
         server_(server),
         query_(query.Pass()),
@@ -150,7 +151,11 @@ class DnsUDPAttempt {
           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;
   }
 
@@ -196,12 +201,14 @@ class DnsUDPAttempt {
 
     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;
@@ -222,6 +229,7 @@ class DnsUDPAttempt {
   }
 
   State next_state_;
+  bool received_malformed_response_;
 
   scoped_ptr<DatagramClientSocket> socket_;
   IPEndPoint server_;
@@ -510,9 +518,9 @@ class DnsTransactionImpl : public DnsTransaction,
           }
           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;
       }
     }