Bug fixes

Flagged by golangci-lint/ineffassign.

- Bug: client broker packet relay only relayed one packet

- Bug: packetman randomized TCP flag transform not properly applied

- Bug: ParametersAccessor.Close had no effect

- Avoid allocation in udpgwPortForward.relayDownstream buffer pool

- Add/fix error return checks

- Remove some unused code

psiphon/common/authPackage.go

@@ -408,7 +408,11 @@ func (streamer *limitedJSONStreamer) Stream() error {
 
 			case stateJSONSeekingStringValueStart:
 				if b == '"' {
-					state = stateJSONSeekingStringValueEnd
+
+					// Note: this assignment is flagged by github.com/gordonklaus/ineffassign,
+					// but is technically the correct state.
+					//
+					//state = stateJSONSeekingStringValueEnd
 
 					key := keyBuffer.String()
 

psiphon/common/inproxy/client.go

@@ -44,7 +44,6 @@ const (
 // initial dial address.
 type ClientConn struct {
 	config       *ClientConfig
-	brokerClient *BrokerClient
 	webRTCConn   *webRTCConn
 	connectionID ID
 	remoteAddr   net.Addr

psiphon/common/inproxy/session_test.go

@@ -347,7 +347,7 @@ func runTestSessions() error {
 
 	request = roundTripper.MakeRequest()
 
-	response, err = unknownInitiatorSessions.RoundTrip(
+	_, err = unknownInitiatorSessions.RoundTrip(
 		ctx,
 		roundTripper,
 		responderPublicKey,
@@ -674,7 +674,6 @@ func runTestNoise() error {
 		return errors.Trace(err)
 	}
 
-	receivedPayload = nil
 	receivedPayload, _, _, err = initiatorHandshake.ReadMessage(nil, responderMsg)
 	if err != nil {
 		return errors.Trace(err)

psiphon/common/packetman/packetman.go

@@ -249,12 +249,15 @@ func (spec *compiledSpec) apply(interceptedPacket gopacket.Packet) ([][]byte, er
 		tmpDataOffset := transformedTCP.DataOffset
 		tmpChecksum := transformedTCP.Checksum
 
-		gopacket.SerializeLayers(
+		err = gopacket.SerializeLayers(
 			buffer,
 			options,
 			serializableNetworkLayer,
 			&transformedTCP,
 			payload)
+		if err != nil {
+			return nil, errors.Trace(err)
+		}
 
 		// In the first SerializeLayers call, all IP and TCP length and checksums
 		// are recalculated and set to the correct values with transformations
@@ -268,13 +271,19 @@ func (spec *compiledSpec) apply(interceptedPacket gopacket.Packet) ([][]byte, er
 		if setCalculatedField {
 			transformedTCP.DataOffset = tmpDataOffset
 			transformedTCP.Checksum = tmpChecksum
-			buffer.Clear()
-			gopacket.SerializeLayers(
+			err = buffer.Clear()
+			if err != nil {
+				return nil, errors.Trace(err)
+			}
+			err = gopacket.SerializeLayers(
 				buffer,
 				gopacket.SerializeOptions{FixLengths: fixLengths, ComputeChecksums: computeChecksums},
 				serializableNetworkLayer,
 				&transformedTCP,
 				payload)
+			if err != nil {
+				return nil, errors.Trace(err)
+			}
 		}
 
 		packets[i] = buffer.Bytes()
@@ -388,15 +397,15 @@ func (t *transformationTCPFlags) apply(tcp *layers.TCP, _ *gopacket.Payload) {
 		flags = t.flags
 	}
 
-	tcp.FIN = strings.Index(t.flags, "F") != -1
-	tcp.SYN = strings.Index(t.flags, "S") != -1
-	tcp.RST = strings.Index(t.flags, "R") != -1
-	tcp.PSH = strings.Index(t.flags, "P") != -1
-	tcp.ACK = strings.Index(t.flags, "A") != -1
-	tcp.URG = strings.Index(t.flags, "U") != -1
-	tcp.ECE = strings.Index(t.flags, "E") != -1
-	tcp.CWR = strings.Index(t.flags, "C") != -1
-	tcp.NS = strings.Index(t.flags, "N") != -1
+	tcp.FIN = strings.Contains(flags, "F")
+	tcp.SYN = strings.Contains(flags, "S")
+	tcp.RST = strings.Contains(flags, "R")
+	tcp.PSH = strings.Contains(flags, "P")
+	tcp.ACK = strings.Contains(flags, "A")
+	tcp.URG = strings.Contains(flags, "U")
+	tcp.ECE = strings.Contains(flags, "E")
+	tcp.CWR = strings.Contains(flags, "C")
+	tcp.NS = strings.Contains(flags, "N")
 }
 
 type transformationTCPField struct {

psiphon/common/parameters/parameters.go

@@ -1687,7 +1687,10 @@ func (p ParametersAccessor) IsNil() bool {
 // where memory footprint is a concern, and where the ParametersAccessor is
 // not immediately going out of scope. After Close is called, all other
 // ParametersAccessor functions will panic if called.
-func (p ParametersAccessor) Close() {
+//
+// Limitation: since ParametersAccessor is typically passed by value, this
+// Close call only impacts the immediate copy.
+func (p *ParametersAccessor) Close() {
 	p.snapshot = nil
 }
 

psiphon/common/prng/prng.go

@@ -198,7 +198,7 @@ func (p *PRNG) Int63() int64 {
 // Uint64 is equivilent to math/rand.Uint64.
 func (p *PRNG) Uint64() uint64 {
 	var b [8]byte
-	p.Read(b[:])
+	_, _ = p.Read(b[:])
 	return binary.BigEndian.Uint64(b[:])
 }
 
@@ -300,7 +300,7 @@ func (p *PRNG) RangeUint32(min, max uint32) uint32 {
 // Bytes returns a new slice containing length random bytes.
 func (p *PRNG) Bytes(length int) []byte {
 	b := make([]byte, length)
-	p.Read(b)
+	_, _ = p.Read(b)
 	return b
 }
 

psiphon/common/quic/obfuscator.go

@@ -270,7 +270,7 @@ func (conn *ObfuscatedPacketConn) Close() error {
 	if conn.isServer {
 
 		// Interrupt any blocked writes.
-		conn.PacketConn.SetWriteDeadline(time.Now())
+		_ = conn.PacketConn.SetWriteDeadline(time.Now())
 
 		close(conn.stopBroadcast)
 		conn.runWaitGroup.Wait()
@@ -568,7 +568,6 @@ func (conn *ObfuscatedPacketConn) readPacket(
 				firstFlowPacket = true
 			} else {
 				isObfuscated = mode.isObfuscated
-				isIETF = mode.isIETF
 			}
 			mode.lastPacketTime = lastPacketTime
 
@@ -636,7 +635,7 @@ func (conn *ObfuscatedPacketConn) readPacket(
 				// There's a possible race condition between the two instances of locking
 				// peerModesMutex: the client might redial in the meantime. Check that the
 				// mode state is unchanged from when the lock was last held.
-				if !ok || mode.isObfuscated != true || mode.isIETF != false ||
+				if !ok || !mode.isObfuscated || mode.isIETF ||
 					mode.lastPacketTime != lastPacketTime {
 					conn.peerModesMutex.Unlock()
 					return n, oobn, flags, addr, true, newTemporaryNetError(
@@ -764,7 +763,7 @@ func (conn *ObfuscatedPacketConn) writePacket(
 			}
 
 			nonce := buffer[0:NONCE_SIZE]
-			conn.noncePRNG.Read(nonce)
+			_, _ = conn.noncePRNG.Read(nonce)
 
 			// This transform may reduce the entropy of the nonce, which increases
 			// the chance of nonce reuse. However, this chacha20 encryption is for
@@ -782,7 +781,7 @@ func (conn *ObfuscatedPacketConn) writePacket(
 			buffer[NONCE_SIZE] = uint8(paddingLen)
 
 			padding := buffer[(NONCE_SIZE + 1) : (NONCE_SIZE+1)+paddingLen]
-			conn.paddingPRNG.Read(padding)
+			_, _ = conn.paddingPRNG.Read(padding)
 
 			copy(buffer[(NONCE_SIZE+1)+paddingLen:], p)
 			dataLen := (NONCE_SIZE + 1) + paddingLen + n

psiphon/common/resolver/resolver.go

@@ -1523,7 +1523,10 @@ func performDNSQuery(
 	startTime := time.Now()
 
 	// Send the DNS request
-	dnsConn.WriteMsg(request)
+	err := dnsConn.WriteMsg(request)
+	if err != nil {
+		return nil, nil, -1, errors.Trace(err)
+	}
 
 	// Read and process the DNS response
 	var IPs []net.IP

psiphon/common/resolver/resolver_test.go

@@ -647,7 +647,7 @@ func runTestResolver() error {
 
 	cancelFunc()
 
-	IPs, err = resolver.ResolveIP(ctx, networkID, params, exampleDomain)
+	_, err = resolver.ResolveIP(ctx, networkID, params, exampleDomain)
 	if err == nil {
 		return errors.TraceNew("unexpected success")
 	}

psiphon/common/tactics/tactics.go

@@ -800,19 +800,6 @@ func (server *Server) GetTacticsPayload(
 	return payload, nil
 }
 
-func marshalTactics(tactics *Tactics) ([]byte, string, error) {
-	marshaledTactics, err := json.Marshal(tactics)
-	if err != nil {
-		return nil, "", errors.Trace(err)
-	}
-
-	// MD5 hash is used solely as a data checksum and not for any security purpose.
-	digest := md5.Sum(marshaledTactics)
-	tag := hex.EncodeToString(digest[:])
-
-	return marshaledTactics, tag, nil
-}
-
 // GetTacticsWithTag returns a GetTactics value along with the associated tag value.
 //
 // Callers must not mutate returned tactics data, which is cached.
@@ -1264,7 +1251,13 @@ func (server *Server) handleSpeedTestRequest(
 	}
 
 	w.WriteHeader(http.StatusOK)
-	w.Write(response)
+	_, err = w.Write(response)
+	if err != nil {
+		server.logger.WithTraceFields(
+			common.LogFields{"error": err}).Warning("failed to write response")
+		common.TerminateHTTPConnection(w, r)
+		return
+	}
 }
 
 func (server *Server) handleTacticsRequest(
@@ -1336,8 +1329,13 @@ func (server *Server) handleTacticsRequest(
 	}
 
 	w.WriteHeader(http.StatusOK)
-	w.Write(boxedResponse)
-
+	_, err = w.Write(boxedResponse)
+	if err != nil {
+		server.logger.WithTraceFields(
+			common.LogFields{"error": err}).Warning("failed to write response")
+		common.TerminateHTTPConnection(w, r)
+		return
+	}
 	// Log a metric.
 
 	logFields := server.logFieldFormatter(geoIPData, apiParams)

psiphon/common/utils.go

@@ -136,8 +136,8 @@ func TruncateTimestampToHour(timestamp string) string {
 func Compress(data []byte) []byte {
 	var compressedData bytes.Buffer
 	writer := zlib.NewWriter(&compressedData)
-	writer.Write(data)
-	writer.Close()
+	_, _ = writer.Write(data)
+	_ = writer.Close()
 	return compressedData.Bytes()
 }
 

psiphon/controller.go

@@ -80,8 +80,6 @@ type Controller struct {
 	candidateServerEntries                  chan *candidateServerEntry
 	untunneledDialConfig                    *DialConfig
 	untunneledSplitTunnelClassifications    *lrucache.Cache
-	splitTunnelClassificationTTL            time.Duration
-	splitTunnelClassificationMaxEntries     int
 	signalFetchCommonRemoteServerList       chan struct{}
 	signalFetchObfuscatedServerLists        chan struct{}
 	signalDownloadUpgrade                   chan string
@@ -1171,7 +1169,11 @@ func (controller *Controller) registerTunnel(tunnel *Tunnel) bool {
 	// Connecting to a TargetServerEntry does not change the
 	// ranking.
 	if controller.config.TargetServerEntry == "" {
-		PromoteServerEntry(controller.config, tunnel.dialParams.ServerEntry.IpAddress)
+		err := PromoteServerEntry(controller.config, tunnel.dialParams.ServerEntry.IpAddress)
+		if err != nil {
+			NoticeWarning("PromoteServerEntry failed: %v", errors.Trace(err))
+			// Proceed with using tunnel
+		}
 	}
 
 	return true
@@ -1414,7 +1416,7 @@ func (controller *Controller) Dial(
 		// The server has indicated that the client should make a direct,
 		// untunneled dial. Cache the classification to avoid this round trip in
 		// the immediate future.
-		untunneledCache.Add(remoteAddr, true, lrucache.DefaultExpiration)
+		untunneledCache.Set(remoteAddr, true, lrucache.DefaultExpiration)
 	}
 
 	NoticeUntunneled(remoteAddr)
@@ -2255,7 +2257,7 @@ loop:
 			if err != nil {
 				NoticeError("failed to get next candidate: %v", errors.Trace(err))
 				controller.SignalComponentFailure()
-				break loop
+				return
 			}
 			if serverEntry == nil {
 				// Completed this iteration
@@ -2414,7 +2416,12 @@ loop:
 		}
 		timer.Stop()
 
-		iterator.Reset()
+		err := iterator.Reset()
+		if err != nil {
+			NoticeError("failed to reset iterator: %v", errors.Trace(err))
+			controller.SignalComponentFailure()
+			return
+		}
 	}
 }
 
@@ -2749,6 +2756,9 @@ loop:
 
 			// Clear the reference to this discarded tunnel and immediately run
 			// a garbage collection to reclaim its memory.
+			//
+			// Note: this assignment is flagged by github.com/gordonklaus/ineffassign,
+			// but should still have some effect on garbage collection?
 			tunnel = nil
 			DoGarbageCollection()
 		}
@@ -2829,6 +2839,7 @@ func (controller *Controller) runInproxyProxy() {
 
 	p := controller.config.GetParameters().Get()
 	allowProxy := p.Bool(parameters.InproxyAllowProxy)
+	activityNoticePeriod := p.Duration(parameters.InproxyProxyTotalActivityNoticePeriod)
 	p.Close()
 
 	// Running an upstream proxy is also an incompatible case.
@@ -2867,7 +2878,6 @@ func (controller *Controller) runInproxyProxy() {
 	// and formatting when debug logging is off.
 	debugLogging := controller.config.InproxyEnableWebRTCDebugLogging
 
-	activityNoticePeriod := p.Duration(parameters.InproxyProxyTotalActivityNoticePeriod)
 	var lastActivityNotice time.Time
 	var lastActivityConnectingClients, lastActivityConnectedClients int32
 	var lastActivityConnectingClientsTotal, lastActivityConnectedClientsTotal int32