Contain panics and report read failures in the Windows DNS collection

Claude-Session: https://claude.ai/code/session_01Y4KM6AL3cDzUsx3HEHCoxs

client/internal/debug/debug_windows.go

@@ -6,6 +6,7 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
+	"net/netip"
 	"strings"
 	"unsafe"
 
@@ -41,8 +42,29 @@ var interfaceDNSValues = []string{
 
 // addDNSInfo collects and adds DNS configuration information to the archive
 func (g *BundleGenerator) addDNSInfo() error {
+	if err := g.addFileToZip(strings.NewReader(g.collectDNSInfo()), dnsInfoFileName); err != nil {
+		return fmt.Errorf("add DNS info to zip: %w", err)
+	}
+
+	return nil
+}
+
+// collectDNSInfo renders the report. Everything below it reaches the platform
+// through COM and through lazily resolved procedures, which panic when a
+// procedure is missing rather than returning an error, and a debug bundle is not
+// allowed to take the daemon down. The panic is contained here, and whatever was
+// collected before it is kept and reported with it.
+func (g *BundleGenerator) collectDNSInfo() (content string) {
 	var sb strings.Builder
 
+	defer func() {
+		if r := recover(); r != nil {
+			log.Errorf("collecting Windows DNS configuration panicked: %v", r)
+			fmt.Fprintf(&sb, "\nerror: collection stopped: %v\n", r)
+		}
+		content = sb.String()
+	}()
+
 	sb.WriteString("Windows DNS configuration\n")
 	sb.WriteString("=========================\n")
 
@@ -58,11 +80,7 @@ func (g *BundleGenerator) addDNSInfo() error {
 	g.writeInterfaceDNS(&sb, "Per-interface DNS, IPv6", nbdns.InterfaceConfigPathV6, adapterNames(adapters))
 	g.writeAdapterDNS(&sb, adapters, adaptersErr)
 
-	if err := g.addFileToZip(strings.NewReader(sb.String()), dnsInfoFileName); err != nil {
-		return fmt.Errorf("add DNS info to zip: %w", err)
-	}
-
-	return nil
+	return sb.String()
 }
 
 // writeNRPTRules lists every rule in a policy store, ours and any other
@@ -170,10 +188,14 @@ func (g *BundleGenerator) writeRegistryKey(sb *strings.Builder, title, path stri
 // value, otherwise only those named and present.
 func (g *BundleGenerator) writeValues(sb *strings.Builder, path string, names []string, indent string) {
 	k, err := registry.OpenKey(registry.LOCAL_MACHINE, path, registry.QUERY_VALUE)
-	if err != nil {
+	switch {
+	case errors.Is(err, registry.ErrNotExist), errors.Is(err, windows.ERROR_PATH_NOT_FOUND):
 		// an absent key is the normal state for the GPO store and for
 		// interfaces without DNS settings
-		log.Debugf("open HKEY_LOCAL_MACHINE\\%s: %v", path, err)
+		log.Debugf("HKEY_LOCAL_MACHINE\\%s does not exist", path)
+		return
+	case err != nil:
+		fmt.Fprintf(sb, "%serror: open HKEY_LOCAL_MACHINE\\%s: %v\n", indent, path, err)
 		return
 	}
 	defer closeKey(k)
@@ -188,7 +210,15 @@ func (g *BundleGenerator) writeValues(sb *strings.Builder, path string, names []
 
 	for _, name := range names {
 		value, err := readRegistryValue(k, name)
-		if err != nil {
+		switch {
+		case errors.Is(err, registry.ErrNotExist):
+			// the caller asks for a fixed set of values, most of which a
+			// given interface does not carry
+			continue
+		case err != nil:
+			// report rather than omit: a value that is there but cannot be
+			// read reads as unset otherwise
+			fmt.Fprintf(sb, "%s%s: error: %v\n", indent, name, err)
 			continue
 		}
 
@@ -262,16 +292,16 @@ func (g *BundleGenerator) writeAdapterDNS(sb *strings.Builder, adapters []*windo
 
 		var servers []string
 		for server := adapter.FirstDnsServerAddress; server != nil; server = server.Next {
-			ip := server.Address.IP()
-			if ip == nil {
+			addr, ok := netip.AddrFromSlice(server.Address.IP())
+			if !ok {
 				continue
 			}
 
-			address := ip.String()
+			addr = addr.Unmap()
 			if g.anonymize {
-				address = g.anonymizer.AnonymizeIPString(address)
+				addr = g.anonymizer.AnonymizeIP(addr)
 			}
-			servers = append(servers, address)
+			servers = append(servers, addr.String())
 		}
 
 		fmt.Fprintf(sb, "  DNS servers: %s\n", strings.Join(servers, ", "))
@@ -372,7 +402,16 @@ func readRegistryValue(k registry.Key, name string) (string, error) {
 
 // adapterAddresses returns the adapter list including DNS servers. The call
 // reports the size it needs, so grow the buffer and retry until it fits.
-func adapterAddresses() ([]*windows.IpAdapterAddresses, error) {
+func adapterAddresses() (adapters []*windows.IpAdapterAddresses, err error) {
+	// GetAdaptersAddresses is resolved on first use and panics when it is
+	// missing, so this reports it as an error and leaves the rest of the
+	// report intact.
+	defer func() {
+		if r := recover(); r != nil {
+			adapters, err = nil, fmt.Errorf("GetAdaptersAddresses: %v", r)
+		}
+	}()
+
 	const flags = windows.GAA_FLAG_SKIP_ANYCAST | windows.GAA_FLAG_SKIP_MULTICAST
 
 	size := uint32(15000)
@@ -388,7 +427,6 @@ func adapterAddresses() ([]*windows.IpAdapterAddresses, error) {
 			return nil, fmt.Errorf("GetAdaptersAddresses: %w", err)
 		}
 
-		var adapters []*windows.IpAdapterAddresses
 		for adapter := first; adapter != nil; adapter = adapter.Next {
 			adapters = append(adapters, adapter)
 		}

client/internal/debug/nrpt_windows.go

@@ -32,13 +32,19 @@ const (
 	nrptPolicyTimeout = 15 * time.Second
 )
 
-// COM initialization results that mean the calling thread is already usable.
+// COM initialization results that leave the calling thread usable: S_FALSE for
+// a thread this process already initialized, RPC_E_CHANGED_MODE for one that
+// belongs to another apartment.
 const (
-	sFalse           = 0x00000001
-	rpcEChangedMode  = 0x80010106
-	nrptQueryTimeout = "timed out"
+	sFalse          = 0x00000001
+	rpcEChangedMode = 0x80010106
 )
 
+// nrptQueryInFlight admits one read of the policy table at a time. A provider
+// that stops answering keeps its goroutine and the OS thread that goroutine
+// pinned, so a later bundle reports that instead of pinning another one.
+var nrptQueryInFlight = make(chan struct{}, 1)
+
 // nrptPolicyEntry is one namespace of the effective policy table, holding the
 // values of an embedded DnsClientPolicyConfiguration instance in the order the
 // provider reported them.
@@ -62,8 +68,18 @@ func effectiveNRPTPolicies() ([]nrptPolicyEntry, error) {
 		err  error
 	}
 
+	select {
+	case nrptQueryInFlight <- struct{}{}:
+	default:
+		return nil, errors.New("an earlier read of the policy table has not returned")
+	}
+
 	done := make(chan result, 1)
 	go func() {
+		// the slot is released here rather than by the caller, so a read that
+		// outlives the timeout holds it until the provider answers
+		defer func() { <-nrptQueryInFlight }()
+
 		text, err := nrptPolicyTableText()
 		done <- result{text: text, err: err}
 	}()
@@ -75,7 +91,7 @@ func effectiveNRPTPolicies() ([]nrptPolicyEntry, error) {
 		}
 		return parseNRPTPolicyTable(res.text), nil
 	case <-time.After(nrptPolicyTimeout):
-		return nil, errors.New(nrptQueryTimeout)
+		return nil, errors.New("read of the policy table timed out")
 	}
 }
 
@@ -95,10 +111,13 @@ func nrptPolicyTableText() (text string, err error) {
 		}
 	}()
 
-	if err := coInitialize(); err != nil {
+	owns, err := coInitialize()
+	if err != nil {
 		return "", err
 	}
-	defer ole.CoUninitialize()
+	if owns {
+		defer ole.CoUninitialize()
+	}
 
 	locator, err := oleutil.CreateObject("WbemScripting.SWbemLocator")
 	if err != nil {
@@ -242,25 +261,29 @@ func unquoteMOFValue(value string) string {
 	return strings.Trim(value, `"`)
 }
 
-func coInitialize() error {
+// coInitialize prepares the calling thread for COM and reports whether this
+// call owns the initialization, which decides whether it may be balanced with
+// CoUninitialize. S_FALSE took a reference on a thread this process had already
+// initialized and so has to be released, while RPC_E_CHANGED_MODE took none:
+// the thread belongs to another apartment, which is usable but is not ours to
+// uninitialize.
+func coInitialize() (bool, error) {
 	err := ole.CoInitializeEx(0, ole.COINIT_MULTITHREADED)
 	if err == nil {
-		return nil
+		return true, nil
 	}
 
-	// An already initialized thread reports S_FALSE, and one initialized in
-	// another apartment reports RPC_E_CHANGED_MODE. Both are usable, and
-	// neither owns the uninitialize call, which is balanced per successful
-	// initialization.
 	var oleErr *ole.OleError
 	if errors.As(err, &oleErr) {
 		switch oleErr.Code() {
-		case sFalse, rpcEChangedMode:
-			return nil
+		case sFalse:
+			return true, nil
+		case rpcEChangedMode:
+			return false, nil
 		}
 	}
 
-	return fmt.Errorf("initialize COM: %w", err)
+	return false, fmt.Errorf("initialize COM: %w", err)
 }
 
 // dispatchCall calls a COM method that returns an object.