tcp,udp: refactor dns override logic

avoid subtle bugs sending repeat summaries on errs for the same netstack conn

Author: ignoramous

intra/common.go

@@ -407,17 +407,18 @@ func (h *baseHandler) stall(flowid string) (secs uint32) {
 	return
 }
 
-func (h *baseHandler) dnsOverride(conn net.Conn, addr netip.AddrPort, uid string) bool {
+func (h *baseHandler) isDNS(addr netip.AddrPort) bool {
+	return addr.IsValid() && h.resolver.IsDnsAddr(addr)
+}
+
+func (h *baseHandler) dnsOverride(conn net.Conn, uid string) bool {
 	// addr with zone information removed; see: netip.ParseAddrPort which h.resolver relies on
 	// addr2 := &net.TCPAddr{IP: addr.IP, Port: addr.Port}
-	if addr.IsValid() && h.resolver.IsDnsAddr(addr) {
-		// conn closed by the resolver
-		core.Gx(h.proto+".dns", func() {
-			h.resolver.Serve(h.proto, conn, uid)
-		})
-		return true
-	}
-	return false
+	// conn closed by the resolver
+	core.Gx(h.proto+".dns", func() {
+		h.resolver.Serve(h.proto, conn, uid)
+	})
+	return true
 }
 
 // End implements netstack.GBaseConnHandler

intra/tcp.go

@@ -157,8 +157,8 @@ func (h *tcpHandler) handshakeIfNeededOrClose(gconn *netstack.GTCPConn, smm *Soc
 	if _, err := gconn.Establish(); err != nil {
 		err = log.EE("tcp: %s handshake err %v; %s => %s for %s",
 			smm.ID, err, smm.Source, smm.Target, smm.UID)
-		clos(gconn)
-		h.queueSummary(smm.done(err))
+		// clos(gconn)
+		// h.queueSummary(smm.done(err))
 		return deny, err // == !open
 	}
 	return allow, nil
@@ -221,8 +221,22 @@ func (h *tcpHandler) Proxy(gconn *netstack.GTCPConn, src, target netip.AddrPort)
 		return deny
 	}
 
-	if isAnyBasePid(pids) { // see udp.go:Connect
-		if synack, _ := h.handshakeIfNeededOrClose(gconn, smm); synack && h.dnsOverride(gconn, target, uid) {
+	isTarget6 := target.Addr().Is6()
+	happyeyeballs := settings.HappyEyeballs.Load()
+	delayForHappyEyeballs := happyeyeballs && isTarget6
+
+	if isAnyBasePid(pids) && h.isDNS(target) { // see udp.go:Connect
+		synack, synackerr := h.handshakeIfNeededOrClose(gconn, smm)
+		if !synack {
+			// if IPv6, stall a bit more so apps doing HappyEyeballs will try IPv4
+			if delayForHappyEyeballs {
+				time.Sleep(400 * time.Millisecond)
+			}
+			clos(gconn)
+			h.queueSummary(smm.done(synackerr))
+			return deny
+		}
+		if h.dnsOverride(gconn, uid) {
 			// SocketSummary not sent; x.DNSSummary supercedes it
 			// conn closed by resolver
 			return allow
@@ -234,9 +248,6 @@ func (h *tcpHandler) Proxy(gconn *netstack.GTCPConn, src, target netip.AddrPort)
 			cid, src, target, actualTargets, excluded, uid, pids)
 	}
 
-	isTarget6 := target.Addr().Is6()
-	happyeyeballs := settings.HappyEyeballs.Load()
-	delayForHappyEyeballs := happyeyeballs && isTarget6
 	cont := true
 	boundSrc := makeAnyAddrPort(src)
 	// pick all realips to connect to
@@ -270,11 +281,11 @@ func (h *tcpHandler) Proxy(gconn *netstack.GTCPConn, src, target netip.AddrPort)
 		}
 	}
 
-	h.queueSummary(smm.done(err))
 	// if IPv6, stall a bit more so apps doing HappyEyeballs will try IPv4
 	if delayForHappyEyeballs {
 		time.Sleep(400 * time.Millisecond)
 	}
+	h.queueSummary(smm.done(err))
 	clos(gconn) // denied
 	return deny
 }

intra/udp.go

@@ -172,8 +172,8 @@ func (h *udpHandler) proxy(gconn *netstack.GUDPConn, src, dst netip.AddrPort, dm
 	if err != nil {
 		clos(gconn, remote) // teardown
 		// smm may be nil; in which case this is a no-op
-		h.queueSummary(smm.done(err))
-		return false // not ok
+		h.queueSummary(smm.done(err)) // no-op if smm is nil
+		return false                  // not ok
 	} else if remote == nil || smm == nil { // dnsOverride or ipn.Block
 		// do not close gconn here; it is either
 		// connected (overridden) or disconnected (blocked) already
@@ -260,8 +260,8 @@ func (h *udpHandler) Connect(gconn *netstack.GUDPConn, src, target netip.AddrPor
 	// as seen (with Flow) is owned by Rethink, then expect the conn
 	// to be marked ipn.Base for queries sent to tunnel's fake DNS addr
 	// and ipn.Exit for anywhere else.
-	if isAnyBasePid(pids) {
-		if h.dnsOverride(gconn, target, uid) {
+	if isAnyBasePid(pids) && h.isDNS(target) {
+		if h.dnsOverride(gconn, uid) {
 			// SocketSummary is not sent to listener; x.DNSSummary is
 			return nil, nil, nil // connect override, no dst
 		} // else: not a dns query or target is not a dns addr