Resolver bug fixes

- Record and return last response error from performDNSQuery, to capture
  more useful failure information.

- Retain previous lastErr in ResolveIP, when the context times out, to capture
  useful failure information. Don't clobber lastErr when launching the next
  round of requests.

- Don't report an RTT if no response is received.

- Add servers to list in setCurrentActiveNetworkAndProperties.

- Fix comment typos.

MobileLibrary/Android/PsiphonTunnel/PsiphonTunnel.java

@@ -1495,6 +1495,7 @@ public class PsiphonTunnel {
                                 if (server.startsWith("/")) {
                                     server = server.substring(1);
                                 }
+                                servers.add(server);
                             }
                         } catch (java.lang.Exception e) {
                         }

MobileLibrary/psi/psi.go

@@ -54,8 +54,8 @@ type PsiphonProvider interface {
 	PsiphonProviderNetwork
 	BindToDevice(fileDescriptor int) (string, error)
 
-	// GetDNSServers must return a comma-delimited list of DNS server
-	// addresses. A single string return value is used gobind does not
+	// GetDNSServersAsString must return a comma-delimited list of DNS server
+	// addresses. A single string return value is used since gobind does not
 	// support string slice types.
 	GetDNSServersAsString() string
 }

psiphon/TCPConn.go

@@ -213,7 +213,7 @@ func tcpDial(ctx context.Context, addr string, config *DialConfig) (net.Conn, er
 		return nil, errors.Trace(err)
 	}
 	if config.ResolveIP == nil {
-		// Fail even if we don't need a resolver fir this dial: this is a code
+		// Fail even if we don't need a resolver for this dial: this is a code
 		// misconfiguration.
 		return nil, errors.TraceNew("missing resolver")
 	}

psiphon/UDPConn.go

@@ -55,7 +55,7 @@ func NewUDPConn(
 	}
 
 	if config.ResolveIP == nil {
-		// Fail even if we don't need a resolver fir this dial: this is a code
+		// Fail even if we don't need a resolver for this dial: this is a code
 		// misconfiguration.
 		return nil, nil, errors.TraceNew("missing resolver")
 	}

psiphon/common/parameters/parameters.go

