Repository navigation
outbound: a disarmed DNS door writes no reply produced after the disarm - #556
Merged
Merged
Conversation
Disarm cancels the resolver, and context.AfterFunc closes each open stream connection from another goroutine. A query read before that close was still answered: a cancel before its exchange wrote a 29-byte SERVFAIL and recorded a DNSFailed event, and a cancel after the upstream answered wrote the real reply. serveConn now drops a query read after the cancel and drops any reply once the resolver is canceled before the write, and answer records no failure for an exchange the cancel ended.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root-causes the third flake on the v0.1.31 round's open list,
TestDNSDoorDisarmEndsAnOpenConnection, which failed with "a disarmed door answered 29 bytes on a connection opened while armed". It was a real, benign race.Mechanism.
Disarmcancels the resolver, andcontext.AfterFunc(r.ctx, conn.Close)closes each open stream connection from another goroutine. A query read before that close is still served:DNSFailedevent, and sends no upstream query. That SERVFAIL is 12 + 13 + 4 = 29 bytes, the flake's exact size.Fix (
outbound/dns.go):serveConnreturns without answering a query it reads once the resolver is canceled.selecttook.answerrecords noDNSFailedfor an exchange the cancel ended.Tests:
TestADisarmedResolverDropsAQueryItReadsAfterTheDisarm: the resolver is canceled before the read, over an unclosable pipe soAfterFunccannot win, and repeated 20 times against the select's randomness.TestADisarmLandingWhileAQueryIsAnsweredWritesNoReplyAndRecordsNoFailure:Nameservers;DNSFailed.DNSFailedrecorded for a dropped query.Cost. One
ctx.Err()check per DNS query on the door, and none on the claim path./simplify: a 3-line production diff with no helper to reuse.
Gates (fail-fast ship script):
0 issues.on darwin and linux; fmt clean; asl clean;utilstest;