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

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

When an unexpected packet is read from the socket, the current behavior is to give up on this attempt and immediately make another one.

Considerable fraction of observed failures is due to ERR_DNS_MALFORMED_RESPONSE errors which might be false positives. For example, if behind a NAT, we might be receiving packets through stale mappings (addressed to other hosts).

Rather than simply ignoring malformed packets, this CL implements the behavior: "don't give up on this attempt, but immediately make another one."

BUG=107413
TEST=./net_unittests  --gtest_filter=DnsTransactionTest.MalformedResponse*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=154171

[mmenke, 2012-08-28 14:15:54]

http://codereview.chromium.org/10824238/diff/8002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/8002/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:154: return received_malformed_response_ ?
ERR_DNS_MALFORMED_RESPONSE : rv;
Err...How does this work?  It looks like if we ever get a malformed response, we
set |received_malformed_response_| to true, and never switch it to false if we
subsequently get a non-malformed response, we'll still return
ERR_DNS_MALFORMED_RESPONSE here.

[szym, 2012-08-28 14:59:44]

http://codereview.chromium.org/10824238/diff/8002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/8002/net/dns/dns_transaction.cc#...
net/dns/dns_transaction.cc:154: return received_malformed_response_ ?
ERR_DNS_MALFORMED_RESPONSE : rv;
On 2012/08/28 14:15:54, Matt Menke wrote:
> Err...How does this work?  It looks like if we ever get a malformed response,
we
> set |received_malformed_response_| to true, and never switch it to false if we
> subsequently get a non-malformed response, we'll still return
> ERR_DNS_MALFORMED_RESPONSE here.

You're right. This isn't finished yet, and the test I wrote doesn't really
exercise it. I'll upload a fixed patch soon.

[szym, 2012-08-28 22:12:37]