@@ -218,6 +218,8 @@ const (
 	ReplayTargetUpstreamBytes                        = "ReplayTargetUpstreamBytes"
 	ReplayTargetDownstreamBytes                      = "ReplayTargetDownstreamBytes"
 	ReplayTargetTunnelDuration                       = "ReplayTargetTunnelDuration"
+	ReplayLaterRoundMoveToFrontProbability           = "ReplayLaterRoundMoveToFrontProbability"
+	ReplayRetainFailedProbability                    = "ReplayRetainFailedProbability"
 	ReplayBPF                                        = "ReplayBPF"
 	ReplaySSH                                        = "ReplaySSH"
 	ReplayObfuscatorPadding                          = "ReplayObfuscatorPadding"
@@ -233,8 +235,6 @@ const (
 	ReplayLivenessTest                               = "ReplayLivenessTest"
 	ReplayUserAgent                                  = "ReplayUserAgent"
 	ReplayAPIRequestPadding                          = "ReplayAPIRequestPadding"
-	ReplayLaterRoundMoveToFrontProbability           = "ReplayLaterRoundMoveToFrontProbability"
-	ReplayRetainFailedProbability                    = "ReplayRetainFailedProbability"
 	ReplayHoldOffTunnel                              = "ReplayHoldOffTunnel"
 	ReplayResolveParameters                          = "ReplayResolveParameters"
 	APIRequestUpstreamPaddingMinBytes                = "APIRequestUpstreamPaddingMinBytes"
@@ -548,6 +548,8 @@ var defaultParameters = map[string]struct {
 	ReplayTargetUpstreamBytes:              {value: 0, minimum: 0},
 	ReplayTargetDownstreamBytes:            {value: 0, minimum: 0},
 	ReplayTargetTunnelDuration:             {value: 1 * time.Second, minimum: time.Duration(0)},
+	ReplayLaterRoundMoveToFrontProbability: {value: 0.0, minimum: 0.0},
+	ReplayRetainFailedProbability:          {value: 0.5, minimum: 0.0},
 	ReplayBPF:                              {value: true},
 	ReplaySSH:                              {value: true},
 	ReplayObfuscatorPadding:                {value: true},
@@ -563,8 +565,6 @@ var defaultParameters = map[string]struct {
 	ReplayLivenessTest:                     {value: true},
 	ReplayUserAgent:                        {value: true},
 	ReplayAPIRequestPadding:                {value: true},
-	ReplayLaterRoundMoveToFrontProbability: {value: 0.0, minimum: 0.0},
-	ReplayRetainFailedProbability:          {value: 0.5, minimum: 0.0},
 	ReplayHoldOffTunnel:                    {value: true},
 	ReplayResolveParameters:                {value: true},
 

psiphon/common/resolver/resolver.go

@@ -623,9 +623,7 @@ func (r *Resolver) ResolveIP(
 				// use a distinct socket per request, as common DNS clients do.
 				conn, err := r.newResolverConn(r.networkConfig.logWarning, server)
 				if err != nil {
-					if resolveCtx.Err() == nil {
-						lastErr.Store(errors.Trace(err))
-					}
+					lastErr.Store(errors.Trace(err))
 					return
 				}
 				defer conn.Close()
@@ -676,22 +674,19 @@ func (r *Resolver) ResolveIP(
 					hostname)
 
 				// Update the min/max RTT metric when reported (>=0) even if
-				// the result is an error -- i.e., the even if there was an
-				// invalid response --  unless the resolveCtx is done
-				// (we don't want to consider the RTT in the case of cancellation).
+				// the result is an error; i.e., the even if there was an
+				// invalid response.
 				//
 				// Limitation: since individual requests aren't cancelled
 				// after requestTimeout, RTT metrics won't reflect
 				// no-response cases, although request and response count
 				// disparities will still show up in the metrics.
-				if RTT >= 0 && resolveCtx.Err() == nil {
+				if RTT >= 0 {
 					r.updateMetricRTT(RTT)
 				}
 
 				if err != nil {
-					if resolveCtx.Err() == nil {
-						lastErr.Store(errors.Trace(err))
-					}
+					lastErr.Store(errors.Trace(err))
 					return
 				}
 
@@ -730,16 +725,13 @@ func (r *Resolver) ResolveIP(
 			// When requestTimeout arrives, loop around and launch the next
 			// attempt; leave the existing requests running in case they
 			// eventually respond.
-			lastErr.Store(errors.TraceNew("timeout"))
 			timerDrained = true
 		case <-resolveCtx.Done():
 			// When resolveCtx is done, exit the attempts loop.
 			//
-			// TODO: retain previous lastErr, for failed_tunnel, if it
-			// provides more infomation about DNS interference? As part of
-			// this, have performDNSQuery also return relevent errors instead
-			// of simply calling LogWarning for invalid DNS responses.
-			lastErr.Store(errors.Trace(ctx.Err()))
+			// Append the existing lastErr, which may convey useful
+			// information to be reported in a failed_tunnel error message.
+			lastErr.Store(errors.Tracef("%v (lastErr: %v)", ctx.Err(), lastErr.Load()))
 			stop = true
 		}
 	}
@@ -1255,16 +1247,30 @@ func performDNSQuery(
 	// Read and process the DNS response
 	var IPs []net.IP
 	var TTLs []time.Duration
+	var lastErr error
+	RTT := time.Duration(-1)
 	for {
 
 		// Stop when resolveCtx is done; the caller, ResolveIP, will also
 		// close conn, which will interrupt a blocking dnsConn.ReadMsg.
 		if resolveCtx.Err() != nil {
-			return nil, nil, time.Since(startTime), errors.Trace(resolveCtx.Err())
+
+			// ResolveIP, which calls performDNSQuery, already records the
+			// context error (e.g., context timeout), so instead report
+			// lastErr, when present, as it may contain more useful
+			// information about why a response was rejected.
+			err := lastErr
+			if err == nil {
+				err = errors.Trace(resolveCtx.Err())
+			}
+
+			return nil, nil, RTT, err
 		}
 
-		// Read a response
+		// Read a response. RTT is the elapsed time between sending the
+		// request and reading the last received response.
 		response, err := dnsConn.ReadMsg()
+		RTT = time.Since(startTime)
 		if err == nil && response.MsgHdr.Id != request.MsgHdr.Id {
 			err = dns.ErrId
 		}
@@ -1275,7 +1281,8 @@ func performDNSQuery(
 			if resolveCtx.Err() == nil {
 				// Only log if resolveCtx is not done; otherwise the error could
 				// be due to conn being closed by ResolveIP.
-				logWarning(errors.Tracef("invalid response: %v", err))
+				lastErr = errors.Tracef("invalid response: %v", err)
+				logWarning(lastErr)
 			}
 			continue
 		}
@@ -1305,7 +1312,8 @@ func performDNSQuery(
 			if !ok {
 				errMsg = fmt.Sprintf("Rcode: %d", response.MsgHdr.Rcode)
 			}
-			logWarning(errors.Tracef("unexpected RCode: %v", errMsg))
+			lastErr = errors.Tracef("unexpected RCode: %v", errMsg)
+			logWarning(lastErr)
 			continue
 		}
 
@@ -1339,7 +1347,8 @@ func performDNSQuery(
 			err := checkDNSAnswerIP(IP)
 			if err != nil {
 				checkFailed = true
-				logWarning(errors.Tracef("invalid IP: %v", err))
+				lastErr = errors.Tracef("invalid IP: %v", err)
+				logWarning(lastErr)
 				// Check the next answer
 				continue
 			}
@@ -1355,7 +1364,8 @@ func performDNSQuery(
 		// logic will delay the parent dial in that case.
 		if questionType == resolverQuestionTypeA && len(IPs) == 0 && !checkFailed {
 			checkFailed = true
-			logWarning(errors.TraceNew("unexpected empty A response"))
+			lastErr = errors.TraceNew("unexpected empty A response")
+			logWarning(lastErr)
 		}
 
 		// Retry if there are no valid IPs and any error; if no error, this
@@ -1365,7 +1375,7 @@ func performDNSQuery(
 			continue
 		}
 
-		return IPs, TTLs, time.Since(startTime), nil
+		return IPs, TTLs, RTT, nil
 	}
 }
 

psiphon/common/transforms/transforms.go

@@ -70,9 +70,9 @@ func (specs Specs) Validate() error {
 	return nil
 }
 
-// ScopedSpecNames defines groups a list of Specs, referenced by their Spec
-// name, with the group defined by a scope. The meaning of scope depends on
-// the context in which the transforms are to be used.
+// ScopedSpecNames groups a list of Specs, referenced by their Spec name, with
+// the group defined by a scope. The meaning of scope depends on the context
+// in which the transforms are to be used.
 //
 // For example, in the context of DNS request transforms, the scope is the DNS
 // server for which a specific group of transforms is known to be effective.
@@ -142,14 +142,14 @@ func (specs Specs) Select(scope string, scopedSpecs ScopedSpecNames) (string, Sp
 	return specName, spec
 }
 
-// Apply applies the Spec to the input string, producting the output string.
+// Apply applies the Spec to the input string, producing the output string.
 //
 // The input seed is used for all random generation. The same seed can be
 // supplied to produce the same output, for replay.
 func (spec Spec) Apply(seed *prng.Seed, input string) (string, error) {
 
-	// TODO: complied regexp and regen could be cached, but the seed is an
-	// issue with the regen.
+	// TODO: the compiled regexp and regen could be cached, but the seed is an
+	// issue with caching the regen.
 
 	value := input
 	for _, transform := range spec {

psiphon/controller.go

@@ -151,8 +151,8 @@ func NewController(config *Config) (controller *Controller, err error) {
 		signalRestartEstablishing: make(chan struct{}, 1),
 	}
 
-	// Initialize untunneledDialConfig, used untunneled dials including remote
-	// server list and upgrade downloads.
+	// Initialize untunneledDialConfig, used by untunneled dials including
+	// remote server list and upgrade downloads.
 	controller.untunneledDialConfig = &DialConfig{
 		UpstreamProxyURL: controller.config.UpstreamProxyURL,
 		CustomHeaders:    controller.config.CustomHeaders,