diff --git a/psiphon/common/authPackage.go b/psiphon/common/authPackage.go index fecc548cc..cb0fbacbf 100644 --- a/psiphon/common/authPackage.go +++ b/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() diff --git a/psiphon/common/inproxy/client.go b/psiphon/common/inproxy/client.go index 7d941fed0..dd0888b52 100644 --- a/psiphon/common/inproxy/client.go +++ b/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 diff --git a/psiphon/common/inproxy/inproxy_test.go b/psiphon/common/inproxy/inproxy_test.go index d2634dcc1..838073b79 100644 --- a/psiphon/common/inproxy/inproxy_test.go +++ b/psiphon/common/inproxy/inproxy_test.go @@ -535,7 +535,6 @@ func runTestInproxy(doMustUpgrade bool) error { DialAddress: addr, PackedDestinationServerEntry: packedDestinationServerEntry, MustUpgrade: func() { - fmt.Printf("HI!\n") close(receivedClientMustUpgrade) cancelDial() }, diff --git a/psiphon/common/inproxy/proxy.go b/psiphon/common/inproxy/proxy.go index cf4a74e23..0fbb5cf6f 100644 --- a/psiphon/common/inproxy/proxy.go +++ b/psiphon/common/inproxy/proxy.go @@ -58,6 +58,8 @@ type Proxy struct { config *ProxyConfig activityUpdateWrapper *activityUpdateWrapper + lastConnectingClients int32 + lastConnectedClients int32 networkDiscoveryMutex sync.Mutex networkDiscoveryRunOnce bool @@ -242,6 +244,9 @@ func (p *Proxy) Run(ctx context.Context) { // for PeakUp/DownstreamBytesPerSecond. This is also a reasonable // frequency for invoking the ActivityUpdater and updating UI widgets. + p.lastConnectingClients = 0 + p.lastConnectedClients = 0 + activityUpdatePeriod := 1 * time.Second ticker := time.NewTicker(activityUpdatePeriod) defer ticker.Stop() @@ -287,11 +292,16 @@ func (p *Proxy) activityUpdate(period time.Duration) { greaterThanSwapInt64(&p.peakBytesUp, bytesUp) greaterThanSwapInt64(&p.peakBytesDown, bytesDown) - if connectingClients == 0 && - connectedClients == 0 && + clientsChanged := connectingClients != p.lastConnectingClients || + connectedClients != p.lastConnectedClients + + p.lastConnectingClients = connectingClients + p.lastConnectedClients = connectedClients + + if !clientsChanged && bytesUp == 0 && bytesDown == 0 { - // Skip the activity callback on idle. + // Skip the activity callback on idle bytes or no change in client counts. return } diff --git a/psiphon/common/inproxy/session_test.go b/psiphon/common/inproxy/session_test.go index 8e238b5e1..4ba9f9c72 100644 --- a/psiphon/common/inproxy/session_test.go +++ b/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) diff --git a/psiphon/common/packetman/packetman.go b/psiphon/common/packetman/packetman.go index c29b63f5c..ba813d932 100644 --- a/psiphon/common/packetman/packetman.go +++ b/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 { diff --git a/psiphon/common/parameters/parameters.go b/psiphon/common/parameters/parameters.go index dca7ae40f..759f5130e 100644 --- a/psiphon/common/parameters/parameters.go +++ b/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 } diff --git a/psiphon/common/prng/prng.go b/psiphon/common/prng/prng.go index df6c54139..6ed6d29c1 100644 --- a/psiphon/common/prng/prng.go +++ b/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 } diff --git a/psiphon/common/quic/obfuscator.go b/psiphon/common/quic/obfuscator.go index 269ec3c55..29f221cbe 100644 --- a/psiphon/common/quic/obfuscator.go +++ 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 diff --git a/psiphon/common/resolver/resolver.go b/psiphon/common/resolver/resolver.go index 887ad9c69..5d475cdd3 100644 --- a/psiphon/common/resolver/resolver.go +++ b/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 diff --git a/psiphon/common/resolver/resolver_test.go b/psiphon/common/resolver/resolver_test.go index 9877263a8..6b0d3e2f0 100644 --- a/psiphon/common/resolver/resolver_test.go +++ b/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") } diff --git a/psiphon/common/tactics/tactics.go b/psiphon/common/tactics/tactics.go index 9086f9a56..81f0fc075 100644 --- a/psiphon/common/tactics/tactics.go +++ b/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) diff --git a/psiphon/common/utils.go b/psiphon/common/utils.go index 0fbc149f6..71c3ff576 100644 --- a/psiphon/common/utils.go +++ b/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() } diff --git a/psiphon/controller.go b/psiphon/controller.go index d07dec205..20bde9952 100644 --- a/psiphon/controller.go +++ b/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 diff --git a/psiphon/dataStore.go b/psiphon/dataStore.go index b2c2ce784..bf3cb7027 100644 --- a/psiphon/dataStore.go +++ b/psiphon/dataStore.go @@ -975,7 +975,10 @@ func (iterator *ServerEntryIterator) Next() (*protocol.ServerEntry, error) { } if doDeleteServerEntry { - deleteServerEntry(iterator.config, serverEntryID) + err := deleteServerEntry(iterator.config, serverEntryID) + NoticeWarning( + "ServerEntryIterator.Next: deleteServerEntry failed: %s", + errors.Trace(err)) continue } @@ -1039,12 +1042,12 @@ func (iterator *ServerEntryIterator) Next() (*protocol.ServerEntry, error) { return errors.Trace(err) } - serverEntries.put(serverEntryID, jsonServerEntryFields) + err = serverEntries.put(serverEntryID, jsonServerEntryFields) if err != nil { return errors.Trace(err) } - serverEntryTags.put([]byte(serverEntryTag), serverEntryID) + err = serverEntryTags.put([]byte(serverEntryTag), serverEntryID) if err != nil { return errors.Trace(err) } @@ -1140,7 +1143,7 @@ func pruneServerEntry(config *Config, serverEntryTag string) error { var serverEntry *protocol.ServerEntry err := json.Unmarshal(serverEntryJson, &serverEntry) if err != nil { - errors.Trace(err) + return errors.Trace(err) } // Only prune sufficiently old server entries. This mitigates the case where @@ -1148,7 +1151,7 @@ func pruneServerEntry(config *Config, serverEntryTag string) error { // being invalid/deleted. serverEntryLocalTimestamp, err := time.Parse(time.RFC3339, serverEntry.LocalTimestamp) if err != nil { - errors.Trace(err) + return errors.Trace(err) } if serverEntryLocalTimestamp.Add(minimumAgeForPruning).After(time.Now()) { return nil @@ -1162,7 +1165,7 @@ func pruneServerEntry(config *Config, serverEntryTag string) error { err = serverEntryTags.delete(serverEntryTagBytes) if err != nil { - errors.Trace(err) + return errors.Trace(err) } if doDeleteServerEntry { @@ -1174,7 +1177,7 @@ func pruneServerEntry(config *Config, serverEntryTag string) error { keyValues, dialParameters) if err != nil { - errors.Trace(err) + return errors.Trace(err) } } @@ -1231,7 +1234,7 @@ func deleteServerEntry(config *Config, serverEntryID []byte) error { keyValues, dialParameters) if err != nil { - errors.Trace(err) + return errors.Trace(err) } // Remove any tags pointing to the deleted server entry. @@ -1259,7 +1262,7 @@ func deleteServerEntryHelper( err := serverEntries.delete(serverEntryID) if err != nil { - errors.Trace(err) + return errors.Trace(err) } affinityServerEntryID := keyValues.get(datastoreAffinityServerEntryIDKey) @@ -1608,14 +1611,14 @@ func TakeOutUnreportedPersistentStats(config *Config) (map[string][][]byte, erro for key, value := cursor.first(); key != nil; key, value = cursor.next() { // Perform a test JSON unmarshaling. In case of data corruption or a bug, - // delete and skip the record. + // attempt to delete and skip the record. var jsonData interface{} err := json.Unmarshal(key, &jsonData) if err != nil { NoticeWarning( "Invalid key in TakeOutUnreportedPersistentStats: %s: %s", string(key), err) - bucket.delete(key) + _ = bucket.delete(key) continue } diff --git a/psiphon/inproxy.go b/psiphon/inproxy.go index 93dc8d0f3..a75b37c9c 100644 --- a/psiphon/inproxy.go +++ b/psiphon/inproxy.go @@ -1074,11 +1074,9 @@ func MakeInproxyBrokerDialParameters( currentTimestamp := time.Now() - var brokerDialParams *InproxyBrokerDialParameters - // Select new broker dial parameters - brokerDialParams = &InproxyBrokerDialParameters{ + brokerDialParams := &InproxyBrokerDialParameters{ brokerSpec: brokerSpec, LastUsedTimestamp: currentTimestamp, LastUsedBrokerSpecHash: hashBrokerSpec(brokerSpec), @@ -2317,13 +2315,6 @@ func newInproxyUDPConn(ctx context.Context, config *Config) (net.PacketConn, err return conn, nil } -func inproxyUDPAddrFromAddrPort(addrPort netip.AddrPort) *net.UDPAddr { - return &net.UDPAddr{ - IP: addrPort.Addr().AsSlice(), - Port: int(addrPort.Port()), - } -} - func (conn *inproxyUDPConn) ReadFrom(p []byte) (int, net.Addr, error) { // net.UDPConn.ReadFrom currently allocates a &UDPAddr{} per call, and so diff --git a/psiphon/remoteServerList.go b/psiphon/remoteServerList.go index f89b1949c..37bbd758e 100644 --- a/psiphon/remoteServerList.go +++ b/psiphon/remoteServerList.go @@ -167,7 +167,11 @@ func FetchObfuscatedServerLists( // the registry, so clear the ETag to ensure that always happens. _, err := os.Stat(cachedFilename) if os.IsNotExist(err) { - SetUrlETag(canonicalURL, "") + err := SetUrlETag(canonicalURL, "") + if err != nil { + NoticeWarning("SetUrlETag failed: %v", errors.Trace(err)) + // Continue + } } // failed is set if any operation fails and should trigger a retry. When the OSL registry diff --git a/psiphon/server/api.go b/psiphon/server/api.go index 8961c8e67..5a79e9877 100644 --- a/psiphon/server/api.go +++ b/psiphon/server/api.go @@ -1788,7 +1788,7 @@ func isGeoHashString(_ *Config, value string) bool { return false } for _, c := range value { - if strings.Index(geohashAlphabet, string(c)) == -1 { + if !strings.Contains(geohashAlphabet, string(c)) { return false } } diff --git a/psiphon/server/meek.go b/psiphon/server/meek.go index 6334656bf..82ce12477 100644 --- a/psiphon/server/meek.go +++ b/psiphon/server/meek.go @@ -1838,7 +1838,10 @@ func (server *MeekServer) inproxyReloadTactics() error { return errors.Trace(err) } - server.inproxyBroker.SetCommonCompartmentIDs(commonCompartmentIDs) + err = server.inproxyBroker.SetCommonCompartmentIDs(commonCompartmentIDs) + if err != nil { + return errors.Trace(err) + } server.inproxyBroker.SetTimeouts( p.Duration(parameters.InproxyBrokerProxyAnnounceTimeout), diff --git a/psiphon/server/replay.go b/psiphon/server/replay.go index 0840cfbc9..a2721ca58 100644 --- a/psiphon/server/replay.go +++ b/psiphon/server/replay.go @@ -229,7 +229,7 @@ func (r *ReplayCache) SetReplayParameters( r.cacheMutex.Lock() defer r.cacheMutex.Unlock() - r.cache.Add(key, value, TTL) + r.cache.Set(key, value, TTL) // go-cache-lru is typically safe for concurrent access but explicit // synchronization is required when accessing Items. Items may include diff --git a/psiphon/server/server_test.go b/psiphon/server/server_test.go index 261e8fbc7..f5b6c3a5d 100644 --- a/psiphon/server/server_test.go +++ b/psiphon/server/server_test.go @@ -1965,7 +1965,7 @@ func runServer(t *testing.T, runConfig *runServerConfig) { // Access the unexported controller.steeringIPCache controllerStruct := reflect.ValueOf(controller).Elem() - steeringIPCacheField := controllerStruct.Field(40) + steeringIPCacheField := controllerStruct.FieldByName("steeringIPCache") steeringIPCacheField = reflect.NewAt( steeringIPCacheField.Type(), unsafe.Pointer(steeringIPCacheField.UnsafeAddr())).Elem() steeringIPCache := steeringIPCacheField.Interface().(*lrucache.Cache) diff --git a/psiphon/server/tunnelServer.go b/psiphon/server/tunnelServer.go index 034576709..40b3d0597 100644 --- a/psiphon/server/tunnelServer.go +++ b/psiphon/server/tunnelServer.go @@ -1455,8 +1455,10 @@ func (sshServer *sshServer) reloadTactics() error { // for broker public keys no longer in the known/expected list; // but will retain any existing sessions for broker public keys // that remain in the list. - sshServer.inproxyBrokerSessions.SetKnownBrokerPublicKeys(brokerPublicKeys) - + err = sshServer.inproxyBrokerSessions.SetKnownBrokerPublicKeys(brokerPublicKeys) + if err != nil { + return errors.Trace(err) + } } } @@ -1553,7 +1555,7 @@ func (sshServer *sshServer) handleClient( conn.Close() }) } - io.Copy(ioutil.Discard, conn) + _, _ = io.Copy(ioutil.Discard, conn) conn.Close() afterFunc.Stop() @@ -2316,7 +2318,8 @@ func (sshClient *sshClient) run( // It is recommended to set ServerOSSHPrefixSpecs, etc., in default // tactics. - p, err := sshClient.sshServer.support.ServerTacticsParametersCache.Get(sshClient.peerGeoIPData) + var p parameters.ParametersAccessor + p, err = sshClient.sshServer.support.ServerTacticsParametersCache.Get(sshClient.peerGeoIPData) // Log error, but continue. A default prefix spec will be used by the server. if err != nil { @@ -2655,8 +2658,8 @@ func (sshClient *sshClient) authLogCallback(conn ssh.ConnMetadata, method string // I/O, as newly connecting clients need to await stop completion of any // existing connection that shares the same session ID. func (sshClient *sshClient) stop() { - sshClient.sshConn.Close() - sshClient.sshConn.Wait() + _ = sshClient.sshConn.Close() + _ = sshClient.sshConn.Wait() } // awaitStopped will block until sshClient.run has exited, at which point all @@ -3677,7 +3680,7 @@ func (sshClient *sshClient) rejectNewChannel(newChannel ssh.NewChannel, logMessa } // Note: logMessage is internal, for logging only; just the reject reason is sent to the client. - newChannel.Reject(reason, reason.String()) + _ = newChannel.Reject(reason, reason.String()) } // setHandshakeState sets the handshake state -- that it completed and @@ -4783,7 +4786,7 @@ func (sshClient *sshClient) handleTCPChannel( return } - newChannel.Reject(protocol.CHANNEL_REJECT_REASON_SPLIT_TUNNEL, "") + _ = newChannel.Reject(protocol.CHANNEL_REJECT_REASON_SPLIT_TUNNEL, "") return } } diff --git a/psiphon/server/udp.go b/psiphon/server/udp.go index f16fcb0bc..b937b7768 100644 --- a/psiphon/server/udp.go +++ b/psiphon/server/udp.go @@ -367,7 +367,8 @@ type udpgwPortForward struct { var udpgwBufferPool = &sync.Pool{ New: func() any { - return make([]byte, udpgwProtocolMaxMessageSize) + b := make([]byte, udpgwProtocolMaxMessageSize) + return &b }, } @@ -386,10 +387,17 @@ func (portForward *udpgwPortForward) relayDownstream() { // TODO: is the buffer size larger than necessary? // Use a buffer pool to minimize GC churn resulting from frequent, - // short-lived UDP flows, including DNS requests. - buffer := udpgwBufferPool.Get().([]byte) + // short-lived UDP flows, including DNS requests. A pointer to a slice is + // used with sync.Pool to avoid an allocation on Put, as would happen if + // passing in a slice instead of a pointer; see + // https://github.com/dominikh/go-tools/issues/1042#issuecomment-869064445 + // and + // https://github.com/dominikh/go-tools/issues/1336#issuecomment-1331206290 + // (which should not apply here). + b := udpgwBufferPool.Get().(*[]byte) + buffer := *b clear(buffer) - defer udpgwBufferPool.Put(buffer) + defer udpgwBufferPool.Put(b) packetBuffer := buffer[portForward.preambleSize:udpgwProtocolMaxMessageSize] for { diff --git a/psiphon/serverApi.go b/psiphon/serverApi.go index aad7474bf..9e1e39294 100644 --- a/psiphon/serverApi.go +++ b/psiphon/serverApi.go @@ -1459,7 +1459,7 @@ func HandleOSLRequest( defer func() { if retErr != nil { - request.Reply(false, nil) + _ = request.Reply(false, nil) } }() @@ -1470,7 +1470,11 @@ func HandleOSLRequest( } if oslRequest.ClearLocalSLOKs { - DeleteSLOKs() + err := DeleteSLOKs() + if err != nil { + NoticeWarning("DeleteSLOKs failed: %v", errors.Trace(err)) + // Continue + } } seededNewSLOK := false @@ -1479,7 +1483,7 @@ func HandleOSLRequest( duplicate, err := SetSLOK(slok.ID, slok.Key) if err != nil { // TODO: return error to trigger retry? - NoticeWarning("SetSLOK failed: %s", errors.Trace(err)) + NoticeWarning("SetSLOK failed: %v", errors.Trace(err)) } else if !duplicate { seededNewSLOK = true } @@ -1493,7 +1497,10 @@ func HandleOSLRequest( tunnelOwner.SignalSeededNewSLOK() } - request.Reply(true, nil) + err = request.Reply(true, nil) + if err != nil { + return errors.Trace(err) + } return nil } @@ -1503,7 +1510,7 @@ func HandleAlertRequest( defer func() { if retErr != nil { - request.Reply(false, nil) + _ = request.Reply(false, nil) } }() @@ -1517,7 +1524,10 @@ func HandleAlertRequest( NoticeServerAlert(alertRequest) } - request.Reply(true, nil) + err = request.Reply(true, nil) + if err != nil { + return errors.Trace(err) + } return nil } diff --git a/psiphon/tactics.go b/psiphon/tactics.go index b0363e06c..17ad05260 100755 --- a/psiphon/tactics.go +++ b/psiphon/tactics.go @@ -79,7 +79,7 @@ func GetTactics(ctx context.Context, config *Config, useStoredTactics bool) (fet GetTacticsStorer(config), config.GetNetworkID()) if err != nil { - NoticeWarning("get stored tactics failed: %s", err) + NoticeWarning("get stored tactics failed: %s", errors.Trace(err)) // The error will be due to a local datastore problem. // While we could proceed with the tactics request, this @@ -97,7 +97,7 @@ func GetTactics(ctx context.Context, config *Config, useStoredTactics bool) (fet iterator, err := NewTacticsServerEntryIterator(config) if err != nil { - NoticeWarning("tactics iterator failed: %s", err) + NoticeWarning("tactics iterator failed: %s", errors.Trace(err)) return } defer iterator.Close() @@ -113,7 +113,7 @@ func GetTactics(ctx context.Context, config *Config, useStoredTactics bool) (fet serverEntry, err := iterator.Next() if err != nil { - NoticeWarning("tactics iterator failed: %s", err) + NoticeWarning("tactics iterator failed: %s", errors.Trace(err)) return } @@ -126,7 +126,11 @@ func GetTactics(ctx context.Context, config *Config, useStoredTactics bool) (fet return } - iterator.Reset() + err := iterator.Reset() + if err != nil { + NoticeWarning("tactics iterator failed: %s", errors.Trace(err)) + return + } continue } @@ -159,7 +163,7 @@ func GetTactics(ctx context.Context, config *Config, useStoredTactics bool) (fet } } - NoticeWarning("tactics request failed: %s", err) + NoticeWarning("tactics request failed: %s", errors.Trace(err)) // On error, proceed with a retry, as the error is likely // due to a network failure. @@ -190,7 +194,7 @@ func GetTactics(ctx context.Context, config *Config, useStoredTactics bool) (fet err := config.SetParameters( tacticsRecord.Tag, true, tacticsRecord.Tactics.Parameters) if err != nil { - NoticeWarning("apply tactics failed: %s", err) + NoticeWarning("apply tactics failed: %s", errors.Trace(err)) // The error will be due to invalid tactics values from // the server. When SetParameters fails, all @@ -258,7 +262,7 @@ func fetchTactics( return nil, errors.Tracef( "failed to make dial parameters for %s: %v", serverEntry.GetDiagnosticID(), - err) + errors.Trace(err)) } NoticeRequestingTactics(dialParams) diff --git a/psiphon/tlsDialer.go b/psiphon/tlsDialer.go index 437bbba0a..1b678b543 100644 --- a/psiphon/tlsDialer.go +++ b/psiphon/tlsDialer.go @@ -485,29 +485,32 @@ func CustomTLSDial( return nil, errors.Trace(err) } - ss := utls.MakeClientSessionState( + sessionState := utls.MakeClientSessionState( obfuscatedSessionState.SessionTicket, obfuscatedSessionState.Vers, obfuscatedSessionState.CipherSuite, obfuscatedSessionState.MasterSecret, nil, nil) - ss.SetCreatedAt(obfuscatedSessionState.CreatedAt) - ss.SetEMS(obfuscatedSessionState.ExtMasterSecret) + sessionState.SetCreatedAt(obfuscatedSessionState.CreatedAt) + sessionState.SetEMS(obfuscatedSessionState.ExtMasterSecret) // TLS 1.3-only fields - ss.SetAgeAdd(obfuscatedSessionState.AgeAdd) - ss.SetUseBy(obfuscatedSessionState.UseBy) + sessionState.SetAgeAdd(obfuscatedSessionState.AgeAdd) + sessionState.SetUseBy(obfuscatedSessionState.UseBy) if isTLS13 { // Sets OOB PSK if required. if containsPSKExt(utlsClientHelloID, utlsClientHelloSpec) { if wrappedCache, ok := clientSessionCache.(*common.UtlsClientSessionCacheWrapper); ok { - wrappedCache.Put("", ss) + wrappedCache.Put("", sessionState) } else { return nil, errors.TraceNew("unexpected clientSessionCache type") } } } else { - conn.SetSessionState(ss) + err := conn.SetSessionState(sessionState) + if err != nil { + return nil, errors.Trace(err) + } } // Apply changes to utls diff --git a/psiphon/tunnel.go b/psiphon/tunnel.go index e43c08af8..6dc27b11e 100644 --- a/psiphon/tunnel.go +++ b/psiphon/tunnel.go @@ -258,30 +258,38 @@ func (tunnel *Tunnel) Activate( go func() { defer wg.Done() notice := true - select { - case serverRequest := <-tunnel.sshServerRequests: - if serverRequest != nil { - if serverRequest.Type == protocol.PSIPHON_API_INPROXY_RELAY_REQUEST_NAME { - - if notice { - NoticeInfo( - "relaying inproxy broker packets for %s", - tunnel.dialParams.ServerEntry.GetDiagnosticID()) - notice = false + for { + select { + case serverRequest := <-tunnel.sshServerRequests: + if serverRequest != nil { + if serverRequest.Type == protocol.PSIPHON_API_INPROXY_RELAY_REQUEST_NAME { + + if notice { + NoticeInfo( + "relaying inproxy broker packets for %s", + tunnel.dialParams.ServerEntry.GetDiagnosticID()) + notice = false + } + err := tunnel.relayInproxyPacketRoundTrip(handshakeCtx, serverRequest) + if err != nil { + NoticeWarning( + "relay inproxy broker packets failed: %v", + errors.Trace(err)) + // Continue + } + + } else { + + // There's a potential race condition in which + // post-handshake SSH requests, such as OSL or + // alert requests, arrive to this handler instead + // of operateTunnel, so invoke HandleServerRequest here. + HandleServerRequest(tunnelOwner, tunnel, serverRequest) } - tunnel.relayInproxyPacketRoundTrip(handshakeCtx, serverRequest) - - } else { - - // There's a potential race condition in which - // post-handshake SSH requests, such as OSL or - // alert requests, arrive to this handler instead - // of operateTunnel, so invoke HandleServerRequest here. - HandleServerRequest(tunnelOwner, tunnel, serverRequest) } + case <-handshakeCtx.Done(): + return } - case <-handshakeCtx.Done(): - return } }() } @@ -363,7 +371,7 @@ func (tunnel *Tunnel) relayInproxyPacketRoundTrip( defer func() { if retErr != nil { - request.Reply(false, nil) + _ = request.Reply(false, nil) } }() @@ -373,6 +381,9 @@ func (tunnel *Tunnel) relayInproxyPacketRoundTrip( var relayRequest protocol.InproxyRelayRequest err := cbor.Unmarshal(request.Payload, &relayRequest) + if err != nil { + return errors.Trace(err) + } inproxyConn := tunnel.dialParams.inproxyConn.Load().(*inproxy.ClientConn) if inproxyConn == nil {