This is now ready for review.

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

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
When does this happen?  Won't we just end up with a ERR_DNS_TIMED_OUT?

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
File net/dns/dns_transaction_unittest.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:51: void AddRcode(int rcode, IoMode mode) {
This and AddResponse should have comments - particularly this one.

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:62: void AddResponse(scoped_ptr<DnsResponse>
response, IoMode mode) {
nit:  Suggest you put this before AddRcode, since that's the same order the
corresponding functions appear in below.

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:87: bool got_written() const {
nit:  Suggest was_written(), or maybe query_was_written().

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:114: private:
While you're here, there should be a blank line above private.

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:214: if (quit_in_callback_)
Please add a comment that this needs to be done before the asserts below.

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:305: void AddResponse(const char*
dotted_name,
While you're here, could you rename this?  The comment is accurate, but the name
is misleading.  AddQueryAndResponse, maybe?

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:345: void AddRcode(const char* dotted_name,
uint16 qtype, int rcode, IoMode mode) {
Same comment as above.  AddQueryAndRcodeResponse, maybe?

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:515: TEST_F(DnsTransactionTest,
MismatchedResponseSync) {
I'd also like a test where we get an ERR_DNS_MALFORMED_RESPONSE

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

Since making the helper method names longer required reformatting, I have
reformatted all the tests.

I added MismatchedResponseFail which verifies that transaction times out if no
matching response ever arrives.

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
On 2012/08/29 16:23:57, Matt Menke wrote:
> When does this happen?  Won't we just end up with a ERR_DNS_TIMED_OUT?

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.

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:158: received_malformed_response_ = false;
On second thought, this is not necessary. At that point the attempt is done.

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
File net/dns/dns_transaction_unittest.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:515: TEST_F(DnsTransactionTest,
MismatchedResponseSync) {
On 2012/08/29 16:23:57, Matt Menke wrote:
> I'd also like a test where we get an ERR_DNS_MALFORMED_RESPONSE

That was not possible in this patch. If the transaction could't make any further
attempts it would wait until the last attempt timed out. See
dns_transaction.cc:520

If we fail early, we will still be vulnerable to unrelated packets, just
slightly less if retries are allowed. If we wait until timeout, we might be
fruitlessly stalling the whole HostResolver::Request. I'd rather err on the
optimistic site for now.

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

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
On 2012/08/29 21:37:26, szym wrote:
> On 2012/08/29 16:23:57, Matt Menke wrote:
> > When does this happen?  Won't we just end up with a ERR_DNS_TIMED_OUT?
> 
> 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.

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]

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
On 2012/08/29 21:43:53, Matt Menke wrote:
> On 2012/08/29 21:37:26, szym wrote:
> > On 2012/08/29 16:23:57, Matt Menke wrote:
> > > When does this happen?  Won't we just end up with a ERR_DNS_TIMED_OUT?
> > 
> > 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.
> 
> 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.

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

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

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
On 2012/08/29 21:43:53, Matt Menke wrote:
> 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.

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]

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
On 2012/08/29 21:55:05, szym wrote:
> On 2012/08/29 21:43:53, Matt Menke wrote:
> > 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.
> 
> 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)."

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:10]

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
On 2012/08/29 22:01:22, Matt Menke wrote:
> 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.

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.

[szym, 2012-08-29 22:19:42]

On 2012/08/29 22:19:10, szym wrote:
> 
> 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.

By "failed" I mean, might have failed.

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

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
On 2012/08/29 22:19:11, szym wrote:
> On 2012/08/29 22:01:22, Matt Menke wrote:
> > 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.
> 
> 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.

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.

[mmenke, 2012-08-29 22:32:04]

On 2012/08/29 22:29:44, Matt Menke wrote:
> http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
> File net/dns/dns_transaction.cc (right):
> 
>
http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
> net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
> On 2012/08/29 22:19:11, szym wrote:
> > On 2012/08/29 22:01:22, Matt Menke wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

Anyways, this CL LGTM, I just think it makes sense for us to keep the old
sockets trying to read data.

[szym, 2012-08-29 22:33:40]

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
On 2012/08/29 22:29:44, Matt Menke wrote:
> On 2012/08/29 22:19:11, szym wrote:
> > On 2012/08/29 22:01:22, Matt Menke wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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."

[mmenke, 2012-08-29 22:39:36]

On 2012/08/29 22:33:40, szym wrote:
> http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc
> File net/dns/dns_transaction.cc (right):
> 
>
http://codereview.chromium.org/10824238/diff/20002/net/dns/dns_transaction.cc...
> net/dns/dns_transaction.cc:156: return ERR_DNS_MALFORMED_RESPONSE;
> On 2012/08/29 22:29:44, Matt Menke wrote:
> > On 2012/08/29 22:19:11, szym wrote:
> > > On 2012/08/29 22:01:22, Matt Menke wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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."

Erm...Right.  Perhaps FinishAttempt should be renamed.  I'd just kinda assumed,
without digging back into it, that it cleaned up the attempt that it
was...well..."finishing", but it's not doing that in a lot of cases.

[szym, 2012-08-29 22:40:21]

On 2012/08/29 22:39:36, Matt Menke wrote:
> Erm...Right.  Perhaps FinishAttempt should be renamed.  I'd just kinda
assumed,
> without digging back into it, that it cleaned up the attempt that it
> was...well..."finishing", but it's not doing that in a lot of cases.

ProcessAttemptResult?

[mmenke, 2012-08-29 22:44:48]

On 2012/08/29 22:40:21, szym wrote:
> On 2012/08/29 22:39:36, Matt Menke wrote:
> > Erm...Right.  Perhaps FinishAttempt should be renamed.  I'd just kinda
> assumed,
> > without digging back into it, that it cleaned up the attempt that it
> > was...well..."finishing", but it's not doing that in a lot of cases.
> 
> ProcessAttemptResult?

Works for me.  Thanks for bearing with me, sorry for my confusion.  I'm kinda
paged this stuff out of my cache.

[mmenke, 2012-08-29 23:00:59]

On 2012/08/29 22:44:48, Matt Menke wrote:
> On 2012/08/29 22:40:21, szym wrote:
> > On 2012/08/29 22:39:36, Matt Menke wrote:
> > > Erm...Right.  Perhaps FinishAttempt should be renamed.  I'd just kinda
> > assumed,
> > > without digging back into it, that it cleaned up the attempt that it
> > > was...well..."finishing", but it's not doing that in a lot of cases.
> > 
> > ProcessAttemptResult?
> 
> Works for me.  Thanks for bearing with me, sorry for my confusion.  I'm kinda
> paged this stuff out of my cache.

And the CL LGTM still, now that I actually have had my two misconceptions about
how the code works cleared up.

Just one of those days, I guess.

[commit-bot: I haz the power, 2012-08-30 03:50:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/10824238/28003

[szym, 2012-08-30 03:52:25]

Thanks for the thorough review. Adding another test allowed me to catch an
infinite loop.

[commit-bot: I haz the power, 2012-08-30 05:21:04]

Try job failure for 10824238-28003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...